linux-kernel - Re: [PATCH v12 4/5] drivers/perf: add DesignWare PCIe PMU driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241209154015.GA12428@willie-the-truck>
Date: Mon, 9 Dec 2024 15:40:16 +0000
From: Will Deacon <will@...nel.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Shuai Xue <xueshuai@...ux.alibaba.com>, ilkka@...amperecomputing.com,
	kaishen@...ux.alibaba.com, yangyicong@...wei.com,
	Jonathan.Cameron@...wei.com, baolin.wang@...ux.alibaba.com,
	robin.murphy@....com, chengyou@...ux.alibaba.com,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-pci@...r.kernel.org, rdunlap@...radead.org,
	mark.rutland@....com, zhuo.song@...ux.alibaba.com,
	renyu.zj@...ux.alibaba.com
Subject: Re: [PATCH v12 4/5] drivers/perf: add DesignWare PCIe PMU driver

Hi Bjorn,

On Fri, Dec 06, 2024 at 10:54:57AM -0600, Bjorn Helgaas wrote:
> On Fri, Dec 08, 2023 at 10:56:51AM +0800, Shuai Xue wrote:
> > This commit adds the PCIe Performance Monitoring Unit (PMU) driver support
> > for T-Head Yitian SoC chip. Yitian is based on the Synopsys PCI Express
> > Core controller IP which provides statistics feature. The PMU is a PCIe
> > configuration space register block provided by each PCIe Root Port in a
> > Vendor-Specific Extended Capability named RAS D.E.S (Debug, Error
> > injection, and Statistics).
> 
> > +#define DWC_PCIE_VSEC_RAS_DES_ID		0x02
> 
> > +static const struct dwc_pcie_vendor_id dwc_pcie_vendor_ids[] = {
> > +	{.vendor_id = PCI_VENDOR_ID_ALIBABA },
> > +	{} /* terminator */
> > +};
> 
> > +static bool dwc_pcie_match_des_cap(struct pci_dev *pdev)
> > +{
> > +	const struct dwc_pcie_vendor_id *vid;
> > +	u16 vsec;
> > +	u32 val;
> > +
> > +	if (!pci_is_pcie(pdev) || !(pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT))
> > +		return false;
> > +
> > +	for (vid = dwc_pcie_vendor_ids; vid->vendor_id; vid++) {
> > +		vsec = pci_find_vsec_capability(pdev, vid->vendor_id,
> > +						DWC_PCIE_VSEC_RAS_DES_ID);
> 
> This looks wrong to me, and it promotes a misunderstanding of how VSEC
> Capabilities work.  The VSEC ID is defined by the vendor, so we have
> to check both the Vendor ID and the VSEC ID before we know what this
> VSEC Capability is.

Thanks for pointing this out! The code's been merged for a while now,
so we'll need to fix what we have rather than revert it, I think.

[...]

> I think the table should be extended to contain the Vendor ID, *and*
> the VSEC ID, *and* the VSEC Rev used by that vendor, i.e., it should
> look like this:
> 
>   struct dwc_pcie_pmu_vsec {
>     u16 vendor_id;
>     u16 vsec_id;
>     u8 vsec_rev;
>   };
> 
>   struct dwc_pcie_pmu_vsec dwc_pcie_pmu_vsec_ids[] = {
>     { .vendor_id = PCI_VENDOR_ID_ALIBABA,
>       .vsec_id = DWC_PCIE_VSEC_RAS_DES_ID, .vsec_rev = 0x4 },
>     { .vendor_id = PCI_VENDOR_ID_AMPERE,
>       .vsec_id = DWC_PCIE_VSEC_RAS_DES_ID, .vsec_rev = 0x4 },
>     { .vendor_id = PCI_VENDOR_ID_QCOM,
>       .vsec_id = DWC_PCIE_VSEC_RAS_DES_ID, .vsec_rev = 0x4 },
>     {}
>   };
> 
> This *looks* the same, but it's not, because it makes it obvious that
> the VSEC ID and VSEC Rev are defined separately by each vendor.  It's
> just a lucky coincidence that they happen to be the same for these
> vendors.

[...]

> I suggest updating dwc_pcie_match_des_cap() to iterate through the
> dwc_pcie_pmu_vsec_ids[] table and return the capability offset so you
> can call it from here.

Any chance you could send a patch with those, please? I'm also not able
to test this stuff, but I'm sure Ilkka would help us out.

Cheers,

Will