[PATCH v4 0/3] cxl: Export cxl1.1 device link status to sysfs

[PATCH v4 0/3] cxl: Export cxl1.1 device link status to sysfs

[Kobayashi,Daisuke]

Export cxl1.1 device link status register value to pci device sysfs.

CXL devices are extensions of PCIe. Therefore, from CXL2.0 onwards,
the link status can be output in the same way as traditional PCIe.
However, unlike devices from CXL2.0 onwards, CXL1.1 requires a
different method to obtain the link status from traditional PCIe.
This is because the link status of the CXL1.1 device is not mapped
in the configuration space (as per cxl3.0 specification 8.1).
Instead, the configuration space containing the link status is mapped
to the memory mapped register region (as per cxl3.0 specification 8.2,
Table 8-18). Therefore, the current lspci has a problem where it does
not display the link status of the CXL1.1 device. 
Solve these issues with sysfs attributes to export the status
registers hidden in the RCRB.

The procedure is as follows:
First, obtain the RCRB address within the cxl driver, then access 
the configuration space. Next, output the link status information from
the configuration space to sysfs. Ultimately, the expectation is that this
sysfs file will be consumed by PCI user tools to utilize link status information.


Changes
v1[1] -> v2:
The following are the main changes made based on the feedback from Dan Williams.
- Modified to perform rcrb access within the CXL driver.
- Added new attributes to the sysfs of the PCI device.
- Output the link status information to the sysfs of the PCI device.
- Retrieve information from sysfs as the source when displaying information in lspci.

v2[2] -> v3:
- Fix unnecessary initialization and wrong types (Bjohn).
- Create a helper function for getting a PCIe capability offset (Bjohn).
- Move platform-specific implementation to the lib directory in pciutils (Martin).

v3[3] -> v4:
- RCRB register values are read once and cached.
- Added a new attribute to the sysfs of the PCI device.
- Separate lspci implementation from this patch.
[1]
https://lore.kernel.org/linux-cxl/20231220050738.178481-1-kobayashi.da-06@fujitsu.com/
[2]
https://lore.kernel.org/linux-cxl/20240227083313.87699-1-kobayashi.da-06@fujitsu.com/
[3]
https://lore.kernel.org/linux-cxl/20240312080559.14904-1-kobayashi.da-06@fujitsu.com/

Kobayashi,Daisuke (3):
  Add rcd_regs to cxl_rcrb_info
  Add rcd_regs initialization at __rcrb_to_component()
  Add sysfs attribute for CXL 1.1 device link status

 drivers/cxl/core/regs.c | 18 ++++++++++
 drivers/cxl/cxl.h       |  3 ++
 drivers/cxl/pci.c       | 74 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 95 insertions(+)

-- 
2.44.0

[PATCH v4 1/3] cxl: Add rcd_regs to cxl_rcrb_info

[Kobayashi,Daisuke]

