linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC Patch 0/1] pciutils: Add basic decode support for CXL
@ 2020-04-08  0:09 Sean V Kelley
  2020-04-08  0:09 ` [RFC Patch 1/1] lspci: Add basic decode support for Compute eXpress Link Sean V Kelley
  0 siblings, 1 reply; 5+ messages in thread
From: Sean V Kelley @ 2020-04-08  0:09 UTC (permalink / raw)
  To: mj, bhelgaas; +Cc: linux-pci, Sean V Kelley

Compute eXpress Link[1] is a new CPU interconnected created with
workload accelerators in mind. The interconnect relies on PCIe Electrial
and Physical interconnect for communication.

Moreover, CXL bus hierarchy appear, to the OS, as an ACPI-described PCIe
Root Bridge with Integrated Endpoint.

I'm interested in feedback on how best to handle the case of DVSEC for CXL.
Up until this point it has been a catch-all:

case PCI_EXT_CAP_ID_DVSEC:
- printf("Designated Vendor-Specific <?>\n");
+ cap_cxl(d, where);
  break;

Should I retain the general printf() in the case of the absence of CXL?
Or other suggestions?

Best regards,

Sean


[1] https://www.computeexpresslink.org

Sean V Kelley (1):
  lspci: Add basic decode support for Compute eXpress Link

 lib/header.h        | 25 +++++++++++++++++++++++++
 ls-ecaps.c          | 29 ++++++++++++++++++++++++++++-
 tests/cap-cxl-dvsec |  8 ++++++++
 3 files changed, 61 insertions(+), 1 deletion(-)
 create mode 100644 tests/cap-cxl-dvsec

--
2.26.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [RFC Patch 1/1] lspci: Add basic decode support for Compute eXpress Link
  2020-04-08  0:09 [RFC Patch 0/1] pciutils: Add basic decode support for CXL Sean V Kelley
@ 2020-04-08  0:09 ` Sean V Kelley
  2020-04-08 16:56   ` Bjorn Helgaas
  0 siblings, 1 reply; 5+ messages in thread
From: Sean V Kelley @ 2020-04-08  0:09 UTC (permalink / raw)
  To: mj, bhelgaas; +Cc: linux-pci, Sean V Kelley

Compute eXpress Link[1] is a new CPU interconnected created with
workload accelerators in mind. The interconnect relies on PCIe Electrial
and Physical interconnect for communication.

Moreover, CXL bus hierarchy appear, to the OS, as an ACPI-described PCIe
Root Bridge with Integrated Endpoint.

This patch introduces basic support for lspci decode for DVSEC CXL
extended capability.

[1] https://www.computeexpresslink.org/

Signed-off-by: Sean V Kelley <sean.v.kelley@linux.intel.com>
---
 lib/header.h        | 25 +++++++++++++++++++++++++
 ls-ecaps.c          | 29 ++++++++++++++++++++++++++++-
 tests/cap-cxl-dvsec |  8 ++++++++
 3 files changed, 61 insertions(+), 1 deletion(-)
 create mode 100644 tests/cap-cxl-dvsec

diff --git a/lib/header.h b/lib/header.h
index bfdcc80..421612d 100644
--- a/lib/header.h
+++ b/lib/header.h
@@ -1042,6 +1042,27 @@
 #define PCI_EVNDR_HEADER	4	/* Vendor-Specific Header */
 #define PCI_EVNDR_REGISTERS	8	/* Vendor-Specific Registers */
 
+/* PCIe CXL Vendor-Specific Capabilities, Control, Status */
+#define PCI_EVNDR_CXL_ID	0
+
+#define PCI_CXL_CAP		0x0a
+#define  PCI_CXL_CAP_CACHE	0x0001
+#define  PCI_CXL_CAP_IO		0x0002
+#define  PCI_CXL_CAP_MEM	0x0004
+#define  PCI_CXL_CAP_MEM_HWINIT	0x0008
+#define  PCI_CXL_CAP_HDM_CNT(x)	(((x) & (3 << 4)) >> 4)
+#define  PCI_CXL_CAP_VIRAL	0x4000
+#define PCI_CXL_CTRL		0x0c
+#define  PCI_CXL_CTRL_CACHE	0x0001
+#define  PCI_CXL_CTRL_IO	0x0002
+#define  PCI_CXL_CTRL_MEM	0x0004
+#define  PCI_CXL_CTRL_CACHE_SF_COV(x)	(((x) & (0x1f << 3)) >> 3)
+#define  PCI_CXL_CTRL_CACHE_SF_GRAN(x)	(((x) & (0x7 << 8)) >> 8)
+#define  PCI_CXL_CTRL_CACHE_CLN	0x0800
+#define  PCI_CXL_CTRL_VIRAL	0x4000
+#define PCI_CXL_STATUS		0x0e
+#define  PCI_CXL_STATUS_VIRAL	0x4000
+
 /* Access Control Services */
 #define PCI_ACS_CAP		0x04	/* ACS Capability Register */
 #define PCI_ACS_CAP_VALID	0x0001	/* ACS Source Validation */
@@ -1348,6 +1369,10 @@
 #define PCI_CLASS_SIGNAL_SYNCHRONIZER	0x1110
 #define PCI_CLASS_SIGNAL_OTHER		0x1180
 
+#define PCI_CLASS_CXL			0x14
+#define PCI_CLASS_CXL_RCIEP		0x1400
+#define PCI_CLASS_CXL_OTHER		0x1480
+
 #define PCI_CLASS_OTHERS		0xff
 
 /* Several ID's we need in the library */
diff --git a/ls-ecaps.c b/ls-ecaps.c
index 0021734..8c09517 100644
--- a/ls-ecaps.c
+++ b/ls-ecaps.c
@@ -207,6 +207,33 @@ cap_aer(struct device *d, int where, int type)
     }
 }
 
+static void
+cap_cxl(struct device *d, int where)
+{
+  u16 l;
+
+  printf("PCIe DVSEC for CXL Device\n");
+  if (verbose < 2)
+    return;
+
+  if (!config_fetch(d, where + PCI_CXL_CAP, 12))
+    return;
+
+  l = get_conf_word(d, where + PCI_CXL_CAP);
+  printf("\t\tCXLCap:\tCache%c IO%c Mem%c Mem HW Init%c HDMCount %d Viral%c\n",
+    FLAG(l, PCI_CXL_CAP_CACHE), FLAG(l, PCI_CXL_CAP_IO), FLAG(l, PCI_CXL_CAP_MEM),
+    FLAG(l, PCI_CXL_CAP_MEM_HWINIT), PCI_CXL_CAP_HDM_CNT(l), FLAG(l, PCI_CXL_CAP_VIRAL));
+
+  l = get_conf_word(d, where + PCI_CXL_CTRL);
+  printf("\t\tCXLCtl:\tCache%c IO%c Mem%c Cache SF Cov %d Cache SF Gran %d Cache Clean%c Viral%c\n",
+    FLAG(l, PCI_CXL_CTRL_CACHE), FLAG(l, PCI_CXL_CTRL_IO), FLAG(l, PCI_CXL_CTRL_MEM),
+    PCI_CXL_CTRL_CACHE_SF_COV(l), PCI_CXL_CTRL_CACHE_SF_GRAN(l), FLAG(l, PCI_CXL_CTRL_CACHE_CLN),
+    FLAG(l, PCI_CXL_CTRL_VIRAL));
+
+  l = get_conf_word(d, where + PCI_CXL_STATUS);
+  printf("\t\tCXLSta:\tViral%c\n", FLAG(l, PCI_CXL_STATUS_VIRAL));
+}
+
 static void cap_dpc(struct device *d, int where)
 {
   u16 l;
@@ -924,7 +951,7 @@ show_ext_caps(struct device *d, int type)
 	    printf("Readiness Time Reporting <?>\n");
 	    break;
 	  case PCI_EXT_CAP_ID_DVSEC:
-	    printf("Designated Vendor-Specific <?>\n");
+	    cap_cxl(d, where);
 	    break;
 	  case PCI_EXT_CAP_ID_VF_REBAR:
 	    printf("VF Resizable BAR <?>\n");
diff --git a/tests/cap-cxl-dvsec b/tests/cap-cxl-dvsec
new file mode 100644
index 0000000..14e1022
--- /dev/null
+++ b/tests/cap-cxl-dvsec
@@ -0,0 +1,8 @@
+Simple diff of lspci -vvxxxx
+
+<       Capabilities: [e00 v1] Designated Vendor-Specific <?>
+---
+>       Capabilities: [e00 v1] PCIe DVSEC for CXL Device
+>               CXLCap: Cache+ IO+ Mem+ Mem HW Init+ HDMCount 1 Viral-
+>               CXLCtl: Cache- IO+ Mem- Cache SF Cov 0 Cache SF Gran 0 Cache Clean- Viral-
+>               CXLSta: Viral-
-- 
2.26.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC Patch 1/1] lspci: Add basic decode support for Compute eXpress Link
  2020-04-08  0:09 ` [RFC Patch 1/1] lspci: Add basic decode support for Compute eXpress Link Sean V Kelley
