Hi Jay, thanks for taking a look at this! On Mon, Apr 20, 2020 at 09:21:27AM -0700, Sean V Kelley wrote: > On 18 Apr 2020, at 1:36, Jay Fang wrote: > > On 2020/4/15 8:47, Sean V Kelley wrote: > > > > > > [1] https://www.computeexpresslink.org/ > > > > > > Signed-off-by: Sean V Kelley <sean.v.kelley@xxxxxxxxxxxxxxx> > > > --- > > > lib/header.h | 20 +++ > > > > > + > > > +static int > > > +is_cxl_cap(struct device *d, int where) > > > +{ > > > + u32 hdr; > > > + u16 w; > > > + > > > + if (!config_fetch(d, where + PCI_DVSEC_HEADER1, 8)) > > > + return 0; > > > + > > > + /* Check for supported Vendor */ > > > + hdr = get_conf_long(d, where + PCI_DVSEC_HEADER1); > > > + w = BITS(hdr, 0, 16); > > > + if (w != PCI_VENDOR_ID_INTEL) > > > > I don't think here checking is quite right. Does only Intel support CXL? > > Other Vendors should also be considered. > > In the absence of currently available hardware, I was attempting to limit > false positive noise. I’m happy to avoid the check on the vendor if there > were to exist a definitive supported list. Bjorn suggested that I check for > vendor ID with DVSEC ID for now. As hardware enters the market, I can > surely revise this with an update when the CXL group publishes a list. The Vendor ID check cannot be avoided. Vendor IDs are allocated by the PCI-SIG, but the DVSEC ID is vendor-defined so there cannot be a global "CXL capability" DVSEC ID. CXL *could* work through the PCI-SIG and define a new CXL Extended Capability that could make this generic. But apparently they've chosen to use the DVSEC mechanism instead. > > > + /* Check for Designated Vendor-Specific ID */ > > > + hdr = get_conf_long(d, where + PCI_DVSEC_HEADER2); > > > + w = BITS(hdr, 0, 16); > > > + if (w == PCI_DVSEC_ID) However, this is slightly wrong. The name "PCI_DVSEC_ID" implies that there can only be one DVSEC ID and it is vendor-independent, but neither is true. This value is allocated by Intel, so we must check for the Intel Vendor ID first. And Intel may define several DVSEC capabilities for different purposes, so the name should also mention CXL. So this should be "PCI_DVSEC_INTEL_CXL" or something similar. But that doesn't mean other vendors need to define their own DVSEC IDs for CXL. The spec (PCIe r5.0, sec 7.9.6) says: [The DVSEC Capability] allows PCI Express component vendors to use the Extended Capability mechanism to expose vendor-specific registers that can be present in components by a variety of vendors. Any vendor that implements this CXL DVSEC the same way Intel does can tag it with PCI_VENDOR_ID_INTEL and PCI_DVSEC_INTEL_CXL. Note that there are two types of vendor-specific capabilities: 1) Vendor-Specific Extended Capability (VSEC). The structure and definition are defined by the *Function* Vendor ID (from offset 0 in config space) and the VSEC ID in the capability. 2) Designated Vendor-Specific Extended Capability (DVSEC). The structure and definition are defined by the *DVSEC* Vendor ID (from the DVSEC capability, *not* the vendor of the Function) and the DVSEC ID in the capability. This CXL capability is the latter, of course. Bjorn