Add rcd regs to cxl_rcrb_info to cache the RCD register values.

Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
---
 drivers/cxl/cxl.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 003feebab79b..2dc827c301a1 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -647,6 +647,9 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
 struct cxl_rcrb_info {
 	resource_size_t base;
 	u16 aer_cap;
+	u16 rcd_lnkctrl;
+	u16 rcd_lnkstatus;
+	u32 rcd_lnkcap;
 };
 
 /**
-- 
2.44.0

[PATCH v4 2/3] cxl/core/regs: Add rcd_regs initialization at __rcrb_to_component()

[Kobayashi,Daisuke]

Add rcd_regs initialization at __rcrb_to_component() to cache the cxl1.1
device link status information. Reduce access to the memory map area
where the RCRB is located by caching the cxl1.1 device link status information.

Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
---
 drivers/cxl/core/regs.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index 372786f80955..308eb951613e 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -514,6 +514,8 @@ resource_size_t __rcrb_to_component(struct device *dev, struct cxl_rcrb_info *ri
 	u32 bar0, bar1;
 	u16 cmd;
 	u32 id;
+	u16 offset;
+	u32 cap_hdr;
 
 	if (which == CXL_RCRB_UPSTREAM)
 		rcrb += SZ_4K;
@@ -537,6 +539,22 @@ resource_size_t __rcrb_to_component(struct device *dev, struct cxl_rcrb_info *ri
 	cmd = readw(addr + PCI_COMMAND);
 	bar0 = readl(addr + PCI_BASE_ADDRESS_0);
 	bar1 = readl(addr + PCI_BASE_ADDRESS_1);
+
+	offset = readw(addr + PCI_CAPABILITY_LIST);
+	offset &= 0x00ff;
+	cap_hdr = readl(addr + offset);
+	while ((cap_hdr & 0x000000ff) != PCI_CAP_ID_EXP) {
+		offset = (cap_hdr >> 8) & 0x000000ff;
+		if (offset == 0) // End of capability list
+			break;
+		cap_hdr = readl(addr + offset);
+	}
+	if (offset) {
+		ri->rcd_lnkcap = readl(addr + offset + PCI_EXP_LNKCAP);
+		ri->rcd_lnkctrl = readl(addr + offset + PCI_EXP_LNKCTL);
+		ri->rcd_lnkstatus = readl(addr + offset + PCI_EXP_LNKSTA);
+	}
+
 	iounmap(addr);
 	release_mem_region(rcrb, SZ_4K);
 
-- 
2.44.0

[PATCH v4 3/3] cxl/pci: Add sysfs attribute for CXL 1.1 device link status

[Kobayashi,Daisuke]

Add sysfs attribute for CXL 1.1 device link status to the cxl pci device.

In CXL1.1, the link status of the device is included in the RCRB mapped to
the memory mapped register area. Critically, that arrangement makes the link
status and control registers invisible to existing PCI user tooling.

Export those registers via sysfs with the expectation that PCI user
tooling will alternatively look for these sysfs files when attempting to
access to these CXL 1.1 endpoints registers.

Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
---
 drivers/cxl/pci.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 2ff361e756d6..0ff15738b1ba 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -786,6 +786,79 @@ static int cxl_event_config(struct pci_host_bridge *host_bridge,
 	return 0;
 }
 
+static ssize_t rcd_link_cap_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct cxl_port *port;
+	struct cxl_dport *dport;
+	struct device *parent = dev->parent;
+	struct pci_dev *parent_pdev = to_pci_dev(parent);
+
+	port = cxl_pci_find_port(parent_pdev, &dport);
+	if (!port)
+		return -EINVAL;
+
+	return sysfs_emit(buf, "%x\n", dport->rcrb.rcd_lnkcap);
+}
+static DEVICE_ATTR_RO(rcd_link_cap);
+
+static ssize_t rcd_link_ctrl_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct cxl_port *port;
+	struct cxl_dport *dport;
+	struct device *parent = dev->parent;
+	struct pci_dev *parent_pdev = to_pci_dev(parent);
+
+	port = cxl_pci_find_port(parent_pdev, &dport);
+	if (!port)
+		return -EINVAL;
+
+	return sysfs_emit(buf, "%x\n", dport->rcrb.rcd_lnkctrl);
+}
+static DEVICE_ATTR_RO(rcd_link_ctrl);
+
+static ssize_t rcd_link_status_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct cxl_port *port;
+	struct cxl_dport *dport;
+	struct device *parent = dev->parent;
+	struct pci_dev *parent_pdev = to_pci_dev(parent);
+
+	port = cxl_pci_find_port(parent_pdev, &dport);
+	if (!port)
+		return -EINVAL;
+
+	return sysfs_emit(buf, "%x\n", dport->rcrb.rcd_lnkstatus);
+}
+static DEVICE_ATTR_RO(rcd_link_status);
+
+static struct attribute *cxl_rcd_attrs[] = {
+		&dev_attr_rcd_link_cap.attr,
+		&dev_attr_rcd_link_ctrl.attr,
+		&dev_attr_rcd_link_status.attr,
+		NULL
+};
+
+static umode_t cxl_rcd_visible(struct kobject *kobj,
+					  struct attribute *a, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (is_cxl_restricted(pdev))
+		return a->mode;
+
+	return 0;
+}
+
+static struct attribute_group cxl_rcd_group = {
+		.attrs = cxl_rcd_attrs,
+		.is_visible = cxl_rcd_visible,
+};
+__ATTRIBUTE_GROUPS(cxl_rcd);
+
 static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus);
@@ -969,6 +1042,7 @@ static struct pci_driver cxl_pci_driver = {
 	.id_table		= cxl_mem_pci_tbl,
 	.probe			= cxl_pci_probe,
 	.err_handler		= &cxl_error_handlers,
+	.dev_groups		= cxl_rcd_groups,
 	.driver	= {
 		.probe_type	= PROBE_PREFER_ASYNCHRONOUS,
 	},
-- 
2.44.0

Re: [PATCH v4 3/3] cxl/pci: Add sysfs attribute for CXL 1.1 device link status

[Bjorn Helgaas]

On Tue, Apr 09, 2024 at 04:35:28PM +0900, Kobayashi,Daisuke wrote:
> Add sysfs attribute for CXL 1.1 device link status to the cxl pci device.
> 
> In CXL1.1, the link status of the device is included in the RCRB mapped to
> the memory mapped register area. Critically, that arrangement makes the link
> status and control registers invisible to existing PCI user tooling.

Idle thought: PCIe does define RCRB, even pre-CXL.  Maybe the PCI core
should be enhanced to comprehend RCRB directly?

> +static ssize_t rcd_link_status_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct cxl_port *port;
> +	struct cxl_dport *dport;
> +	struct device *parent = dev->parent;
> +	struct pci_dev *parent_pdev = to_pci_dev(parent);
> +
> +	port = cxl_pci_find_port(parent_pdev, &dport);
> +	if (!port)
> +		return -EINVAL;
> +
> +	return sysfs_emit(buf, "%x\n", dport->rcrb.rcd_lnkstatus);

Is it really what you want to capture PCI_EXP_LNKSTA once at
enumeration-time and expose that static value forever?  I assume
status bits can change over time, so I would naively expect that you
want the *current* value, not just a value from the distant past.

Bjorn

Re: [PATCH v4 3/3] cxl/pci: Add sysfs attribute for CXL 1.1 device link status

[Dan Williams]

Bjorn Helgaas wrote:
> On Tue, Apr 09, 2024 at 04:35:28PM +0900, Kobayashi,Daisuke wrote:
> > Add sysfs attribute for CXL 1.1 device link status to the cxl pci device.
> > 
> > In CXL1.1, the link status of the device is included in the RCRB mapped to
> > the memory mapped register area. Critically, that arrangement makes the link
> > status and control registers invisible to existing PCI user tooling.
> 
> Idle thought: PCIe does define RCRB, even pre-CXL.  Maybe the PCI core
> should be enhanced to comprehend RCRB directly?

It depends on if this slow drip of features continues, and it seems that
PCIe base RCRB is scoped to a single device/port whereas CXL appears to
extend it to merge the endpoint config space and root-port config space
into a double-sized RCRB area.

I.e. there will continue to be CXL specifics involved.

Also, this is a one-generation-quirk as CXL 2.0+ hosts drop this awkward
RCRB arrangement.

> > +static ssize_t rcd_link_status_show(struct device *dev,
> > +				   struct device_attribute *attr, char *buf)
> > +{
> > +	struct cxl_port *port;
> > +	struct cxl_dport *dport;
> > +	struct device *parent = dev->parent;
> > +	struct pci_dev *parent_pdev = to_pci_dev(parent);
> > +
> > +	port = cxl_pci_find_port(parent_pdev, &dport);
> > +	if (!port)
> > +		return -EINVAL;
> > +
> > +	return sysfs_emit(buf, "%x\n", dport->rcrb.rcd_lnkstatus);
> 
> Is it really what you want to capture PCI_EXP_LNKSTA once at
> enumeration-time and expose that static value forever?  I assume
> status bits can change over time, so I would naively expect that you
> want the *current* value, not just a value from the distant past.

I expect this should copy what is done for aer_cap where that single
RCRB capability block is cached for future access. That said many of the
link status change events would also cause the device to be rescanned
and that value is refreshed once per driver bind event.

Re: [PATCH v4 1/3] cxl: Add rcd_regs to cxl_rcrb_info

[Dave Jiang]

On 4/9/24 12:35 AM, Kobayashi,Daisuke wrote:
> Add rcd regs to cxl_rcrb_info to cache the RCD register values.

I suggest you squash this patch with 2/3. There's not much meaning of adding variables without showing usage.

> 
> Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
> ---
>  drivers/cxl/cxl.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 003feebab79b..2dc827c301a1 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -647,6 +647,9 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
>  struct cxl_rcrb_info {
>  	resource_size_t base;
>  	u16 aer_cap;
> +	u16 rcd_lnkctrl;
> +	u16 rcd_lnkstatus;
> +	u32 rcd_lnkcap;
>  };
>  
>  /**

Re: [PATCH v4 2/3] cxl/core/regs: Add rcd_regs initialization at __rcrb_to_component()

[Dave Jiang]

On 4/9/24 12:35 AM, Kobayashi,Daisuke wrote:
> Add rcd_regs initialization at __rcrb_to_component() to cache the cxl1.1
> device link status information. Reduce access to the memory map area
> where the RCRB is located by caching the cxl1.1 device link status information.
> 
> Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
> ---
>  drivers/cxl/core/regs.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index 372786f80955..308eb951613e 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -514,6 +514,8 @@ resource_size_t __rcrb_to_component(struct device *dev, struct cxl_rcrb_info *ri
>  	u32 bar0, bar1;
>  	u16 cmd;
>  	u32 id;
> +	u16 offset;
> +	u32 cap_hdr;
>  
>  	if (which == CXL_RCRB_UPSTREAM)
>  		rcrb += SZ_4K;
> @@ -537,6 +539,22 @@ resource_size_t __rcrb_to_component(struct device *dev, struct cxl_rcrb_info *ri
>  	cmd = readw(addr + PCI_COMMAND);
>  	bar0 = readl(addr + PCI_BASE_ADDRESS_0);
>  	bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> +
> +	offset = readw(addr + PCI_CAPABILITY_LIST);
> +	offset &= 0x00ff;

GENMASK(7,0) is preferred to 0x00ff. Although a properly defined mask would be nice.
Also please consider using FIELD_GET().

> +	cap_hdr = readl(addr + offset);
> +	while ((cap_hdr & 0x000000ff) != PCI_CAP_ID_EXP) {

Same comment as above


> +		offset = (cap_hdr >> 8) & 0x000000ff;

Also here

> +		if (offset == 0) // End of capability list

Please use /* */ instead of // for Linux kernel code

