| Message ID | 20241113215429.3177981-4-terry.bowman@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable CXL PCIe port protocol error handling and logging | expand |
On Wed, Nov 13, 2024 at 03:54:17PM -0600, Terry Bowman wrote: > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -5038,6 +5038,20 @@ static u16 cxl_port_dvsec(struct pci_dev *dev) > PCI_DVSEC_CXL_PORT); > } > > +bool pcie_is_cxl_port(struct pci_dev *dev) > +{ > + if (!pcie_is_cxl(dev)) > + return false; > + > + if ((pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT) && > + (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) && > + (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM)) > + return false; > + > + return cxl_port_dvsec(dev); > +} > +EXPORT_SYMBOL_GPL(pcie_is_cxl_port); This doesn't need to be exported because the only caller introduced in this series is in drivers/pci/pcie/aer.c (in patch 05/15), which is dependent on CONFIG_PCIEAER, which is bool not tristate. The "!pcie_is_cxl(dev)" check at the top of the function is identical to the return value "cxl_port_dvsec(dev)". This looks redundant. However one cannot call pci_pcie_type() without first checking pci_is_pcie(). So I'm wondering if the "!pcie_is_cxl(dev)" check is actually erroneous and supposed to be "!pci_is_pcie(dev)"? That would make more sense to me. Alternatively, just return true instead of "cxl_port_dvsec(dev)". That would probably be the simplest solution here. > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -443,6 +443,7 @@ struct pci_dev { > unsigned int is_hotplug_bridge:1; > unsigned int shpc_managed:1; /* SHPC owned by shpchp */ > unsigned int is_thunderbolt:1; /* Thunderbolt controller */ > + unsigned int is_cxl:1; /* CXL alternate protocol */ I suspect the audience consists mostly of CXL-unaware PCI developers, so spelling out Compute Express Link here (and omitting "alternate protocol" if it doesn't fit) might be more appropriate. Thanks, Lukas
Hi Lukas, I added comments below. On 11/14/2024 9:45 AM, Lukas Wunner wrote: > On Wed, Nov 13, 2024 at 03:54:17PM -0600, Terry Bowman wrote: >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -5038,6 +5038,20 @@ static u16 cxl_port_dvsec(struct pci_dev *dev) >> PCI_DVSEC_CXL_PORT); >> } >> >> +bool pcie_is_cxl_port(struct pci_dev *dev) >> +{ >> + if (!pcie_is_cxl(dev)) >> + return false; >> + >> + if ((pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT) && >> + (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) && >> + (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM)) >> + return false; >> + >> + return cxl_port_dvsec(dev); >> +} >> +EXPORT_SYMBOL_GPL(pcie_is_cxl_port); > This doesn't need to be exported because the only caller introduced > in this series is in drivers/pci/pcie/aer.c (in patch 05/15), which > is dependent on CONFIG_PCIEAER, which is bool not tristate. Ok. > The "!pcie_is_cxl(dev)" check at the top of the function is identical > to the return value "cxl_port_dvsec(dev)". This looks redundant. > However one cannot call pci_pcie_type() without first checking > pci_is_pcie(). So I'm wondering if the "!pcie_is_cxl(dev)" check > is actually erroneous and supposed to be "!pci_is_pcie(dev)"? > That would make more sense to me. I see pcie_is_cxl(dev) is different than cxl_port_dvsec(dev).They check different DVSECs.[1] CXL flexbus DVSEC presence is cached in pci_dev::is_cxl and returned by pcie_is_cxl(). This is used for indicating CXL device. cxl_port_dvsec(dev) returns boolean based on presence of CXL port DVSEC to indicate a CXL port device. I don't believe they are redundant if you consider you can have a CXL device that is not a CXL port device. [1] - CXL 3.1, 8.1.1 Specification, PCIe Designated Vendor-Specific Extended Capability (DVSEC) ID Assignment > Alternatively, just return true instead of "cxl_port_dvsec(dev)". > That would probably be the simplest solution here. > >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -443,6 +443,7 @@ struct pci_dev { >> unsigned int is_hotplug_bridge:1; >> unsigned int shpc_managed:1; /* SHPC owned by shpchp */ >> unsigned int is_thunderbolt:1; /* Thunderbolt controller */ >> + unsigned int is_cxl:1; /* CXL alternate protocol */ > I suspect the audience consists mostly of CXL-unaware PCI developers, > so spelling out Compute Express Link here (and omitting "alternate > protocol" if it doesn't fit) might be more appropriate. > > Thanks, > > Lukas Ok. Regards, Terry
On Thu, Nov 14, 2024 at 10:45:39AM -0600, Bowman, Terry wrote: > On 11/14/2024 9:45 AM, Lukas Wunner wrote: > > On Wed, Nov 13, 2024 at 03:54:17PM -0600, Terry Bowman wrote: > > > --- a/drivers/pci/pci.c > > > +++ b/drivers/pci/pci.c > > > @@ -5038,6 +5038,20 @@ static u16 cxl_port_dvsec(struct pci_dev *dev) > > > PCI_DVSEC_CXL_PORT); > > > } > > > > > > +bool pcie_is_cxl_port(struct pci_dev *dev) > > > +{ > > > + if (!pcie_is_cxl(dev)) > > > + return false; > > > + > > > + if ((pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT) && > > > + (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) && > > > + (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM)) > > > + return false; > > > + > > > + return cxl_port_dvsec(dev); > > > +} > > > +EXPORT_SYMBOL_GPL(pcie_is_cxl_port); > > > > The "!pcie_is_cxl(dev)" check at the top of the function is identical > > to the return value "cxl_port_dvsec(dev)". This looks redundant. > > However one cannot call pci_pcie_type() without first checking > > pci_is_pcie(). So I'm wondering if the "!pcie_is_cxl(dev)" check > > is actually erroneous and supposed to be "!pci_is_pcie(dev)"? > > That would make more sense to me. > > I see pcie_is_cxl(dev) is different than cxl_port_dvsec(dev). > They check different DVSECs. Ah, sorry, I missed that. > CXL flexbus DVSEC presence is cached in pci_dev::is_cxl and returned by > pcie_is_cxl(). This is used for indicating CXL device. > > cxl_port_dvsec(dev) returns boolean based on presence of CXL port DVSEC to > indicate a CXL port device. > > I don't believe they are redundant if you consider you can have a CXL > device that > is not a CXL port device. Can you have a CXL port that is not a CXL device? If not, it would seem to me that checking for Flexbus DVSEC presence *is* redundant. Or do you anticipate broken devices which lack the Flexbus DVSEC and that you explicitly want to exclude? Thanks, Lukas
On 11/14/2024 10:52 AM, Lukas Wunner wrote: > On Thu, Nov 14, 2024 at 10:45:39AM -0600, Bowman, Terry wrote: >> On 11/14/2024 9:45 AM, Lukas Wunner wrote: >>> On Wed, Nov 13, 2024 at 03:54:17PM -0600, Terry Bowman wrote: >>>> --- a/drivers/pci/pci.c >>>> +++ b/drivers/pci/pci.c >>>> @@ -5038,6 +5038,20 @@ static u16 cxl_port_dvsec(struct pci_dev *dev) >>>> PCI_DVSEC_CXL_PORT); >>>> } >>>> >>>> +bool pcie_is_cxl_port(struct pci_dev *dev) >>>> +{ >>>> + if (!pcie_is_cxl(dev)) >>>> + return false; >>>> + >>>> + if ((pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT) && >>>> + (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) && >>>> + (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM)) >>>> + return false; >>>> + >>>> + return cxl_port_dvsec(dev); >>>> +} >>>> +EXPORT_SYMBOL_GPL(pcie_is_cxl_port); >>> The "!pcie_is_cxl(dev)" check at the top of the function is identical >>> to the return value "cxl_port_dvsec(dev)". This looks redundant. >>> However one cannot call pci_pcie_type() without first checking >>> pci_is_pcie(). So I'm wondering if the "!pcie_is_cxl(dev)" check >>> is actually erroneous and supposed to be "!pci_is_pcie(dev)"? >>> That would make more sense to me. >> I see pcie_is_cxl(dev) is different than cxl_port_dvsec(dev). >> They check different DVSECs. > Ah, sorry, I missed that. > >> CXL flexbus DVSEC presence is cached in pci_dev::is_cxl and returned by >> pcie_is_cxl(). This is used for indicating CXL device. >> >> cxl_port_dvsec(dev) returns boolean based on presence of CXL port DVSEC to >> indicate a CXL port device. >> >> I don't believe they are redundant if you consider you can have a CXL >> device that >> is not a CXL port device. > Can you have a CXL port that is not a CXL device? > > If not, it would seem to me that checking for Flexbus DVSEC presence > *is* redundant. Or do you anticipate broken devices which lack the > Flexbus DVSEC and that you explicitly want to exclude? > > Thanks, > > Lukas No, the CXL port device is always a CXL device per spec. This was added to short-circuit the function by returning immediately if the device is _not_ a CXL device. Otherwise for PCIe Port devices, the CXL Port DVSEC will be searched. I was trying to avoid the unnecessary CXL port DVSEC search unless the other criteria are met. And I expect most cases will not be a CXL device. I will remove the "if (!pcie_is_cxl(dev))" block as you suggested. Regards, Terry
On Thu, Nov 14, 2024 at 11:07:26AM -0600, Bowman, Terry wrote: > > Can you have a CXL port that is not a CXL device? > > > > If not, it would seem to me that checking for Flexbus DVSEC presence > > *is* redundant. Or do you anticipate broken devices which lack the > > Flexbus DVSEC and that you explicitly want to exclude? > > No, the CXL port device is always a CXL device per spec. > > This was added to short-circuit the function by returning immediately > if the device is _not_ a CXL device. Otherwise for PCIe Port devices, > the CXL Port DVSEC will be searched. I was trying to avoid the unnecessary > CXL port DVSEC search unless the other criteria are met. > And I expect most cases will not be a CXL device. > > I will remove the "if (!pcie_is_cxl(dev))" block as you suggested. Ah, this is meant as a speed-up. Actually that makes sense, so feel free to keep it. If you do remove it, I think you'll have to move the cxl_port_dvsec() invocation up in the function, in front of the pci_pcie_type() checks. The latter require that one first checks that the device is PCIe. That's done implicitly by cxl_port_dvsec() because it returns 0 in the non-PCIe case. (Due to the "if (dev->cfg_size <= PCI_CFG_SPACE_SIZE)" check in pci_find_next_ext_capability().) Another idea would be to put a "if (!pcie_is_cxl(dev)) return 0;" speed-up in cxl_port_dvsec() so that the other caller benefits from it as well. Thanks, Lukas
On 11/15/2024 2:47 AM, Lukas Wunner wrote: > On Thu, Nov 14, 2024 at 11:07:26AM -0600, Bowman, Terry wrote: >>> Can you have a CXL port that is not a CXL device? >>> >>> If not, it would seem to me that checking for Flexbus DVSEC presence >>> *is* redundant. Or do you anticipate broken devices which lack the >>> Flexbus DVSEC and that you explicitly want to exclude? >> No, the CXL port device is always a CXL device per spec. >> >> This was added to short-circuit the function by returning immediately >> if the device is _not_ a CXL device. Otherwise for PCIe Port devices, >> the CXL Port DVSEC will be searched. I was trying to avoid the unnecessary >> CXL port DVSEC search unless the other criteria are met. >> And I expect most cases will not be a CXL device. >> >> I will remove the "if (!pcie_is_cxl(dev))" block as you suggested. > Ah, this is meant as a speed-up. Actually that makes sense, > so feel free to keep it. > > If you do remove it, I think you'll have to move the cxl_port_dvsec() > invocation up in the function, in front of the pci_pcie_type() checks. > The latter require that one first checks that the device is PCIe. > That's done implicitly by cxl_port_dvsec() because it returns 0 in > the non-PCIe case. (Due to the "if (dev->cfg_size <= PCI_CFG_SPACE_SIZE)" > check in pci_find_next_ext_capability().) > > Another idea would be to put a "if (!pcie_is_cxl(dev)) return 0;" speed-up > in cxl_port_dvsec() so that the other caller benefits from it as well. > > Thanks, > > Lukas Ok, I'll look at adding the same pcie_is_cxl() call and check in cxl_port_devsec(). Regards, Terry
On Fri, Nov 15, 2024 at 07:54:37AM -0600, Bowman, Terry wrote: > On 11/15/2024 2:47 AM, Lukas Wunner wrote: > > On Thu, Nov 14, 2024 at 11:07:26AM -0600, Bowman, Terry wrote: > > > I will remove the "if (!pcie_is_cxl(dev))" block as you suggested. > > > > Ah, this is meant as a speed-up. Actually that makes sense, > > so feel free to keep it. > > > > If you do remove it, I think you'll have to move the cxl_port_dvsec() > > invocation up in the function, in front of the pci_pcie_type() checks. > > The latter require that one first checks that the device is PCIe. > > That's done implicitly by cxl_port_dvsec() because it returns 0 in > > the non-PCIe case. (Due to the "if (dev->cfg_size <= PCI_CFG_SPACE_SIZE)" > > check in pci_find_next_ext_capability().) > > > > Another idea would be to put a "if (!pcie_is_cxl(dev)) return 0;" speed-up > > in cxl_port_dvsec() so that the other caller benefits from it as well. > > Ok, I'll look at adding the same pcie_is_cxl() call and check in > cxl_port_devsec(). If you put "if (!pcie_is_cxl(dev)) return 0;" in cxl_port_devsec() and move the call to cxl_port_devsec() in pcie_is_cxl_port() up in front of the pci_pcie_type() checks, I think you won't need an additional "!pcie_is_cxl(dev)" check in pcie_is_cxl_port(). Thanks, Lukas
On 11/17/2024 11:02 AM, Lukas Wunner wrote: > On Fri, Nov 15, 2024 at 07:54:37AM -0600, Bowman, Terry wrote: >> On 11/15/2024 2:47 AM, Lukas Wunner wrote: >>> On Thu, Nov 14, 2024 at 11:07:26AM -0600, Bowman, Terry wrote: >>>> I will remove the "if (!pcie_is_cxl(dev))" block as you suggested. >>> Ah, this is meant as a speed-up. Actually that makes sense, >>> so feel free to keep it. >>> >>> If you do remove it, I think you'll have to move the cxl_port_dvsec() >>> invocation up in the function, in front of the pci_pcie_type() checks. >>> The latter require that one first checks that the device is PCIe. >>> That's done implicitly by cxl_port_dvsec() because it returns 0 in >>> the non-PCIe case. (Due to the "if (dev->cfg_size <= PCI_CFG_SPACE_SIZE)" >>> check in pci_find_next_ext_capability().) >>> >>> Another idea would be to put a "if (!pcie_is_cxl(dev)) return 0;" speed-up >>> in cxl_port_dvsec() so that the other caller benefits from it as well. >> Ok, I'll look at adding the same pcie_is_cxl() call and check in >> cxl_port_devsec(). > If you put "if (!pcie_is_cxl(dev)) return 0;" in cxl_port_devsec() > and move the call to cxl_port_devsec() in pcie_is_cxl_port() up in front > of the pci_pcie_type() checks, I think you won't need an additional > "!pcie_is_cxl(dev)" check in pcie_is_cxl_port(). > > Thanks, > > Lukas Ok. I'll make that change. Regards, Terry
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 225a6cd2e9ca..6db6c171df54 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -5038,6 +5038,20 @@ static u16 cxl_port_dvsec(struct pci_dev *dev) PCI_DVSEC_CXL_PORT); } +bool pcie_is_cxl_port(struct pci_dev *dev) +{ + if (!pcie_is_cxl(dev)) + return false; + + if ((pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT) && + (pci_pcie_type(dev) != PCI_EXP_TYPE_UPSTREAM) && + (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM)) + return false; + + return cxl_port_dvsec(dev); +} +EXPORT_SYMBOL_GPL(pcie_is_cxl_port); + static bool cxl_sbr_masked(struct pci_dev *dev) { u16 dvsec, reg; diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index f1615805f5b0..277e3fc8e1a7 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1631,6 +1631,14 @@ static void set_pcie_thunderbolt(struct pci_dev *dev) dev->is_thunderbolt = 1; } +static void set_pcie_cxl(struct pci_dev *dev) +{ + u16 dvsec = pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL, + PCI_DVSEC_CXL_FLEXBUS); + if (dvsec) + dev->is_cxl = 1; +} + static void set_pcie_untrusted(struct pci_dev *dev) { struct pci_dev *parent; @@ -1945,6 +1953,8 @@ int pci_setup_device(struct pci_dev *dev) /* Need to have dev->cfg_size ready */ set_pcie_thunderbolt(dev); + set_pcie_cxl(dev); + set_pcie_untrusted(dev); /* "Unknown power state" */ diff --git a/include/linux/pci.h b/include/linux/pci.h index 106ac83e3a7b..d3b1af9fb273 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -443,6 +443,7 @@ struct pci_dev { unsigned int is_hotplug_bridge:1; unsigned int shpc_managed:1; /* SHPC owned by shpchp */ unsigned int is_thunderbolt:1; /* Thunderbolt controller */ + unsigned int is_cxl:1; /* CXL alternate protocol */ /* * Devices marked being untrusted are the ones that can potentially * execute DMA attacks and similar. They are typically connected @@ -743,6 +744,9 @@ static inline bool pci_is_vga(struct pci_dev *pdev) return false; } +#define pcie_is_cxl(dev) (dev->is_cxl) +bool pcie_is_cxl_port(struct pci_dev *dev); + #define for_each_pci_bridge(dev, bus) \ list_for_each_entry(dev, &bus->devices, bus_list) \ if (!pci_is_bridge(dev)) {} else diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h index 12323b3334a9..5df6c74963c5 100644 --- a/include/uapi/linux/pci_regs.h +++ b/include/uapi/linux/pci_regs.h @@ -1186,9 +1186,10 @@ #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL 0x00ff0000 #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX 0xff000000 -/* Compute Express Link (CXL r3.1, sec 8.1.5) */ +/* Compute Express Link (CXL r3.1, sec 8.1) */ #define PCI_DVSEC_CXL_PORT 3 #define PCI_DVSEC_CXL_PORT_CTL 0x0c #define PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR 0x00000001 +#define PCI_DVSEC_CXL_FLEXBUS 7 #endif /* LINUX_PCI_REGS_H */
CXL and AER drivers need the ability to identify CXL devices and CXL port devices. First, add set_pcie_cxl() with logic checking for CXL Flexbus DVSEC presence. The CXL Flexbus DVSEC presence is used because it is required for all the CXL PCIe devices.[1] Add boolean 'struct pci_dev::is_cxl' with the purpose to cache the CXL Flexbus presence. Add pcie_is_cxl() as a macro to return 'struct pci_dev::is_cxl', Add pcie_is_cxl_port() to check if a device is a CXL root port, CXL upstream switch port, or CXL downstream switch port. Also, verify the CXL extensions DVSEC for port is present.[1] [1] CXL 3.1 Spec, 8.1.1 PCIe Designated Vendor-Specific Extended Capability (DVSEC) ID Assignment, Table 8-2 Signed-off-by: Terry Bowman <terry.bowman@amd.com> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> Reviewed-by: Fan Ni <fan.ni@samsung.com> --- drivers/pci/pci.c | 14 ++++++++++++++ drivers/pci/probe.c | 10 ++++++++++ include/linux/pci.h | 4 ++++ include/uapi/linux/pci_regs.h | 3 ++- 4 files changed, 30 insertions(+), 1 deletion(-)