Re: [PATCH v5 2/2] pciutils: Decode Compute eXpress Link DVSEC

[Bjorn Helgaas <helgaas@kernel.org>]

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@linux.intel.com>
> > > ---
> > >  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