> +			break;
> +		cap_hdr = readl(addr + offset);
> +	}
> +	if (offset) {
> +		ri->rcd_lnkcap = readl(addr + offset + PCI_EXP_LNKCAP);
> +		ri->rcd_lnkctrl = readl(addr + offset + PCI_EXP_LNKCTL);
> +		ri->rcd_lnkstatus = readl(addr + offset + PCI_EXP_LNKSTA);
> +	}
> +
>  	iounmap(addr);
>  	release_mem_region(rcrb, SZ_4K);
>  

RE: [PATCH v4 3/3] cxl/pci: Add sysfs attribute for CXL 1.1 device link status

[Dan Williams]

Kobayashi,Daisuke wrote:
> Add sysfs attribute for CXL 1.1 device link status to the cxl pci device.
> 
> In CXL1.1, the link status of the device is included in the RCRB mapped to
> the memory mapped register area. Critically, that arrangement makes the link
> status and control registers invisible to existing PCI user tooling.
> 
> Export those registers via sysfs with the expectation that PCI user
> tooling will alternatively look for these sysfs files when attempting to
> access to these CXL 1.1 endpoints registers.
> 
> Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
> ---
>  drivers/cxl/pci.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 2ff361e756d6..0ff15738b1ba 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -786,6 +786,79 @@ static int cxl_event_config(struct pci_host_bridge *host_bridge,
>  	return 0;
>  }
>  
> +static ssize_t rcd_link_cap_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct cxl_port *port;
> +	struct cxl_dport *dport;
> +	struct device *parent = dev->parent;
> +	struct pci_dev *parent_pdev = to_pci_dev(parent);
> +
> +	port = cxl_pci_find_port(parent_pdev, &dport);
> +	if (!port)
> +		return -EINVAL;

