Message ID | 20250107143852.3692571-4-terry.bowman@amd.com |
---|---|
State | New |
Headers | show |
Series | Enable CXL PCIe port protocol error handling and logging | expand |
Terry Bowman wrote: > 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 Ports 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 | 13 +++++++++++++ > drivers/pci/probe.c | 10 ++++++++++ > include/linux/pci.h | 4 ++++ > include/uapi/linux/pci_regs.h | 3 ++- > 4 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 661f98c6c63a..9319c62e3488 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -5036,10 +5036,23 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe) > > static u16 cxl_port_dvsec(struct pci_dev *dev) > { > + if (!pcie_is_cxl(dev)) > + return 0; > + > return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL, > PCI_DVSEC_CXL_PORT); > } > > +bool pcie_is_cxl_port(struct pci_dev *dev) > +{ > + 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); Returning bool from a function which returns u16 is odd and I don't think it should be coded this way. I don't think it is wrong right now but this really ought to code the pcie_is_cxl() here and leave cxl_port_dvsec() alone. Calling cxl_port_dvsec(), checking for if the dvsec exists, and returning bool. > +} > + [snip] > diff --git a/include/linux/pci.h b/include/linux/pci.h > index e2e36f11205c..08350302b3e9 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -452,6 +452,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; /* Compute Express Link (CXL) */ > /* > * Devices marked being untrusted are the ones that can potentially > * execute DMA attacks and similar. They are typically connected > @@ -739,6 +740,9 @@ static inline bool pci_is_vga(struct pci_dev *pdev) > return false; > } > > +#define pcie_is_cxl(dev) (dev->is_cxl) This should be an inline function which takes struct pci_dev * for type safety. Ira [snip]
On 1/13/2025 5:49 PM, Ira Weiny wrote: > Terry Bowman wrote: >> 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 Ports 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 | 13 +++++++++++++ >> drivers/pci/probe.c | 10 ++++++++++ >> include/linux/pci.h | 4 ++++ >> include/uapi/linux/pci_regs.h | 3 ++- >> 4 files changed, 29 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index 661f98c6c63a..9319c62e3488 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -5036,10 +5036,23 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe) >> >> static u16 cxl_port_dvsec(struct pci_dev *dev) >> { >> + if (!pcie_is_cxl(dev)) >> + return 0; >> + >> return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL, >> PCI_DVSEC_CXL_PORT); >> } >> >> +bool pcie_is_cxl_port(struct pci_dev *dev) >> +{ >> + 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); > Returning bool from a function which returns u16 is odd and I don't think > it should be coded this way. I don't think it is wrong right now but this > really ought to code the pcie_is_cxl() here and leave cxl_port_dvsec() > alone. Calling cxl_port_dvsec(), checking for if the dvsec exists, and > returning bool. Hi Ira, Thanks for reviewing. Is this what you are looking for here: +bool pcie_is_cxl_port(struct pci_dev *dev) +{ + return (cxl_port_dvsec(dev) > 0); >> +} >> + > [snip] > >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index e2e36f11205c..08350302b3e9 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -452,6 +452,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; /* Compute Express Link (CXL) */ >> /* >> * Devices marked being untrusted are the ones that can potentially >> * execute DMA attacks and similar. They are typically connected >> @@ -739,6 +740,9 @@ static inline bool pci_is_vga(struct pci_dev *pdev) >> return false; >> } >> >> +#define pcie_is_cxl(dev) (dev->is_cxl) > This should be an inline function which takes struct pci_dev * for type > safety. > > Ira Ok, Thanks for reviewing the patches. Regards, Terry > [snip]
Bowman, Terry wrote: > > > > On 1/13/2025 5:49 PM, Ira Weiny wrote: > > Terry Bowman wrote: > >> 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 Ports 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 | 13 +++++++++++++ > >> drivers/pci/probe.c | 10 ++++++++++ > >> include/linux/pci.h | 4 ++++ > >> include/uapi/linux/pci_regs.h | 3 ++- > >> 4 files changed, 29 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > >> index 661f98c6c63a..9319c62e3488 100644 > >> --- a/drivers/pci/pci.c > >> +++ b/drivers/pci/pci.c > >> @@ -5036,10 +5036,23 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe) > >> > >> static u16 cxl_port_dvsec(struct pci_dev *dev) > >> { > >> + if (!pcie_is_cxl(dev)) > >> + return 0; > >> + > >> return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL, > >> PCI_DVSEC_CXL_PORT); > >> } > >> > >> +bool pcie_is_cxl_port(struct pci_dev *dev) > >> +{ > >> + 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); > > Returning bool from a function which returns u16 is odd and I don't think > > it should be coded this way. I don't think it is wrong right now but this > > really ought to code the pcie_is_cxl() here and leave cxl_port_dvsec() > > alone. Calling cxl_port_dvsec(), checking for if the dvsec exists, and > > returning bool. > > Hi Ira, > > Thanks for reviewing. Is this what you are looking for here: > > +bool pcie_is_cxl_port(struct pci_dev *dev) > +{ > + return (cxl_port_dvsec(dev) > 0); With the type checks, yes that is more clear. Ira [snip]
On 1/14/2025 5:33 PM, Ira Weiny wrote: > Bowman, Terry wrote: >> >> >> On 1/13/2025 5:49 PM, Ira Weiny wrote: >>> Terry Bowman wrote: >>>> 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 Ports 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 | 13 +++++++++++++ >>>> drivers/pci/probe.c | 10 ++++++++++ >>>> include/linux/pci.h | 4 ++++ >>>> include/uapi/linux/pci_regs.h | 3 ++- >>>> 4 files changed, 29 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >>>> index 661f98c6c63a..9319c62e3488 100644 >>>> --- a/drivers/pci/pci.c >>>> +++ b/drivers/pci/pci.c >>>> @@ -5036,10 +5036,23 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe) >>>> >>>> static u16 cxl_port_dvsec(struct pci_dev *dev) >>>> { >>>> + if (!pcie_is_cxl(dev)) >>>> + return 0; >>>> + >>>> return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL, >>>> PCI_DVSEC_CXL_PORT); >>>> } >>>> >>>> +bool pcie_is_cxl_port(struct pci_dev *dev) >>>> +{ >>>> + 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); >>> Returning bool from a function which returns u16 is odd and I don't think >>> it should be coded this way. I don't think it is wrong right now but this >>> really ought to code the pcie_is_cxl() here and leave cxl_port_dvsec() >>> alone. Calling cxl_port_dvsec(), checking for if the dvsec exists, and >>> returning bool. >> Hi Ira, >> >> Thanks for reviewing. Is this what you are looking for here: >> >> +bool pcie_is_cxl_port(struct pci_dev *dev) >> +{ >> + return (cxl_port_dvsec(dev) > 0); > With the type checks, yes that is more clear. > > Ira > > [snip] Since sending the above I made update to be: static u16 cxl_port_dvsec(struct pci_dev *dev) { return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL, PCI_DVSEC_CXL_PORT); } inline bool pcie_is_cxl(struct pci_dev *pci_dev) { return pci_dev->is_cxl; } bool pcie_is_cxl_port(struct pci_dev *pci_dev) { if (!pcie_is_cxl(pci_dev)) return false; return (cxl_port_dvsec(pci_dev) > 0); } I can change if you see anything is needed. Regards, Terry
On Tue, Jan 14, 2025 at 09:19:12AM -0600, Bowman, Terry wrote: > On 1/13/2025 5:49 PM, Ira Weiny wrote: > > Terry Bowman wrote: > > > --- a/drivers/pci/pci.c > > > +++ b/drivers/pci/pci.c > > > @@ -5036,10 +5036,23 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe) > > > > > > static u16 cxl_port_dvsec(struct pci_dev *dev) > > > { > > > + if (!pcie_is_cxl(dev)) > > > + return 0; > > > + > > > return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL, > > > PCI_DVSEC_CXL_PORT); > > > } > > > > > > +bool pcie_is_cxl_port(struct pci_dev *dev) > > > +{ > > > + 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); > > > > Returning bool from a function which returns u16 is odd and I don't think > > it should be coded this way. I don't think it is wrong right now but this > > really ought to code the pcie_is_cxl() here and leave cxl_port_dvsec() > > alone. Calling cxl_port_dvsec(), checking for if the dvsec exists, and > > returning bool. > > Thanks for reviewing. Is this what you are looking for here: > > +bool pcie_is_cxl_port(struct pci_dev *dev) > +{ > + return (cxl_port_dvsec(dev) > 0); Since cxl_port_dvsec() cannot return a negative integer, you might as well use: return !!cxl_port_dvsec(dev); However last I checked gcc generates code which implicitly turns a number bigger than 1 into a 1 if the return type is bool. (I had to fix a bug caused by this behavior once, see 009f8c90f571). Thanks, Lukas
Bowman, Terry wrote: > > > > On 1/14/2025 5:33 PM, Ira Weiny wrote: > > Bowman, Terry wrote: > >> > >> > >> On 1/13/2025 5:49 PM, Ira Weiny wrote: > >>> Terry Bowman wrote: [snip] > >>>> +bool pcie_is_cxl_port(struct pci_dev *dev) > >>>> +{ > >>>> + 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); > >>> Returning bool from a function which returns u16 is odd and I don't think > >>> it should be coded this way. I don't think it is wrong right now but this > >>> really ought to code the pcie_is_cxl() here and leave cxl_port_dvsec() > >>> alone. Calling cxl_port_dvsec(), checking for if the dvsec exists, and > >>> returning bool. > >> Hi Ira, > >> > >> Thanks for reviewing. Is this what you are looking for here: > >> > >> +bool pcie_is_cxl_port(struct pci_dev *dev) > >> +{ > >> + return (cxl_port_dvsec(dev) > 0); > > With the type checks, yes that is more clear. > > > > Ira > > > > [snip] > Since sending the above I made update to be: > > static u16 cxl_port_dvsec(struct pci_dev *dev) > { > return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL, > PCI_DVSEC_CXL_PORT); > } > > inline bool pcie_is_cxl(struct pci_dev *pci_dev) > { > return pci_dev->is_cxl; > } > > bool pcie_is_cxl_port(struct pci_dev *pci_dev) > { > if (!pcie_is_cxl(pci_dev)) > return false; > > return (cxl_port_dvsec(pci_dev) > 0); > } > > I can change if you see anything is needed. Looks good thanks! Ira [snip]
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 661f98c6c63a..9319c62e3488 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -5036,10 +5036,23 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe) static u16 cxl_port_dvsec(struct pci_dev *dev) { + if (!pcie_is_cxl(dev)) + return 0; + return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL, PCI_DVSEC_CXL_PORT); } +bool pcie_is_cxl_port(struct pci_dev *dev) +{ + 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); +} + 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 2e81ab0f5a25..ee40a1e2ec75 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1633,6 +1633,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 = pci_upstream_bridge(dev); @@ -1963,6 +1971,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); if (pci_is_pcie(dev)) diff --git a/include/linux/pci.h b/include/linux/pci.h index e2e36f11205c..08350302b3e9 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -452,6 +452,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; /* Compute Express Link (CXL) */ /* * Devices marked being untrusted are the ones that can potentially * execute DMA attacks and similar. They are typically connected @@ -739,6 +740,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 1601c7ed5fab..4251af090742 100644 --- a/include/uapi/linux/pci_regs.h +++ b/include/uapi/linux/pci_regs.h @@ -1208,9 +1208,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 */