@ 2020-04-08 16:56   ` Bjorn Helgaas
  2020-04-08 21:12     ` Sean V Kelley
  0 siblings, 1 reply; 5+ messages in thread
From: Bjorn Helgaas @ 2020-04-08 16:56 UTC (permalink / raw)
  To: Sean V Kelley; +Cc: mj, linux-pci

On Tue, Apr 07, 2020 at 05:09:59PM -0700, Sean V Kelley wrote:
> Compute eXpress Link[1] is a new CPU interconnected created with
> workload accelerators in mind. The interconnect relies on PCIe Electrial
> and Physical interconnect for communication.

s/interconnected/interconnect/
s/Electrial/Electrical/

> Moreover, CXL bus hierarchy appear, to the OS, as an ACPI-described PCIe
> Root Bridge with Integrated Endpoint.

s/Moreover,/The/
s/appear,/appears/
s/the OS,/the OS/

Actually, I don't think this paragraph is really relevant.  At least
at the level of lspci, it doesn't matter whether the host bridge is
described via ACPI, DT, or something else.  And I don't think it
matters whether this is an Integrated Endpoint or otherwise.  All
lspci cares about is that we can read config space for the device.

> This patch introduces basic support for lspci decode for DVSEC CXL
> extended capability.
> 
> [1] https://www.computeexpresslink.org/
> 
> Signed-off-by: Sean V Kelley <sean.v.kelley@linux.intel.com>
> ---
>  lib/header.h        | 25 +++++++++++++++++++++++++
>  ls-ecaps.c          | 29 ++++++++++++++++++++++++++++-
>  tests/cap-cxl-dvsec |  8 ++++++++
>  3 files changed, 61 insertions(+), 1 deletion(-)
>  create mode 100644 tests/cap-cxl-dvsec
> 
> diff --git a/lib/header.h b/lib/header.h
> index bfdcc80..421612d 100644
> --- a/lib/header.h
> +++ b/lib/header.h
> @@ -1042,6 +1042,27 @@
>  #define PCI_EVNDR_HEADER	4	/* Vendor-Specific Header */
>  #define PCI_EVNDR_REGISTERS	8	/* Vendor-Specific Registers */
>  
> +/* PCIe CXL Vendor-Specific Capabilities, Control, Status */