A few problems with this:

1/ No need to convert to the parent PCI device when there is a lookup
routine to go from cxl_memdev to its upstream port.

        struct cxl_dev_state *cxlds = dev_get_drvdata(dev);
        struct cxl_memdev *cxlmd = cxlds->cxlmd;

2/ The port reference is leaked Add a put_cxl_port() __free() routine
like this:

	diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
	index 534e25e2f0a4..d81bc4cc0a4c 100644
	--- a/drivers/cxl/cxl.h
	+++ b/drivers/cxl/cxl.h
	@@ -744,6 +744,7 @@ DEFINE_FREE(put_cxl_root, struct cxl_root *, if (_T) put_cxl_root(_T))
	 int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd);
	 void cxl_bus_rescan(void);
	 void cxl_bus_drain(void);
	+DEFINE_FREE(put_cxl_port, struct cxl_port *, if (_T) put_cxl_port(_T))
	 struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev,
	                                   struct cxl_dport **dport);
	 struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
	
...and then:

	struct cxl_port *port __free(put_cxl_port) = cxl_mem_find_port(cxlmd, &dport);

3/ The port corresponding to a memdev can disappear at any time so you
need to do the same validation the cxl_mem_probe() does to keep the port
active during the register access:

	guard(device)(&port->dev);
	if (!port->dev.driver)
		return -ENXIO;

