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

[Jay Fang <f.fangjian@huawei.com>]

Got it.
Bjorn, thanks.

Jay

On 2020/4/21 1:47, Bjorn Helgaas wrote:
> 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
> 
> .
>