s/Vendor-Specific/Designated Vendor-Specific/

> +#define PCI_EVNDR_CXL_ID	0

Unused in this patch.  Is this the DVSEC Vendor ID as described in
PCIe r5.0, sec 7.9.6.2?  Is 0 really the ID assigned for CXL?

> +#define PCI_CXL_CAP		0x0a
> +#define  PCI_CXL_CAP_CACHE	0x0001
> +#define  PCI_CXL_CAP_IO		0x0002
> +#define  PCI_CXL_CAP_MEM	0x0004
> +#define  PCI_CXL_CAP_MEM_HWINIT	0x0008
> +#define  PCI_CXL_CAP_HDM_CNT(x)	(((x) & (3 << 4)) >> 4)
> +#define  PCI_CXL_CAP_VIRAL	0x4000
> +#define PCI_CXL_CTRL		0x0c
> +#define  PCI_CXL_CTRL_CACHE	0x0001
> +#define  PCI_CXL_CTRL_IO	0x0002
> +#define  PCI_CXL_CTRL_MEM	0x0004
> +#define  PCI_CXL_CTRL_CACHE_SF_COV(x)	(((x) & (0x1f << 3)) >> 3)
> +#define  PCI_CXL_CTRL_CACHE_SF_GRAN(x)	(((x) & (0x7 << 8)) >> 8)
> +#define  PCI_CXL_CTRL_CACHE_CLN	0x0800
> +#define  PCI_CXL_CTRL_VIRAL	0x4000
> +#define PCI_CXL_STATUS		0x0e
> +#define  PCI_CXL_STATUS_VIRAL	0x4000
> +
>  /* Access Control Services */
>  #define PCI_ACS_CAP		0x04	/* ACS Capability Register */
>  #define PCI_ACS_CAP_VALID	0x0001	/* ACS Source Validation */
> @@ -1348,6 +1369,10 @@
>  #define PCI_CLASS_SIGNAL_SYNCHRONIZER	0x1110
>  #define PCI_CLASS_SIGNAL_OTHER		0x1180
>  
> +#define PCI_CLASS_CXL			0x14
> +#define PCI_CLASS_CXL_RCIEP		0x1400
> +#define PCI_CLASS_CXL_OTHER		0x1480
> +
>  #define PCI_CLASS_OTHERS		0xff
>  
>  /* Several ID's we need in the library */
> diff --git a/ls-ecaps.c b/ls-ecaps.c
> index 0021734..8c09517 100644
> --- a/ls-ecaps.c
> +++ b/ls-ecaps.c
> @@ -207,6 +207,33 @@ cap_aer(struct device *d, int where, int type)
>      }
>  }
>  
> +static void
> +cap_cxl(struct device *d, int where)
> +{
> +  u16 l;
> +
> +  printf("PCIe DVSEC for CXL Device\n");
> +  if (verbose < 2)
> +    return;
> +
> +  if (!config_fetch(d, where + PCI_CXL_CAP, 12))
> +    return;
> +
> +  l = get_conf_word(d, where + PCI_CXL_CAP);
> +  printf("\t\tCXLCap:\tCache%c IO%c Mem%c Mem HW Init%c HDMCount %d Viral%c\n",
> +    FLAG(l, PCI_CXL_CAP_CACHE), FLAG(l, PCI_CXL_CAP_IO), FLAG(l, PCI_CXL_CAP_MEM),
> +    FLAG(l, PCI_CXL_CAP_MEM_HWINIT), PCI_CXL_CAP_HDM_CNT(l), FLAG(l, PCI_CXL_CAP_VIRAL));
> +
> +  l = get_conf_word(d, where + PCI_CXL_CTRL);
> +  printf("\t\tCXLCtl:\tCache%c IO%c Mem%c Cache SF Cov %d Cache SF Gran %d Cache Clean%c Viral%c\n",
> +    FLAG(l, PCI_CXL_CTRL_CACHE), FLAG(l, PCI_CXL_CTRL_IO), FLAG(l, PCI_CXL_CTRL_MEM),
> +    PCI_CXL_CTRL_CACHE_SF_COV(l), PCI_CXL_CTRL_CACHE_SF_GRAN(l), FLAG(l, PCI_CXL_CTRL_CACHE_CLN),
> +    FLAG(l, PCI_CXL_CTRL_VIRAL));
> +
> +  l = get_conf_word(d, where + PCI_CXL_STATUS);
> +  printf("\t\tCXLSta:\tViral%c\n", FLAG(l, PCI_CXL_STATUS_VIRAL));
> +}
> +
>  static void cap_dpc(struct device *d, int where)
>  {
>    u16 l;
> @@ -924,7 +951,7 @@ show_ext_caps(struct device *d, int type)
>  	    printf("Readiness Time Reporting <?>\n");
>  	    break;
>  	  case PCI_EXT_CAP_ID_DVSEC:
> -	    printf("Designated Vendor-Specific <?>\n");
> +	    cap_cxl(d, where);

This assumes that *every* DVSEC capability is a CXL Designated
Vendor-Specific capability.  I think this needs to check for the
correct DVSEC Vendor ID and do cap_cxl() if it matches and the
previous behavior otherwise.

Based on the spec, I would expect to see a check for both the DVSEC
Vendor ID and the DVSEC ID before decoding the registers.

Actually, it would be nice if the generic "I don't know what this is"
code would at least print the DVSEC Vendor ID and the DVSEC ID.
Ideally this would be a separate preparatory patch.

>  	    break;
>  	  case PCI_EXT_CAP_ID_VF_REBAR:
>  	    printf("VF Resizable BAR <?>\n");
> diff --git a/tests/cap-cxl-dvsec b/tests/cap-cxl-dvsec
> new file mode 100644
> index 0000000..14e1022
> --- /dev/null
> +++ b/tests/cap-cxl-dvsec
> @@ -0,0 +1,8 @@
> +Simple diff of lspci -vvxxxx
> +
> +<       Capabilities: [e00 v1] Designated Vendor-Specific <?>
> +---
> +>       Capabilities: [e00 v1] PCIe DVSEC for CXL Device
> +>               CXLCap: Cache+ IO+ Mem+ Mem HW Init+ HDMCount 1 Viral-
> +>               CXLCtl: Cache- IO+ Mem- Cache SF Cov 0 Cache SF Gran 0 Cache Clean- Viral-
> +>               CXLSta: Viral-

I think this should be complete "lspci -vvxxxx" or "lspci -xxxx"
output.  If there's secret stuff in there you don't want to expose
yet, I don't personally object to editing the hexdump to zero it out.
But the point is that people should be able to run "lspci -F" on this
file to decode it.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC Patch 1/1] lspci: Add basic decode support for Compute eXpress Link
  2020-04-08 16:56   ` Bjorn Helgaas
@ 2020-04-08 21:12     ` Sean V Kelley
  2020-04-08 21:47       ` Bjorn Helgaas
  0 siblings, 1 reply; 5+ messages in thread
From: Sean V Kelley @ 2020-04-08 21:12 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: mj, linux-pci

Bjorn,

Thanks for taking time to review.  My comments below:

On 8 Apr 2020, at 9:56, Bjorn Helgaas wrote:

> On Tue, Apr 07, 2020 at 05:09:59PM -0700, Sean V Kelley wrote:
>> Compute eXpress Link[1] is a new CPU interconnected created with
>> workload accelerators in mind. The interconnect relies on PCIe 
>> Electrial
>> and Physical interconnect for communication.
>
> s/interconnected/interconnect/
> s/Electrial/Electrical/
>
>> Moreover, CXL bus hierarchy appear, to the OS, as an ACPI-described 
>> PCIe
>> Root Bridge with Integrated Endpoint.
>
> s/Moreover,/The/
> s/appear,/appears/
> s/the OS,/the OS/
>
> Actually, I don't think this paragraph is really relevant.  At least
> at the level of lspci, it doesn't matter whether the host bridge is
> described via ACPI, DT, or something else.  And I don't think it
> matters whether this is an Integrated Endpoint or otherwise.  All
> lspci cares about is that we can read config space for the device.

Will do.

>
>> This patch introduces basic support for lspci decode for DVSEC CXL
>> extended capability.
>>
>> [1] https://www.computeexpresslink.org/
>>
>> Signed-off-by: Sean V Kelley <sean.v.kelley@linux.intel.com>
>> ---
>>  lib/header.h        | 25 +++++++++++++++++++++++++
>>  ls-ecaps.c          | 29 ++++++++++++++++++++++++++++-
>>  tests/cap-cxl-dvsec |  8 ++++++++
>>  3 files changed, 61 insertions(+), 1 deletion(-)
>>  create mode 100644 tests/cap-cxl-dvsec
>>
>> diff --git a/lib/header.h b/lib/header.h
>> index bfdcc80..421612d 100644
>> --- a/lib/header.h
>> +++ b/lib/header.h
>> @@ -1042,6 +1042,27 @@
>>  #define PCI_EVNDR_HEADER	4	/* Vendor-Specific Header */
>>  #define PCI_EVNDR_REGISTERS	8	/* Vendor-Specific Registers */
>>
>> +/* PCIe CXL Vendor-Specific Capabilities, Control, Status */
>
> s/Vendor-Specific/Designated Vendor-Specific/
>

Will do.

>> +#define PCI_EVNDR_CXL_ID	0
>
> Unused in this patch.  Is this the DVSEC Vendor ID as described in
> PCIe r5.0, sec 7.9.6.2?  Is 0 really the ID assigned for CXL?

It is as described in the PCIe Spec.  The DVSEC ID is set to 0x0 here to 
advertise that this is a Flex Bus feature capability structure. It’s 
assigned in table 58 “PCI Express DVSEC Register Settings for Flex Bus 
Device” of the CXL 1.1 spec.

>
>> +#define PCI_CXL_CAP		0x0a
>> +#define  PCI_CXL_CAP_CACHE	0x0001
>> +#define  PCI_CXL_CAP_IO		0x0002
>> +#define  PCI_CXL_CAP_MEM	0x0004
>> +#define  PCI_CXL_CAP_MEM_HWINIT	0x0008
>> +#define  PCI_CXL_CAP_HDM_CNT(x)	(((x) & (3 << 4)) >> 4)
>> +#define  PCI_CXL_CAP_VIRAL	0x4000
>> +#define PCI_CXL_CTRL		0x0c
>> +#define  PCI_CXL_CTRL_CACHE	0x0001
>> +#define  PCI_CXL_CTRL_IO	0x0002
>> +#define  PCI_CXL_CTRL_MEM	0x0004
>> +#define  PCI_CXL_CTRL_CACHE_SF_COV(x)	(((x) & (0x1f << 3)) >> 3)
>> +#define  PCI_CXL_CTRL_CACHE_SF_GRAN(x)	(((x) & (0x7 << 8)) >> 8)
>> +#define  PCI_CXL_CTRL_CACHE_CLN	0x0800
>> +#define  PCI_CXL_CTRL_VIRAL	0x4000
>> +#define PCI_CXL_STATUS		0x0e
>> +#define  PCI_CXL_STATUS_VIRAL	0x4000
>> +
>>  /* Access Control Services */
>>  #define PCI_ACS_CAP		0x04	/* ACS Capability Register */
>>  #define PCI_ACS_CAP_VALID	0x0001	/* ACS Source Validation */
>> @@ -1348,6 +1369,10 @@
>>  #define PCI_CLASS_SIGNAL_SYNCHRONIZER	0x1110
>>  #define PCI_CLASS_SIGNAL_OTHER		0x1180
>>
>> +#define PCI_CLASS_CXL			0x14
>> +#define PCI_CLASS_CXL_RCIEP		0x1400
>> +#define PCI_CLASS_CXL_OTHER		0x1480
>> +
>>  #define PCI_CLASS_OTHERS		0xff
>>
>>  /* Several ID's we need in the library */
>> diff --git a/ls-ecaps.c b/ls-ecaps.c
>> index 0021734..8c09517 100644
>> --- a/ls-ecaps.c
>> +++ b/ls-ecaps.c
>> @@ -207,6 +207,33 @@ cap_aer(struct device *d, int where, int type)
>>      }
>>  }
>>
>> +static void
>> +cap_cxl(struct device *d, int where)
>> +{
>> +  u16 l;
>> +
>> +  printf("PCIe DVSEC for CXL Device\n");
>> +  if (verbose < 2)
>> +    return;
>> +
>> +  if (!config_fetch(d, where + PCI_CXL_CAP, 12))
>> +    return;
>> +
>> +  l = get_conf_word(d, where + PCI_CXL_CAP);
>> +  printf("\t\tCXLCap:\tCache%c IO%c Mem%c Mem HW Init%c HDMCount %d 
>> Viral%c\n",
>> +    FLAG(l, PCI_CXL_CAP_CACHE), FLAG(l, PCI_CXL_CAP_IO), FLAG(l, 
>> PCI_CXL_CAP_MEM),
>> +    FLAG(l, PCI_CXL_CAP_MEM_HWINIT), PCI_CXL_CAP_HDM_CNT(l), FLAG(l, 
>> PCI_CXL_CAP_VIRAL));
>> +
>> +  l = get_conf_word(d, where + PCI_CXL_CTRL);
>> +  printf("\t\tCXLCtl:\tCache%c IO%c Mem%c Cache SF Cov %d Cache SF 
>> Gran %d Cache Clean%c Viral%c\n",
>> +    FLAG(l, PCI_CXL_CTRL_CACHE), FLAG(l, PCI_CXL_CTRL_IO), FLAG(l, 
>> PCI_CXL_CTRL_MEM),
>> +    PCI_CXL_CTRL_CACHE_SF_COV(l), PCI_CXL_CTRL_CACHE_SF_GRAN(l), 
>> FLAG(l, PCI_CXL_CTRL_CACHE_CLN),
>> +    FLAG(l, PCI_CXL_CTRL_VIRAL));
>> +
>> +  l = get_conf_word(d, where + PCI_CXL_STATUS);
>> +  printf("\t\tCXLSta:\tViral%c\n", FLAG(l, PCI_CXL_STATUS_VIRAL));
>> +}
>> +
>>  static void cap_dpc(struct device *d, int where)
>>  {
>>    u16 l;
>> @@ -924,7 +951,7 @@ show_ext_caps(struct device *d, int type)
>>  	    printf("Readiness Time Reporting <?>\n");
>>  	    break;
>>  	  case PCI_EXT_CAP_ID_DVSEC:
>> -	    printf("Designated Vendor-Specific <?>\n");
>> +	    cap_cxl(d, where);
>
> This assumes that *every* DVSEC capability is a CXL Designated
> Vendor-Specific capability.  I think this needs to check for the
> correct DVSEC Vendor ID and do cap_cxl() if it matches and the
> previous behavior otherwise.

Yes, that’s what I’m looking for on feedback as CXL is one possible 
use of DVSEC.

>
> Based on the spec, I would expect to see a check for both the DVSEC
> Vendor ID and the DVSEC ID before decoding the registers.

Right, agreed.

>
> Actually, it would be nice if the generic "I don't know what this is"
> code would at least print the DVSEC Vendor ID and the DVSEC ID.
> Ideally this would be a separate preparatory patch.

Will do.

>
>>  	    break;
>>  	  case PCI_EXT_CAP_ID_VF_REBAR:
>>  	    printf("VF Resizable BAR <?>\n");
>> diff --git a/tests/cap-cxl-dvsec b/tests/cap-cxl-dvsec
>> new file mode 100644
>> index 0000000..14e1022
>> --- /dev/null
>> +++ b/tests/cap-cxl-dvsec
>> @@ -0,0 +1,8 @@
>> +Simple diff of lspci -vvxxxx
>> +
>> +<       Capabilities: [e00 v1] Designated Vendor-Specific <?>
>> +---
>> +>       Capabilities: [e00 v1] PCIe DVSEC for CXL Device
>> +>               CXLCap: Cache+ IO+ Mem+ Mem HW Init+ HDMCount 1 
>> Viral-
>> +>               CXLCtl: Cache- IO+ Mem- Cache SF Cov 0 Cache SF Gran 
>> 0 Cache Clean- Viral-
>> +>               CXLSta: Viral-
>
> I think this should be complete "lspci -vvxxxx" or "lspci -xxxx"
> output.  If there's secret stuff in there you don't want to expose
> yet, I don't personally object to editing the hexdump to zero it out.
> But the point is that people should be able to run "lspci -F" on this
> file to decode it.

Understood. I’m running this in simulation right now.  I will see what 
I can do.

Best regards,

Sean

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC Patch 1/1] lspci: Add basic decode support for Compute eXpress Link
  2020-04-08 21:12     ` Sean V Kelley