...then you can read from the cached PCIe capability similar to how the
error handler path reads from aer_cap.

RE: [PATCH v4 3/3] cxl/pci: Add sysfs attribute for CXL 1.1 device link status

[Dan Williams]

Dan Williams wrote:
> Kobayashi,Daisuke wrote:
> > Add sysfs attribute for CXL 1.1 device link status to the cxl pci device.
> > 
> > In CXL1.1, the link status of the device is included in the RCRB mapped to
> > the memory mapped register area. Critically, that arrangement makes the link
> > status and control registers invisible to existing PCI user tooling.
> > 
> > Export those registers via sysfs with the expectation that PCI user
> > tooling will alternatively look for these sysfs files when attempting to
> > access to these CXL 1.1 endpoints registers.
> > 
> > Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
> > ---
> >  drivers/cxl/pci.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 74 insertions(+)
> > 
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index 2ff361e756d6..0ff15738b1ba 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
[..]
> 3/ The port corresponding to a memdev can disappear at any time so you
> need to do the same validation the cxl_mem_probe() does to keep the port
> active during the register access:
> 
> 	guard(device)(&port->dev);
> 	if (!port->dev.driver)
> 		return -ENXIO;

Apologies, I made a mistake here. Copy how cxl_mem_probe() accesses the
dport.

	endpoint_parent = port->uport_dev;
	guard(device)(&endpoint_parent->dev);
	if (!endpoint_parent->driver)
		return -ENXIO;

Re: [PATCH v4 3/3] cxl/pci: Add sysfs attribute for CXL 1.1 device link status

[Lukas Wunner]

On Tue, Apr 09, 2024 at 10:05:40AM -0500, Bjorn Helgaas wrote:
> On Tue, Apr 09, 2024 at 04:35:28PM +0900, Kobayashi,Daisuke wrote:
> > Add sysfs attribute for CXL 1.1 device link status to the cxl pci device.
> > 
> > In CXL1.1, the link status of the device is included in the RCRB mapped to
> > the memory mapped register area. Critically, that arrangement makes the link
> > status and control registers invisible to existing PCI user tooling.
> 
> Idle thought: PCIe does define RCRB, even pre-CXL.  Maybe the PCI core
> should be enhanced to comprehend RCRB directly?

