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

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

[Bjorn Helgaas]

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.

In this patch, we only find a VSEC Capability that matches
(PCI_VENDOR_ID_ALIBABA, DWC_PCIE_VSEC_RAS_DES_ID), but as of
v6.13-rc1, it finds all of these:

  (PCI_VENDOR_ID_ALIBABA, DWC_PCIE_VSEC_RAS_DES_ID)
  (PCI_VENDOR_ID_AMPERE, DWC_PCIE_VSEC_RAS_DES_ID)
  (PCI_VENDOR_ID_QCOM, DWC_PCIE_VSEC_RAS_DES_ID)

There is no assurance that DWC_PCIE_VSEC_RAS_DES_ID means the same
thing to Alibaba, Ampere, and Qualcomm because each company defines
what 0x02 means to it.  PCIe r6.0, sec 7.9.5 for details.

I alluded to this earlier [1] and suggested that these devices should
have used a Designated Vendor-Specific (DVSEC) Capability instead of a 
Vendor-Specific (VSEC) Capability.

But since they didn't, I think the dwc_pcie_vendor_ids[] table is a
dangerous way to work around this because it suggests that all we have
to do is add new vendors to that table.

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.

> +		if (vsec)
> +			break;
> +	}
> +	if (!vsec)
> +		return false;
> +
> +	pci_read_config_dword(pdev, vsec + PCI_VNDR_HEADER, &val);
> +	if (PCI_VNDR_HEADER_REV(val) != 0x04)
> +		return false;
> +
> +	pci_dbg(pdev,
> +		"Detected PCIe Vendor-Specific Extended Capability RAS DES\n");
> +	return true;
> +}

> +static int dwc_pcie_pmu_probe(struct platform_device *plat_dev)
> +{
> +	struct pci_dev *pdev = plat_dev->dev.platform_data;
> +	struct dwc_pcie_pmu *pcie_pmu;
> +	char *name;
> +	u32 bdf, val;
> +	u16 vsec;
> +	int ret;
> +
> +	vsec = pci_find_vsec_capability(pdev, pdev->vendor,
> +					DWC_PCIE_VSEC_RAS_DES_ID);
> +     pci_read_config_dword(pdev, vsec + PCI_VNDR_HEADER, &val);

Nit: "val" is never used, so why read it?

This looks even more wrong, because this matches ANY VSEC Capability
from ANY vendor that happens to be numbered DWC_PCIE_VSEC_RAS_DES_ID.

I know this is indirectly qualified by the check above in
dwc_pcie_match_des_cap(), but duplicating this here just spreads the
confusion about how to interpret VSEC IDs.

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.

Bjorn

[1] https://lore.kernel.org/r/20231012162512.GA1069387@bhelgaas

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

[Will Deacon]

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

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

[Bjorn Helgaas]

On Mon, Dec 09, 2024 at 03:40:16PM +0000, Will Deacon wrote:
> 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.

Yep, for sure.

> 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.

Posted at https://lore.kernel.org/linux-pci/20241209222938.3219364-1-helgaas@kernel.org

Bjorn

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

[Shuai Xue]

在 2024/12/10 06:47, Bjorn Helgaas 写道:
> On Mon, Dec 09, 2024 at 03:40:16PM +0000, Will Deacon wrote:
>> 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.
> 
> Yep, for sure.
> 
>> 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.
> 
> Posted at https://lore.kernel.org/linux-pci/20241209222938.3219364-1-helgaas@kernel.org
> 
> Bjorn

Hi, Bjorn and Will,

Thanks for pointing this out and fix it so quickly, I will also test this patch
on our platform later.

Best Regards,
Shuai