@ 2020-04-08 21:47       ` Bjorn Helgaas
  0 siblings, 0 replies; 5+ messages in thread
From: Bjorn Helgaas @ 2020-04-08 21:47 UTC (permalink / raw)
  To: Sean V Kelley; +Cc: mj, linux-pci

On Wed, Apr 08, 2020 at 02:12:45PM -0700, Sean V Kelley wrote:
> On 8 Apr 2020, at 9:56, Bjorn Helgaas wrote:
> > On Tue, Apr 07, 2020 at 05:09:59PM -0700, Sean V Kelley wrote:

> > > +#define PCI_EVNDR_CXL_ID	0
> > 
> > Unused in this patch.  Is this the DVSEC Vendor ID as described in
> > PCIe r5.0, sec 7.9.6.2?  Is 0 really the ID assigned for CXL?
> 
> It is as described in the PCIe Spec.  The DVSEC ID is set to 0x0 here to
> advertise that this is a Flex Bus feature capability structure. It’s
> assigned in table 58 “PCI Express DVSEC Register Settings for Flex Bus
> Device” of the CXL 1.1 spec.

It sounds like it's the "DVSEC ID" (not the "DVSEC Vendor ID") that is
zero.  That would make sense to me because the "DVSEC ID" is
completely under vendor control.

I would expect the "DVSEC Vendor ID" to be assigned by the PCI-SIG,
and I would be surprised if they assigned zero to CXL.

So I assume you'll eventually #define both IDs here and I won't be
confused any more :)

Thanks for proactively working on this support in lspci!

Bjorn

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-04-08 21:47 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-08  0:09 [RFC Patch 0/1] pciutils: Add basic decode support for CXL Sean V Kelley
2020-04-08  0:09 ` [RFC Patch 1/1] lspci: Add basic decode support for Compute eXpress Link Sean V Kelley
2020-04-08 16:56   ` Bjorn Helgaas
2020-04-08 21:12     ` Sean V Kelley
2020-04-08 21:47       ` Bjorn Helgaas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).