The way CXL 1.1 (ab)uses the RCRB differs from what the PCIe Base Spec
envisions:

Per PCIe r6.2 sec 7.2.3, the RCRB contains additional Extended Capabilities
of a Root Port -- in addition to those in the Root Port's Config Space.
What we could do in the PCI core to support this is to amend our helpers
which search for Extended Capabilities to also search for them in the RCRB.

In fact, two years ago I cooked up a patch which does exactly that:

https://github.com/l1k/linux/commit/3eb94f042527

And I cooked up another patch to fetch the RCRB's address from the CXL
Early Discovery ACPI table:

https://github.com/l1k/linux/commit/d9d3cf45cf8c

The reason I never submitted the patches?  I realized after the fact that
CXL 1.1 uses the RCRB in a completely different way:

Per CXL r3.0 sec 8.2.1, RCH Downstream and RCD Upstream Ports do not
actually possess a Config Space.  Instead, they possess *only* an RCRB.
And that RCRB contains a Type 1 Configuration Space Header.

But because the PCIe Base Spec prescribes that there has to be an
Extended Capability at offset 0 of the RCRB, the CXL spec puts a
Null Extended Capability at offset 0 so that the Type 1 Config Space
Header is skipped.

However this means that the first dword of the Type 1 Config Space
Header does not contain a Vendor ID and Device ID.

So what we could do is create a fake pci_dev for each RCH Downstream and
RCD Upstream Port plus a specially crafted struct pci_ops whose ->read()
and write() callbacks access the RCRB.  But how do we know which Vendor
and Device ID to return from a ->read()?  There is none in the RCRB!

The CXL Consortium seems to have realized the mess they made with
CXL 1.1 and from CXL 2.0 onwards everything is now a proper PCI device.
I talked to Dan about my findings and his decision was basically to
not enable any of that legacy CXL 1.1 RCRB functionality in the kernel.

Thanks,

Lukas

RE: [PATCH v4 2/3] cxl/core/regs: Add rcd_regs initialization at __rcrb_to_component()

[Daisuke Kobayashi (Fujitsu)]

Dave Jiang wrote:
> On 4/9/24 12:35 AM, Kobayashi,Daisuke wrote:
> > Add rcd_regs initialization at __rcrb_to_component() to cache the
> > cxl1.1 device link status information. Reduce access to the memory map
> > area where the RCRB is located by caching the cxl1.1 device link status
> information.
> >
> > Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
> > ---
> >  drivers/cxl/core/regs.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c index
> > 372786f80955..308eb951613e 100644
> > --- a/drivers/cxl/core/regs.c
> > +++ b/drivers/cxl/core/regs.c
> > @@ -514,6 +514,8 @@ resource_size_t __rcrb_to_component(struct device
> *dev, struct cxl_rcrb_info *ri
> >  	u32 bar0, bar1;
> >  	u16 cmd;
> >  	u32 id;
> > +	u16 offset;
> > +	u32 cap_hdr;
> >
> >  	if (which == CXL_RCRB_UPSTREAM)
> >  		rcrb += SZ_4K;
> > @@ -537,6 +539,22 @@ resource_size_t __rcrb_to_component(struct device
> *dev, struct cxl_rcrb_info *ri
> >  	cmd = readw(addr + PCI_COMMAND);
> >  	bar0 = readl(addr + PCI_BASE_ADDRESS_0);
> >  	bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> > +
> > +	offset = readw(addr + PCI_CAPABILITY_LIST);
> > +	offset &= 0x00ff;
> 
> GENMASK(7,0) is preferred to 0x00ff. Although a properly defined mask would
> be nice.
> Also please consider using FIELD_GET().
> 
Thank you for your feedback.
I will update the patch to use those macros.

> > +	cap_hdr = readl(addr + offset);
> > +	while ((cap_hdr & 0x000000ff) != PCI_CAP_ID_EXP) {
> 
> Same comment as above
> 
> 
> > +		offset = (cap_hdr >> 8) & 0x000000ff;
> 
> Also here
> 
> > +		if (offset == 0) // End of capability list
> 
> Please use /* */ instead of // for Linux kernel code
> 
Also I will fix it.

> > +			break;
> > +		cap_hdr = readl(addr + offset);
> > +	}
> > +	if (offset) {
> > +		ri->rcd_lnkcap = readl(addr + offset + PCI_EXP_LNKCAP);
> > +		ri->rcd_lnkctrl = readl(addr + offset + PCI_EXP_LNKCTL);
> > +		ri->rcd_lnkstatus = readl(addr + offset + PCI_EXP_LNKSTA);
> > +	}
> > +
> >  	iounmap(addr);
> >  	release_mem_region(rcrb, SZ_4K);
> >

RE: [PATCH v4 3/3] cxl/pci: Add sysfs attribute for CXL 1.1 device link status

[Daisuke Kobayashi (Fujitsu)]

> Dan Williams wrote:
> > Kobayashi,Daisuke wrote:
> > > Add sysfs attribute for CXL 1.1 device link status to the cxl pci device.
> > >
> > > In CXL1.1, the link status of the device is included in the RCRB mapped to
> > > the memory mapped register area. Critically, that arrangement makes the
> link
> > > status and control registers invisible to existing PCI user tooling.
> > >
> > > Export those registers via sysfs with the expectation that PCI user
> > > tooling will alternatively look for these sysfs files when attempting to
> > > access to these CXL 1.1 endpoints registers.
> > >
> > > Signed-off-by: "Kobayashi,Daisuke" <kobayashi.da-06@fujitsu.com>
> > > ---
> > >  drivers/cxl/pci.c | 74
> +++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 74 insertions(+)
> > >
> > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > > index 2ff361e756d6..0ff15738b1ba 100644
> > > --- a/drivers/cxl/pci.c
> > > +++ b/drivers/cxl/pci.c
> [..]
> > 3/ The port corresponding to a memdev can disappear at any time so you
> > need to do the same validation the cxl_mem_probe() does to keep the port
> > active during the register access:
> >
> > 	guard(device)(&port->dev);
> > 	if (!port->dev.driver)
> > 		return -ENXIO;
> 
> Apologies, I made a mistake here. Copy how cxl_mem_probe() accesses the
> dport.
> 
> 	endpoint_parent = port->uport_dev;
> 	guard(device)(&endpoint_parent->dev);
> 	if (!endpoint_parent->driver)
> 		return -ENXIO;

Thank you for your feedback.
I could not find the exact same code as the suggestion from cxl_mem_probe(), 
but would your suggestion be correct with the following modification:

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 2ff361e756d6..0ff15738b1ba 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -786,6 +786,79 @@ static int cxl_event_config(struct pci_host_bridge *host_bridge,
 	return 0;
 }
 
+static ssize_t rcd_link_cap_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct cxl_port *port;
+	struct cxl_dport *dport;
+	struct cxl_dev_state *cxlds = dev_get_drvdata(dev);
+	struct cxl_memdev *cxlmd = cxlds->cxlmd;
+	struct device *endpoint_parent;
+
+	port = cxl_mem_find_port(cxlmd, &dport);
+	if (!port)
+		return -EINVAL;
+
+	endpoint_parent = port->uport_dev;
+	guard(device)(&endpoint_parent);
+	if (!endpoint_parent->driver)
+		return -ENXIO;
+	return sysfs_emit(buf, "%x\n", dport->rcrb.rcd_lnkcap);
+}
+static DEVICE_ATTR_RO(rcd_link_cap);
[..]
--

From reading the guard macro, my understanding is that this is a macro which
calls the constructor here, and calls the destructor when the scope is exited. 
Will this prevent the port from disappearing?