[PATCH v2 0/2] CXL ACPI tables for object creation

[PATCH v2 0/2] CXL ACPI tables for object creation

[Alison Schofield]

Changes since v1 [1]:
- open code the cfmws restrictions to decode flags work (Dan)
- add range info on error messages when adding a decoder fails (Dan)
- make find_dport_by_dev() static (Dan)
- add linux-acpi to cc list (Dan)

[1] https://lore.kernel.org/linux-cxl/cover.1623705308.git.alison.schofield@intel.com/


Parse the ACPI CXL Early Discovery Table (CEDT) and use the CHBS & CFMWS
when creating port and decoder objects.

CHBS: CXL Host Bridge Structure - Patch 1
CFMWS: CXL Fixed Memory Window Structure - Patch 2

Alison Schofield (2):
  cxl/acpi: Add the Host Bridge base address to CXL port objects
  cxl/acpi: Use the ACPI CFMWS to create static decoder objects

 drivers/cxl/acpi.c | 219 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 213 insertions(+), 6 deletions(-)


base-commit: 195d5a63f0f9a47aa128a5050fe4ad7f5d27a901
-- 
2.26.2

[PATCH v2 1/2] cxl/acpi: Add the Host Bridge base address to CXL port objects

[Alison Schofield]

The base address for the Host Bridge port component registers is located
in the CXL Host Bridge Structure (CHBS) of the ACPI CXL Early Discovery
Table (CEDT). Retrieve the CHBS for each Host Bridge (ACPI0016 device)
and include that base address in the port object.

Co-developed-by: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/acpi.c | 105 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 99 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index be357eea552c..b6d9cd45428c 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -8,6 +8,61 @@
 #include <linux/pci.h>
 #include "cxl.h"
 
+static struct acpi_table_header *cedt_table;
+
+static struct acpi_cedt_chbs *cxl_acpi_match_chbs(struct device *dev, u32 uid)
+{
+	struct acpi_cedt_chbs *chbs, *chbs_match = NULL;
+	acpi_size len, cur = 0;
+	void *cedt_base;
+	int rc = 0;
+
+	len = cedt_table->length - sizeof(*cedt_table);
+	cedt_base = cedt_table + 1;
+
+	while (cur < len) {
+		struct acpi_cedt_header *c = cedt_base + cur;
+
+		if (c->type != ACPI_CEDT_TYPE_CHBS) {
+			cur += c->length;
+			continue;
+		}
+
+		chbs = cedt_base + cur;
+
+		if (chbs->header.length < sizeof(*chbs)) {
+			dev_err(dev, "Invalid CHBS header length: %u\n",
+				chbs->header.length);
+			rc = -EINVAL;
+			break;
+		}
+
+		if (chbs->uid == uid && !chbs_match) {
+			chbs_match = chbs;
+			cur += c->length;
+			continue;
+		}
+
+		if (chbs->uid == uid && chbs_match) {
+			dev_err(dev, "Duplicate CHBS UIDs %u\n", uid);
+			rc = -EINVAL;
+			break;
+		}
+		cur += c->length;
+	}
+	if (!chbs_match)
+		rc = -EINVAL;
+	if (rc)
+		return ERR_PTR(rc);
+
+	return chbs_match;
+}
+
+static resource_size_t get_chbcr(struct acpi_cedt_chbs *chbs)
+{
+	return IS_ERR(chbs) ? CXL_RESOURCE_NONE : chbs->base;
+}
+
 struct cxl_walk_context {
 	struct device *dev;
 	struct pci_bus *root;
@@ -50,6 +105,21 @@ static int match_add_root_ports(struct pci_dev *pdev, void *data)
 	return 0;
 }
 
+static struct cxl_dport *find_dport_by_dev(struct cxl_port *port, struct device *dev)
+{
+	struct cxl_dport *dport;
+
+	device_lock(&port->dev);
+	list_for_each_entry(dport, &port->dports, list)
+		if (dport->dport == dev) {
+			device_unlock(&port->dev);
+			return dport;
+		}
+
+	device_unlock(&port->dev);
+	return NULL;
+}
+
 static struct acpi_device *to_cxl_host_bridge(struct device *dev)
 {
 	struct acpi_device *adev = to_acpi_device(dev);
@@ -71,6 +141,7 @@ static int add_host_bridge_uport(struct device *match, void *arg)
 	struct acpi_pci_root *pci_root;
 	struct cxl_walk_context ctx;
 	struct cxl_decoder *cxld;
+	struct cxl_dport *dport;
 	struct cxl_port *port;
 
 	if (!bridge)
@@ -80,8 +151,15 @@ static int add_host_bridge_uport(struct device *match, void *arg)
 	if (!pci_root)
 		return -ENXIO;
 
-	/* TODO: fold in CEDT.CHBS retrieval */
-	port = devm_cxl_add_port(host, match, CXL_RESOURCE_NONE, root_port);
+	dport = find_dport_by_dev(root_port, match);
+	if (!dport) {
+		dev_dbg(host, "host bridge expected and not found\n");
+		return -ENODEV;
+	}
+
+	port = devm_cxl_add_port(host, match, dport->component_reg_phys,
+				 root_port);
+
 	if (IS_ERR(port))
 		return PTR_ERR(port);
 	dev_dbg(host, "%s: add: %s\n", dev_name(match), dev_name(&port->dev));
@@ -120,6 +198,7 @@ static int add_host_bridge_dport(struct device *match, void *arg)
 	int rc;
 	acpi_status status;
 	unsigned long long uid;
+	struct acpi_cedt_chbs *chbs;
 	struct cxl_port *root_port = arg;
 	struct device *host = root_port->dev.parent;
 	struct acpi_device *bridge = to_cxl_host_bridge(match);
@@ -135,7 +214,12 @@ static int add_host_bridge_dport(struct device *match, void *arg)
 		return -ENODEV;
 	}
 
-	rc = cxl_add_dport(root_port, match, uid, CXL_RESOURCE_NONE);
+	chbs = cxl_acpi_match_chbs(host, uid);
+	if (IS_ERR(chbs))
+		dev_dbg(host, "No CHBS found for Host Bridge: %s\n",
+			dev_name(match));
+
+	rc = cxl_add_dport(root_port, match, uid, get_chbcr(chbs));
 	if (rc) {
 		dev_err(host, "failed to add downstream port: %s\n",
 			dev_name(match));
@@ -148,6 +232,7 @@ static int add_host_bridge_dport(struct device *match, void *arg)
 static int cxl_acpi_probe(struct platform_device *pdev)
 {
 	int rc;
+	acpi_status status;
 	struct cxl_port *root_port;
 	struct device *host = &pdev->dev;
 	struct acpi_device *adev = ACPI_COMPANION(host);
@@ -157,17 +242,25 @@ static int cxl_acpi_probe(struct platform_device *pdev)
 		return PTR_ERR(root_port);
 	dev_dbg(host, "add: %s\n", dev_name(&root_port->dev));
 
+	status = acpi_get_table(ACPI_SIG_CEDT, 0, &cedt_table);
+	if (ACPI_FAILURE(status))
+		return -ENXIO;
+
 	rc = bus_for_each_dev(adev->dev.bus, NULL, root_port,
 			      add_host_bridge_dport);
 	if (rc)
-		return rc;
+		goto out;
 
 	/*
 	 * Root level scanned with host-bridge as dports, now scan host-bridges
 	 * for their role as CXL uports to their CXL-capable PCIe Root Ports.
 	 */
-	return bus_for_each_dev(adev->dev.bus, NULL, root_port,
-				add_host_bridge_uport);
+	rc = bus_for_each_dev(adev->dev.bus, NULL, root_port,
+			      add_host_bridge_uport);
+
+out:
+	acpi_put_table(cedt_table);
+	return rc;
 }
 
 static const struct acpi_device_id cxl_acpi_ids[] = {
-- 
2.26.2

[PATCH v2 2/2] cxl/acpi: Use the ACPI CFMWS to create static decoder objects

[Alison Schofield]

The ACPI CXL Early Discovery Table (CEDT) includes a list of CXL memory
resources in CXL Fixed Memory Window Structures (CFMWS). Retrieve each
CFMWS in the CEDT and add a cxl_decoder object to the root port (root0)
for each memory resource.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/acpi.c | 114 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 114 insertions(+)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index b6d9cd45428c..e3aa356d4dcd 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -8,8 +8,120 @@
 #include <linux/pci.h>
 #include "cxl.h"
 
+/* Encode defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register */
+#define CFMWS_INTERLEAVE_WAYS(x)	(1 << (x)->interleave_ways)
+#define CFMWS_INTERLEAVE_GRANULARITY(x)	((x)->granularity + 8)
+
 static struct acpi_table_header *cedt_table;
 
+static unsigned long cfmws_to_decoder_flags(int restrictions)
+{
+	unsigned long flags = 0;
+
+	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE2)
+		flags |= CXL_DECODER_F_TYPE2;
+	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE3)
+		flags |= CXL_DECODER_F_TYPE3;
+	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE)
+		flags |= CXL_DECODER_F_RAM;
+	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_PMEM)
+		flags |= CXL_DECODER_F_PMEM;
+	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_FIXED)
+		flags |= CXL_DECODER_F_LOCK;
+
+	return flags;
+}
+
+static int cxl_acpi_cfmws_verify(struct device *dev,
+				 struct acpi_cedt_cfmws *cfmws)
+{
+	int expected_len;
+
+	if (cfmws->interleave_arithmetic != ACPI_CEDT_CFMWS_ARITHMETIC_MODULO) {
+		dev_err(dev, "CFMWS Unsupported Interleave Arithmetic\n");
+		return -EINVAL;
+	}
+
+	if (!IS_ALIGNED(cfmws->base_hpa, SZ_256M)) {
+		dev_err(dev, "CFMWS Base HPA not 256MB aligned\n");
+		return -EINVAL;
+	}
+
+	if (!IS_ALIGNED(cfmws->window_size, SZ_256M)) {
+		dev_err(dev, "CFMWS Window Size not 256MB aligned\n");
+		return -EINVAL;
+	}
+
+	expected_len = struct_size((cfmws), interleave_targets,
+				   CFMWS_INTERLEAVE_WAYS(cfmws));
+
+	if (expected_len != cfmws->header.length) {
+		dev_err(dev, "CFMWS interleave ways and targets mismatch\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void cxl_add_cfmws_decoders(struct device *dev,
+				   struct cxl_port *root_port)
+{
+	struct acpi_cedt_cfmws *cfmws;
+	struct cxl_decoder *cxld;
+	acpi_size len, cur = 0;
+	void *cedt_base;
+	int rc;
+
+	len = cedt_table->length - sizeof(*cedt_table);
+	cedt_base = cedt_table + 1;
+
+	while (cur < len) {
+		struct acpi_cedt_header *c = cedt_base + cur;
+
+		if (c->type != ACPI_CEDT_TYPE_CFMWS) {
+			cur += c->length;
+			continue;
+		}
+
+		cfmws = cedt_base + cur;
+
+		if (cfmws->header.length < sizeof(*cfmws)) {
+			dev_err(dev, "Invalid CFMWS header length %u\n",
+				cfmws->header.length);
+			dev_err(dev, "Failed to add decoders\n");
+			return;
+		}
+
+		rc = cxl_acpi_cfmws_verify(dev, cfmws);
+		if (rc) {
+			dev_err(dev, "CFMWS range %#llx-%#llx not registered\n",
+				cfmws->base_hpa, cfmws->base_hpa +
+				cfmws->window_size - 1);
+			cur += c->length;
+			continue;
+		}
+
+		cxld = devm_cxl_add_decoder(dev, root_port,
+				CFMWS_INTERLEAVE_WAYS(cfmws),
+				cfmws->base_hpa, cfmws->window_size,
+				CFMWS_INTERLEAVE_WAYS(cfmws),
+				CFMWS_INTERLEAVE_GRANULARITY(cfmws),
+				CXL_DECODER_EXPANDER,
+				cfmws_to_decoder_flags(cfmws->restrictions));
+
+		if (IS_ERR(cxld)) {
+			dev_err(dev, "Failed to add decoder for %#llx-%#llx\n",
+				cfmws->base_hpa, cfmws->base_hpa +
+				cfmws->window_size - 1);
+		} else {
+			dev_dbg(dev, "add: %s range %#llx-%#llx\n",
+				dev_name(&cxld->dev), cfmws->base_hpa,
+				 cfmws->base_hpa + cfmws->window_size - 1);
+		}
+		cur += c->length;
+	}
+}
+
 static struct acpi_cedt_chbs *cxl_acpi_match_chbs(struct device *dev, u32 uid)
 {
 	struct acpi_cedt_chbs *chbs, *chbs_match = NULL;
@@ -251,6 +363,8 @@ static int cxl_acpi_probe(struct platform_device *pdev)
 	if (rc)
 		goto out;
 
+	cxl_add_cfmws_decoders(host, root_port);
+
 	/*
 	 * Root level scanned with host-bridge as dports, now scan host-bridges
 	 * for their role as CXL uports to their CXL-capable PCIe Root Ports.
-- 
2.26.2

Re: [PATCH v2 1/2] cxl/acpi: Add the Host Bridge base address to CXL port objects

[Ben Widawsky]

On 21-06-15 17:20:38, Alison Schofield wrote:
> The base address for the Host Bridge port component registers is located
> in the CXL Host Bridge Structure (CHBS) of the ACPI CXL Early Discovery
> Table (CEDT). Retrieve the CHBS for each Host Bridge (ACPI0016 device)
> and include that base address in the port object.
> 
> Co-developed-by: Vishal Verma <vishal.l.verma@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  drivers/cxl/acpi.c | 105 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 99 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index be357eea552c..b6d9cd45428c 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -8,6 +8,61 @@
>  #include <linux/pci.h>
>  #include "cxl.h"
>  
> +static struct acpi_table_header *cedt_table;

cedt_header would really be a better name. "Table" is redundant as the 't' in
CEDT is table.

> +
> +static struct acpi_cedt_chbs *cxl_acpi_match_chbs(struct device *dev, u32 uid)
> +{
> +	struct acpi_cedt_chbs *chbs, *chbs_match = NULL;
> +	acpi_size len, cur = 0;
> +	void *cedt_base;

maybe "cedt_body", or "cedt_subtables"

> +	int rc = 0;
> +
> +	len = cedt_table->length - sizeof(*cedt_table);
> +	cedt_base = cedt_table + 1;

As per naming recommendation above, this looks really funny...

> +
> +	while (cur < len) {
> +		struct acpi_cedt_header *c = cedt_base + cur;

Okay, now I see why you may have not called the previous thing a header.

> +
> +		if (c->type != ACPI_CEDT_TYPE_CHBS) {
> +			cur += c->length;
> +			continue;
> +		}
> +
> +		chbs = cedt_base + cur;
> +
> +		if (chbs->header.length < sizeof(*chbs)) {
> +			dev_err(dev, "Invalid CHBS header length: %u\n",
> +				chbs->header.length);
> +			rc = -EINVAL;
> +			break;
> +		}

I'd just continue here. Maybe there will be another chbs with the correct size.

> +
> +		if (chbs->uid == uid && !chbs_match) {
> +			chbs_match = chbs;
> +			cur += c->length;
> +			continue;
> +		}
> +
> +		if (chbs->uid == uid && chbs_match) {
> +			dev_err(dev, "Duplicate CHBS UIDs %u\n", uid);
> +			rc = -EINVAL;
> +			break;
> +		}

I'd also just continue here. I think if we have a match, we can just use it and
ignore BIOS bugs. I'd probably write it like this:

if (chbs->uid == uid) {
	dev_WARN_ONCE(dev, chbs_match, "Duplicate CHBS UIDs %u\n", uid);
	chbs_match = chbs; /* last one wins */
	cur += c->length;
	continue;
}

Up to you how you actually write it, but do consider not failing here.

> +		cur += c->length;
> +	}
> +	if (!chbs_match)
> +		rc = -EINVAL;

Maybe ENODEV or something like it is more appropriate?

> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	return chbs_match;
> +}
> +
> +static resource_size_t get_chbcr(struct acpi_cedt_chbs *chbs)
> +{
> +	return IS_ERR(chbs) ? CXL_RESOURCE_NONE : chbs->base;
> +}
> +
>  struct cxl_walk_context {
>  	struct device *dev;
>  	struct pci_bus *root;
> @@ -50,6 +105,21 @@ static int match_add_root_ports(struct pci_dev *pdev, void *data)
>  	return 0;
>  }
>  
> +static struct cxl_dport *find_dport_by_dev(struct cxl_port *port, struct device *dev)
> +{
> +	struct cxl_dport *dport;
> +
> +	device_lock(&port->dev);
> +	list_for_each_entry(dport, &port->dports, list)
> +		if (dport->dport == dev) {
> +			device_unlock(&port->dev);
> +			return dport;
> +		}
> +
> +	device_unlock(&port->dev);
> +	return NULL;
> +}
> +
>  static struct acpi_device *to_cxl_host_bridge(struct device *dev)
>  {
>  	struct acpi_device *adev = to_acpi_device(dev);
> @@ -71,6 +141,7 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>  	struct acpi_pci_root *pci_root;
>  	struct cxl_walk_context ctx;
>  	struct cxl_decoder *cxld;
> +	struct cxl_dport *dport;
>  	struct cxl_port *port;
>  
>  	if (!bridge)
> @@ -80,8 +151,15 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>  	if (!pci_root)
>  		return -ENXIO;
>  
> -	/* TODO: fold in CEDT.CHBS retrieval */
> -	port = devm_cxl_add_port(host, match, CXL_RESOURCE_NONE, root_port);
> +	dport = find_dport_by_dev(root_port, match);
> +	if (!dport) {
> +		dev_dbg(host, "host bridge expected and not found\n");
> +		return -ENODEV;
> +	}
> +
> +	port = devm_cxl_add_port(host, match, dport->component_reg_phys,
> +				 root_port);
> +
>  	if (IS_ERR(port))
>  		return PTR_ERR(port);
>  	dev_dbg(host, "%s: add: %s\n", dev_name(match), dev_name(&port->dev));
> @@ -120,6 +198,7 @@ static int add_host_bridge_dport(struct device *match, void *arg)
>  	int rc;
>  	acpi_status status;
>  	unsigned long long uid;
> +	struct acpi_cedt_chbs *chbs;
>  	struct cxl_port *root_port = arg;
>  	struct device *host = root_port->dev.parent;
>  	struct acpi_device *bridge = to_cxl_host_bridge(match);
> @@ -135,7 +214,12 @@ static int add_host_bridge_dport(struct device *match, void *arg)
>  		return -ENODEV;
>  	}
>  
> -	rc = cxl_add_dport(root_port, match, uid, CXL_RESOURCE_NONE);
> +	chbs = cxl_acpi_match_chbs(host, uid);
> +	if (IS_ERR(chbs))
> +		dev_dbg(host, "No CHBS found for Host Bridge: %s\n",
> +			dev_name(match));
> +
> +	rc = cxl_add_dport(root_port, match, uid, get_chbcr(chbs));
>  	if (rc) {
>  		dev_err(host, "failed to add downstream port: %s\n",
>  			dev_name(match));
> @@ -148,6 +232,7 @@ static int add_host_bridge_dport(struct device *match, void *arg)
>  static int cxl_acpi_probe(struct platform_device *pdev)
>  {
>  	int rc;
> +	acpi_status status;
>  	struct cxl_port *root_port;
>  	struct device *host = &pdev->dev;
>  	struct acpi_device *adev = ACPI_COMPANION(host);
> @@ -157,17 +242,25 @@ static int cxl_acpi_probe(struct platform_device *pdev)
>  		return PTR_ERR(root_port);
>  	dev_dbg(host, "add: %s\n", dev_name(&root_port->dev));
>  
> +	status = acpi_get_table(ACPI_SIG_CEDT, 0, &cedt_table);
> +	if (ACPI_FAILURE(status))
> +		return -ENXIO;
> +
>  	rc = bus_for_each_dev(adev->dev.bus, NULL, root_port,
>  			      add_host_bridge_dport);
>  	if (rc)
> -		return rc;
> +		goto out;
>  
>  	/*
>  	 * Root level scanned with host-bridge as dports, now scan host-bridges
>  	 * for their role as CXL uports to their CXL-capable PCIe Root Ports.
>  	 */
> -	return bus_for_each_dev(adev->dev.bus, NULL, root_port,
> -				add_host_bridge_uport);
> +	rc = bus_for_each_dev(adev->dev.bus, NULL, root_port,
> +			      add_host_bridge_uport);
> +
> +out:
> +	acpi_put_table(cedt_table);
> +	return rc;
>  }
>  
>  static const struct acpi_device_id cxl_acpi_ids[] = {
> -- 
> 2.26.2
> 

Re: [PATCH v2 1/2] cxl/acpi: Add the Host Bridge base address to CXL port objects

[Jonathan Cameron]

On Tue, 15 Jun 2021 17:20:38 -0700
Alison Schofield <alison.schofield@intel.com> wrote:

> The base address for the Host Bridge port component registers is located
> in the CXL Host Bridge Structure (CHBS) of the ACPI CXL Early Discovery
> Table (CEDT). Retrieve the CHBS for each Host Bridge (ACPI0016 device)
> and include that base address in the port object.
> 
> Co-developed-by: Vishal Verma <vishal.l.verma@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

Hi Alison,

A few small suggestions from me.

> ---
>  drivers/cxl/acpi.c | 105 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 99 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index be357eea552c..b6d9cd45428c 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -8,6 +8,61 @@
>  #include <linux/pci.h>
>  #include "cxl.h"
>  
> +static struct acpi_table_header *cedt_table;
> +
> +static struct acpi_cedt_chbs *cxl_acpi_match_chbs(struct device *dev, u32 uid)
> +{
> +	struct acpi_cedt_chbs *chbs, *chbs_match = NULL;
> +	acpi_size len, cur = 0;
> +	void *cedt_base;
> +	int rc = 0;
> +
> +	len = cedt_table->length - sizeof(*cedt_table);
> +	cedt_base = cedt_table + 1;
> +
> +	while (cur < len) {
> +		struct acpi_cedt_header *c = cedt_base + cur;
> +
> +		if (c->type != ACPI_CEDT_TYPE_CHBS) {
> +			cur += c->length;
> +			continue;
> +		}
> +
> +		chbs = cedt_base + cur;
> +
> +		if (chbs->header.length < sizeof(*chbs)) {
> +			dev_err(dev, "Invalid CHBS header length: %u\n",
> +				chbs->header.length);
> +			rc = -EINVAL;

As below, direct return would be more obvious to my eyes.

> +			break;
> +		}
> +
> +		if (chbs->uid == uid && !chbs_match) {
> +			chbs_match = chbs;
> +			cur += c->length;
> +			continue;
> +		}
> +
> +		if (chbs->uid == uid && chbs_match) {
> +			dev_err(dev, "Duplicate CHBS UIDs %u\n", uid);

Do we actually care, or should we just drop out on first match?
I don't think think there is any obligation to catch broken tables.

> +			rc = -EINVAL;

Direct return might be easier to follow.
			return ERR_PTR(-EINVAL);

> +			break;
> +		}

Maybe more readable as (your option is fine if you prefer it).

		if (chbs->uuid != uid) {
			cur += c->length;
			continue;
		}

		if (chbs_match) {
			dev_err(dev, "D...");
			return ERR_PTR(-EINVAL);
		}

		chbs_match = chbs;


> +		cur += c->length;
> +	}
> +	if (!chbs_match)
> +		rc = -EINVAL;

		return ERR_PTR(-EINVAL);

> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	return chbs_match;
> +}
> +
> +static resource_size_t get_chbcr(struct acpi_cedt_chbs *chbs)
> +{
> +	return IS_ERR(chbs) ? CXL_RESOURCE_NONE : chbs->base;
> +}
> +
>  struct cxl_walk_context {
>  	struct device *dev;
>  	struct pci_bus *root;
> @@ -50,6 +105,21 @@ static int match_add_root_ports(struct pci_dev *pdev, void *data)
>  	return 0;
>  }
>  
> +static struct cxl_dport *find_dport_by_dev(struct cxl_port *port, struct device *dev)
> +{
> +	struct cxl_dport *dport;
> +
> +	device_lock(&port->dev);
> +	list_for_each_entry(dport, &port->dports, list)
> +		if (dport->dport == dev) {
> +			device_unlock(&port->dev);
> +			return dport;
> +		}
> +
> +	device_unlock(&port->dev);
> +	return NULL;
> +}
> +
>  static struct acpi_device *to_cxl_host_bridge(struct device *dev)
>  {
>  	struct acpi_device *adev = to_acpi_device(dev);
> @@ -71,6 +141,7 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>  	struct acpi_pci_root *pci_root;
>  	struct cxl_walk_context ctx;
>  	struct cxl_decoder *cxld;
> +	struct cxl_dport *dport;
>  	struct cxl_port *port;
>  
>  	if (!bridge)
> @@ -80,8 +151,15 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>  	if (!pci_root)
>  		return -ENXIO;
>  
> -	/* TODO: fold in CEDT.CHBS retrieval */
> -	port = devm_cxl_add_port(host, match, CXL_RESOURCE_NONE, root_port);
> +	dport = find_dport_by_dev(root_port, match);
> +	if (!dport) {
> +		dev_dbg(host, "host bridge expected and not found\n");
> +		return -ENODEV;
> +	}
> +
> +	port = devm_cxl_add_port(host, match, dport->component_reg_phys,
> +				 root_port);
> +

Nitpick, no blank line before error handling block.

>  	if (IS_ERR(port))
>  		return PTR_ERR(port);
>  	dev_dbg(host, "%s: add: %s\n", dev_name(match), dev_name(&port->dev));
> @@ -120,6 +198,7 @@ static int add_host_bridge_dport(struct device *match, void *arg)
>  	int rc;
>  	acpi_status status;
>  	unsigned long long uid;
> +	struct acpi_cedt_chbs *chbs;
>  	struct cxl_port *root_port = arg;
>  	struct device *host = root_port->dev.parent;
>  	struct acpi_device *bridge = to_cxl_host_bridge(match);
> @@ -135,7 +214,12 @@ static int add_host_bridge_dport(struct device *match, void *arg)
>  		return -ENODEV;
>  	}
>  
> -	rc = cxl_add_dport(root_port, match, uid, CXL_RESOURCE_NONE);
> +	chbs = cxl_acpi_match_chbs(host, uid);
> +	if (IS_ERR(chbs))
> +		dev_dbg(host, "No CHBS found for Host Bridge: %s\n",
> +			dev_name(match));
> +
> +	rc = cxl_add_dport(root_port, match, uid, get_chbcr(chbs));
>  	if (rc) {
>  		dev_err(host, "failed to add downstream port: %s\n",
>  			dev_name(match));
> @@ -148,6 +232,7 @@ static int add_host_bridge_dport(struct device *match, void *arg)
>  static int cxl_acpi_probe(struct platform_device *pdev)
>  {
>  	int rc;
> +	acpi_status status;
>  	struct cxl_port *root_port;
>  	struct device *host = &pdev->dev;
>  	struct acpi_device *adev = ACPI_COMPANION(host);
> @@ -157,17 +242,25 @@ static int cxl_acpi_probe(struct platform_device *pdev)
>  		return PTR_ERR(root_port);
>  	dev_dbg(host, "add: %s\n", dev_name(&root_port->dev));
>  
> +	status = acpi_get_table(ACPI_SIG_CEDT, 0, &cedt_table);
> +	if (ACPI_FAILURE(status))
> +		return -ENXIO;
> +
>  	rc = bus_for_each_dev(adev->dev.bus, NULL, root_port,
>  			      add_host_bridge_dport);
>  	if (rc)
> -		return rc;
> +		goto out;
>  
>  	/*
>  	 * Root level scanned with host-bridge as dports, now scan host-bridges
>  	 * for their role as CXL uports to their CXL-capable PCIe Root Ports.
>  	 */
> -	return bus_for_each_dev(adev->dev.bus, NULL, root_port,
> -				add_host_bridge_uport);
> +	rc = bus_for_each_dev(adev->dev.bus, NULL, root_port,
> +			      add_host_bridge_uport);
> +
> +out:
> +	acpi_put_table(cedt_table);
> +	return rc;
>  }
>  
>  static const struct acpi_device_id cxl_acpi_ids[] = {

Re: [PATCH v2 2/2] cxl/acpi: Use the ACPI CFMWS to create static decoder objects

[Ben Widawsky]

On 21-06-15 17:20:39, Alison Schofield wrote:
> The ACPI CXL Early Discovery Table (CEDT) includes a list of CXL memory
> resources in CXL Fixed Memory Window Structures (CFMWS). Retrieve each
> CFMWS in the CEDT and add a cxl_decoder object to the root port (root0)
> for each memory resource.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  drivers/cxl/acpi.c | 114 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 114 insertions(+)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index b6d9cd45428c..e3aa356d4dcd 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -8,8 +8,120 @@
>  #include <linux/pci.h>
>  #include "cxl.h"
>  
> +/* Encode defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register */
> +#define CFMWS_INTERLEAVE_WAYS(x)	(1 << (x)->interleave_ways)
> +#define CFMWS_INTERLEAVE_GRANULARITY(x)	((x)->granularity + 8)
> +
>  static struct acpi_table_header *cedt_table;
>  
> +static unsigned long cfmws_to_decoder_flags(int restrictions)
> +{
> +	unsigned long flags = 0;
> +
> +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE2)
> +		flags |= CXL_DECODER_F_TYPE2;
> +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE3)
> +		flags |= CXL_DECODER_F_TYPE3;
> +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE)
> +		flags |= CXL_DECODER_F_RAM;
> +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_PMEM)
> +		flags |= CXL_DECODER_F_PMEM;
> +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_FIXED)
> +		flags |= CXL_DECODER_F_LOCK;
> +
> +	return flags;
> +}

I know these flags aren't introduced by this patch, but I'm wondering if it
makes sense to not just use the spec definitions rather than defining our own.
It doesn't do much harm, but it's extra typing everytime the spec adds new flags
and I don't really see the upside.

> +
> +static int cxl_acpi_cfmws_verify(struct device *dev,
> +				 struct acpi_cedt_cfmws *cfmws)
> +{
> +	int expected_len;
> +
> +	if (cfmws->interleave_arithmetic != ACPI_CEDT_CFMWS_ARITHMETIC_MODULO) {
> +		dev_err(dev, "CFMWS Unsupported Interleave Arithmetic\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!IS_ALIGNED(cfmws->base_hpa, SZ_256M)) {
> +		dev_err(dev, "CFMWS Base HPA not 256MB aligned\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!IS_ALIGNED(cfmws->window_size, SZ_256M)) {
> +		dev_err(dev, "CFMWS Window Size not 256MB aligned\n");
> +		return -EINVAL;
> +	}
> +
> +	expected_len = struct_size((cfmws), interleave_targets,
> +				   CFMWS_INTERLEAVE_WAYS(cfmws));
> +
> +	if (expected_len != cfmws->header.length) {

I'd switch this to:
if (expected_len < cfmws->header.length)

If it's too big, just print a dev_dbg.

> +		dev_err(dev, "CFMWS interleave ways and targets mismatch\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void cxl_add_cfmws_decoders(struct device *dev,
> +				   struct cxl_port *root_port)
> +{
> +	struct acpi_cedt_cfmws *cfmws;
> +	struct cxl_decoder *cxld;
> +	acpi_size len, cur = 0;
> +	void *cedt_base;
> +	int rc;
> +
> +	len = cedt_table->length - sizeof(*cedt_table);
> +	cedt_base = cedt_table + 1;

naming suggestions per previous patch... up to you though.

> +
> +	while (cur < len) {
> +		struct acpi_cedt_header *c = cedt_base + cur;
> +
> +		if (c->type != ACPI_CEDT_TYPE_CFMWS) {
> +			cur += c->length;
> +			continue;
> +		}
> +
> +		cfmws = cedt_base + cur;
> +
> +		if (cfmws->header.length < sizeof(*cfmws)) {
> +			dev_err(dev, "Invalid CFMWS header length %u\n",
> +				cfmws->header.length);
> +			dev_err(dev, "Failed to add decoders\n");
> +			return;
> +		}
> +
> +		rc = cxl_acpi_cfmws_verify(dev, cfmws);
> +		if (rc) {
> +			dev_err(dev, "CFMWS range %#llx-%#llx not registered\n",
> +				cfmws->base_hpa, cfmws->base_hpa +
> +				cfmws->window_size - 1);
> +			cur += c->length;
> +			continue;
> +		}
> +
> +		cxld = devm_cxl_add_decoder(dev, root_port,
> +				CFMWS_INTERLEAVE_WAYS(cfmws),
> +				cfmws->base_hpa, cfmws->window_size,
> +				CFMWS_INTERLEAVE_WAYS(cfmws),

Interesting... this made me question, how can we have a different number of
targets and ways?

> +				CFMWS_INTERLEAVE_GRANULARITY(cfmws),
> +				CXL_DECODER_EXPANDER,
> +				cfmws_to_decoder_flags(cfmws->restrictions));
> +
> +		if (IS_ERR(cxld)) {
> +			dev_err(dev, "Failed to add decoder for %#llx-%#llx\n",
> +				cfmws->base_hpa, cfmws->base_hpa +
> +				cfmws->window_size - 1);
> +		} else {
> +			dev_dbg(dev, "add: %s range %#llx-%#llx\n",
> +				dev_name(&cxld->dev), cfmws->base_hpa,
> +				 cfmws->base_hpa + cfmws->window_size - 1);
> +		}
> +		cur += c->length;
> +	}
> +}
> +
>  static struct acpi_cedt_chbs *cxl_acpi_match_chbs(struct device *dev, u32 uid)
>  {
>  	struct acpi_cedt_chbs *chbs, *chbs_match = NULL;
> @@ -251,6 +363,8 @@ static int cxl_acpi_probe(struct platform_device *pdev)
>  	if (rc)
>  		goto out;
>  
> +	cxl_add_cfmws_decoders(host, root_port);
> +
>  	/*
>  	 * Root level scanned with host-bridge as dports, now scan host-bridges
>  	 * for their role as CXL uports to their CXL-capable PCIe Root Ports.
> -- 
> 2.26.2
> 

Re: [PATCH v2 2/2] cxl/acpi: Use the ACPI CFMWS to create static decoder objects

[Dan Williams]

On Wed, Jun 16, 2021 at 9:17 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 21-06-15 17:20:39, Alison Schofield wrote:
> > The ACPI CXL Early Discovery Table (CEDT) includes a list of CXL memory
> > resources in CXL Fixed Memory Window Structures (CFMWS). Retrieve each
> > CFMWS in the CEDT and add a cxl_decoder object to the root port (root0)
> > for each memory resource.
> >
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  drivers/cxl/acpi.c | 114 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 114 insertions(+)
> >
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index b6d9cd45428c..e3aa356d4dcd 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -8,8 +8,120 @@
> >  #include <linux/pci.h>
> >  #include "cxl.h"
> >
> > +/* Encode defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register */
> > +#define CFMWS_INTERLEAVE_WAYS(x)     (1 << (x)->interleave_ways)
> > +#define CFMWS_INTERLEAVE_GRANULARITY(x)      ((x)->granularity + 8)
> > +
> >  static struct acpi_table_header *cedt_table;
> >
> > +static unsigned long cfmws_to_decoder_flags(int restrictions)
> > +{
> > +     unsigned long flags = 0;
> > +
> > +     if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE2)
> > +             flags |= CXL_DECODER_F_TYPE2;
> > +     if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE3)
> > +             flags |= CXL_DECODER_F_TYPE3;
> > +     if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE)
> > +             flags |= CXL_DECODER_F_RAM;
> > +     if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_PMEM)
> > +             flags |= CXL_DECODER_F_PMEM;
> > +     if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_FIXED)
> > +             flags |= CXL_DECODER_F_LOCK;
> > +
> > +     return flags;
> > +}
>
> I know these flags aren't introduced by this patch, but I'm wondering if it
> makes sense to not just use the spec definitions rather than defining our own.
> It doesn't do much harm, but it's extra typing everytime the spec adds new flags
> and I don't really see the upside.

The flags are bounded by what's in HDM decoders, I don't see them
moving so fast that the kernel can not keep up. The rationale for the
split is the same as the split between ACPI NFIT and the LIBNVDIMM
core. The ACPI specifics are just one way to convey a common platform
attribute to the core.

In fact this was one of the main feedbacks of the initial "ND"
subsystem which eventually became LIBNVDIMM [1]. ND stood for "NFIT
Defined" and the arch split between ACPI specific and Linux
translation has paid off over the years.

[1]: https://lore.kernel.org/lkml/20150420070624.GB13876@gmail.com/


>
> > +
> > +static int cxl_acpi_cfmws_verify(struct device *dev,
> > +                              struct acpi_cedt_cfmws *cfmws)
> > +{
> > +     int expected_len;
> > +
> > +     if (cfmws->interleave_arithmetic != ACPI_CEDT_CFMWS_ARITHMETIC_MODULO) {
> > +             dev_err(dev, "CFMWS Unsupported Interleave Arithmetic\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (!IS_ALIGNED(cfmws->base_hpa, SZ_256M)) {
> > +             dev_err(dev, "CFMWS Base HPA not 256MB aligned\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (!IS_ALIGNED(cfmws->window_size, SZ_256M)) {
> > +             dev_err(dev, "CFMWS Window Size not 256MB aligned\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     expected_len = struct_size((cfmws), interleave_targets,
> > +                                CFMWS_INTERLEAVE_WAYS(cfmws));
> > +
> > +     if (expected_len != cfmws->header.length) {
>
> I'd switch this to:
> if (expected_len < cfmws->header.length)
>
> If it's too big, just print a dev_dbg.

Maybe call it min_len then?

[..]
> > +
> > +             cxld = devm_cxl_add_decoder(dev, root_port,
> > +                             CFMWS_INTERLEAVE_WAYS(cfmws),
> > +                             cfmws->base_hpa, cfmws->window_size,
> > +                             CFMWS_INTERLEAVE_WAYS(cfmws),
>
> Interesting... this made me question, how can we have a different number of
> targets and ways?

These settings can be changed later on a switch-level decoder, for a
root-level decoder these initial values are fixed.

Re: [PATCH v2 2/2] cxl/acpi: Use the ACPI CFMWS to create static decoder objects

[Jonathan Cameron]

On Tue, 15 Jun 2021 17:20:39 -0700
Alison Schofield <alison.schofield@intel.com> wrote:

> The ACPI CXL Early Discovery Table (CEDT) includes a list of CXL memory
> resources in CXL Fixed Memory Window Structures (CFMWS). Retrieve each
> CFMWS in the CEDT and add a cxl_decoder object to the root port (root0)
> for each memory resource.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

LGTM

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/acpi.c | 114 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 114 insertions(+)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index b6d9cd45428c..e3aa356d4dcd 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -8,8 +8,120 @@
>  #include <linux/pci.h>
>  #include "cxl.h"
>  
> +/* Encode defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register */
> +#define CFMWS_INTERLEAVE_WAYS(x)	(1 << (x)->interleave_ways)
> +#define CFMWS_INTERLEAVE_GRANULARITY(x)	((x)->granularity + 8)
> +
>  static struct acpi_table_header *cedt_table;
>  
> +static unsigned long cfmws_to_decoder_flags(int restrictions)
> +{
> +	unsigned long flags = 0;
> +
> +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE2)
> +		flags |= CXL_DECODER_F_TYPE2;
> +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE3)
> +		flags |= CXL_DECODER_F_TYPE3;
> +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE)
> +		flags |= CXL_DECODER_F_RAM;
> +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_PMEM)
> +		flags |= CXL_DECODER_F_PMEM;
> +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_FIXED)
> +		flags |= CXL_DECODER_F_LOCK;
> +
> +	return flags;
> +}
> +
> +static int cxl_acpi_cfmws_verify(struct device *dev,
> +				 struct acpi_cedt_cfmws *cfmws)
> +{
> +	int expected_len;
> +
> +	if (cfmws->interleave_arithmetic != ACPI_CEDT_CFMWS_ARITHMETIC_MODULO) {
> +		dev_err(dev, "CFMWS Unsupported Interleave Arithmetic\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!IS_ALIGNED(cfmws->base_hpa, SZ_256M)) {
> +		dev_err(dev, "CFMWS Base HPA not 256MB aligned\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!IS_ALIGNED(cfmws->window_size, SZ_256M)) {
> +		dev_err(dev, "CFMWS Window Size not 256MB aligned\n");
> +		return -EINVAL;
> +	}
> +
> +	expected_len = struct_size((cfmws), interleave_targets,
> +				   CFMWS_INTERLEAVE_WAYS(cfmws));
> +
> +	if (expected_len != cfmws->header.length) {
> +		dev_err(dev, "CFMWS interleave ways and targets mismatch\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static void cxl_add_cfmws_decoders(struct device *dev,
> +				   struct cxl_port *root_port)
> +{
> +	struct acpi_cedt_cfmws *cfmws;
> +	struct cxl_decoder *cxld;
> +	acpi_size len, cur = 0;
> +	void *cedt_base;
> +	int rc;
> +
> +	len = cedt_table->length - sizeof(*cedt_table);
> +	cedt_base = cedt_table + 1;
> +
> +	while (cur < len) {
> +		struct acpi_cedt_header *c = cedt_base + cur;
> +
> +		if (c->type != ACPI_CEDT_TYPE_CFMWS) {
> +			cur += c->length;
> +			continue;
> +		}
> +
> +		cfmws = cedt_base + cur;
> +
> +		if (cfmws->header.length < sizeof(*cfmws)) {
> +			dev_err(dev, "Invalid CFMWS header length %u\n",
> +				cfmws->header.length);
> +			dev_err(dev, "Failed to add decoders\n");
> +			return;
> +		}
> +
> +		rc = cxl_acpi_cfmws_verify(dev, cfmws);
> +		if (rc) {
> +			dev_err(dev, "CFMWS range %#llx-%#llx not registered\n",
> +				cfmws->base_hpa, cfmws->base_hpa +
> +				cfmws->window_size - 1);
> +			cur += c->length;
> +			continue;
> +		}
> +
> +		cxld = devm_cxl_add_decoder(dev, root_port,
> +				CFMWS_INTERLEAVE_WAYS(cfmws),
> +				cfmws->base_hpa, cfmws->window_size,
> +				CFMWS_INTERLEAVE_WAYS(cfmws),
> +				CFMWS_INTERLEAVE_GRANULARITY(cfmws),
> +				CXL_DECODER_EXPANDER,
> +				cfmws_to_decoder_flags(cfmws->restrictions));
> +
> +		if (IS_ERR(cxld)) {
> +			dev_err(dev, "Failed to add decoder for %#llx-%#llx\n",
> +				cfmws->base_hpa, cfmws->base_hpa +
> +				cfmws->window_size - 1);
> +		} else {
> +			dev_dbg(dev, "add: %s range %#llx-%#llx\n",
> +				dev_name(&cxld->dev), cfmws->base_hpa,
> +				 cfmws->base_hpa + cfmws->window_size - 1);
> +		}
> +		cur += c->length;
> +	}
> +}
> +
>  static struct acpi_cedt_chbs *cxl_acpi_match_chbs(struct device *dev, u32 uid)
>  {
>  	struct acpi_cedt_chbs *chbs, *chbs_match = NULL;
> @@ -251,6 +363,8 @@ static int cxl_acpi_probe(struct platform_device *pdev)
>  	if (rc)
>  		goto out;
>  
> +	cxl_add_cfmws_decoders(host, root_port);
> +
>  	/*
>  	 * Root level scanned with host-bridge as dports, now scan host-bridges
>  	 * for their role as CXL uports to their CXL-capable PCIe Root Ports.

Re: [PATCH v2 1/2] cxl/acpi: Add the Host Bridge base address to CXL port objects

[Alison Schofield]

Thanks for the review Ben -

On Wed, Jun 16, 2021 at 09:08:16AM -0700, Ben Widawsky wrote:
> On 21-06-15 17:20:38, Alison Schofield wrote:
> > The base address for the Host Bridge port component registers is located
> > in the CXL Host Bridge Structure (CHBS) of the ACPI CXL Early Discovery
> > Table (CEDT). Retrieve the CHBS for each Host Bridge (ACPI0016 device)
> > and include that base address in the port object.
> > 
> > Co-developed-by: Vishal Verma <vishal.l.verma@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  drivers/cxl/acpi.c | 105 ++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 99 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index be357eea552c..b6d9cd45428c 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -8,6 +8,61 @@
> >  #include <linux/pci.h>
> >  #include "cxl.h"
> >  
> > +static struct acpi_table_header *cedt_table;
> 
> cedt_header would really be a better name. "Table" is redundant as the 't' in
> CEDT is table.
> 

Agree. Renamed to acpi_cedt in v3. See if you like.

> > +
> > +static struct acpi_cedt_chbs *cxl_acpi_match_chbs(struct device *dev, u32 uid)
> > +{
> > +	struct acpi_cedt_chbs *chbs, *chbs_match = NULL;
> > +	acpi_size len, cur = 0;
> > +	void *cedt_base;
> 
> maybe "cedt_body", or "cedt_subtables"

got it in v3.

> 
> > +	int rc = 0;
> > +
> > +	len = cedt_table->length - sizeof(*cedt_table);
> > +	cedt_base = cedt_table + 1;
> 
> As per naming recommendation above, this looks really funny...
> 
:)
> > +
> > +	while (cur < len) {
> > +		struct acpi_cedt_header *c = cedt_base + cur;
> 
> Okay, now I see why you may have not called the previous thing a header.
> 
> > +
> > +		if (c->type != ACPI_CEDT_TYPE_CHBS) {
> > +			cur += c->length;
> > +			continue;
> > +		}
> > +
> > +		chbs = cedt_base + cur;
> > +
> > +		if (chbs->header.length < sizeof(*chbs)) {
> > +			dev_err(dev, "Invalid CHBS header length: %u\n",
> > +				chbs->header.length);
> > +			rc = -EINVAL;
> > +			break;
> > +		}
> 
> I'd just continue here. Maybe there will be another chbs with the correct size.
>

Got it.

> > +
> > +		if (chbs->uid == uid && !chbs_match) {
> > +			chbs_match = chbs;
> > +			cur += c->length;
> > +			continue;
> > +		}
> > +
> > +		if (chbs->uid == uid && chbs_match) {
> > +			dev_err(dev, "Duplicate CHBS UIDs %u\n", uid);
> > +			rc = -EINVAL;
> > +			break;
> > +		}
> 
> I'd also just continue here. I think if we have a match, we can just use it and
> ignore BIOS bugs. I'd probably write it like this:
> 
> if (chbs->uid == uid) {
> 	dev_WARN_ONCE(dev, chbs_match, "Duplicate CHBS UIDs %u\n", uid);
> 	chbs_match = chbs; /* last one wins */
> 	cur += c->length;
> 	continue;
> }
> 
> Up to you how you actually write it, but do consider not failing here.
>

Thanks for the snippet. I added the dev_WARN_ONCE to both the length
mismatch and duplicate cases. 


> > +		cur += c->length;
> > +	}
> > +	if (!chbs_match)
> > +		rc = -EINVAL;
> 
> Maybe ENODEV or something like it is more appropriate?

Got it.

> 
snip

Re: [PATCH v2 1/2] cxl/acpi: Add the Host Bridge base address to CXL port objects

[Alison Schofield]

Thanks for the review Jonathan -

On Wed, Jun 16, 2021 at 05:13:40PM +0100, Jonathan Cameron wrote:
> On Tue, 15 Jun 2021 17:20:38 -0700
> Alison Schofield <alison.schofield@intel.com> wrote:
> 
> > The base address for the Host Bridge port component registers is located
> > in the CXL Host Bridge Structure (CHBS) of the ACPI CXL Early Discovery
> > Table (CEDT). Retrieve the CHBS for each Host Bridge (ACPI0016 device)
> > and include that base address in the port object.
> > 
> > Co-developed-by: Vishal Verma <vishal.l.verma@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> 
> Hi Alison,
> 
> A few small suggestions from me.
> 
> > ---
> >  drivers/cxl/acpi.c | 105 ++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 99 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index be357eea552c..b6d9cd45428c 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -8,6 +8,61 @@
> >  #include <linux/pci.h>
> >  #include "cxl.h"
> >  
> > +static struct acpi_table_header *cedt_table;
> > +
> > +static struct acpi_cedt_chbs *cxl_acpi_match_chbs(struct device *dev, u32 uid)
> > +{
> > +	struct acpi_cedt_chbs *chbs, *chbs_match = NULL;
> > +	acpi_size len, cur = 0;
> > +	void *cedt_base;
> > +	int rc = 0;
> > +
> > +	len = cedt_table->length - sizeof(*cedt_table);
> > +	cedt_base = cedt_table + 1;
> > +
> > +	while (cur < len) {
> > +		struct acpi_cedt_header *c = cedt_base + cur;
> > +
> > +		if (c->type != ACPI_CEDT_TYPE_CHBS) {
> > +			cur += c->length;
> > +			continue;
> > +		}
> > +
> > +		chbs = cedt_base + cur;
> > +
> > +		if (chbs->header.length < sizeof(*chbs)) {
> > +			dev_err(dev, "Invalid CHBS header length: %u\n",
> > +				chbs->header.length);
> > +			rc = -EINVAL;
> 
> As below, direct return would be more obvious to my eyes.
> 

Well....I decided to warn & continue on this case. See the updated flow
in v3.

> > +			break;
> > +		}
> > +
> > +		if (chbs->uid == uid && !chbs_match) {
> > +			chbs_match = chbs;
> > +			cur += c->length;
> > +			continue;
> > +		}
> > +
> > +		if (chbs->uid == uid && chbs_match) {
> > +			dev_err(dev, "Duplicate CHBS UIDs %u\n", uid);
> 
> Do we actually care, or should we just drop out on first match?
> I don't think think there is any obligation to catch broken tables.
> 

Agree on the obligation part, but if things go wrong, this would be
nice to know. I left it in as a dev warn once. If you think that's 
too strong - let me know.


> > +			rc = -EINVAL;
> 
> Direct return might be easier to follow.
> 			return ERR_PTR(-EINVAL);
> 
> > +			break;
> > +		}
> 
> Maybe more readable as (your option is fine if you prefer it).
> 
> 		if (chbs->uuid != uid) {
> 			cur += c->length;
> 			continue;
> 		}
> 
> 		if (chbs_match) {
> 			dev_err(dev, "D...");
> 			return ERR_PTR(-EINVAL);
> 		}
> 
> 		chbs_match = chbs;
> 
> 

Thanks, I reworked the flow along these lines.


snip
> > +
> > +	port = devm_cxl_add_port(host, match, dport->component_reg_phys,
> > +				 root_port);
> > +
> 
> Nitpick, no blank line before error handling block.
> 

Got it. Thanks!

snip

Re: [PATCH v2 2/2] cxl/acpi: Use the ACPI CFMWS to create static decoder objects

[Alison Schofield]

Thanks for the review Ben -

On Wed, Jun 16, 2021 at 09:17:40AM -0700, Ben Widawsky wrote:
> On 21-06-15 17:20:39, Alison Schofield wrote:

snip

> > +static unsigned long cfmws_to_decoder_flags(int restrictions)
> > +{
> > +	unsigned long flags = 0;
> > +
> > +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE2)
> > +		flags |= CXL_DECODER_F_TYPE2;
> > +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE3)
> > +		flags |= CXL_DECODER_F_TYPE3;
> > +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE)
> > +		flags |= CXL_DECODER_F_RAM;
> > +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_PMEM)
> > +		flags |= CXL_DECODER_F_PMEM;
> > +	if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_FIXED)
> > +		flags |= CXL_DECODER_F_LOCK;
> > +
> > +	return flags;
> > +}
> 
> I know these flags aren't introduced by this patch, but I'm wondering if it
> makes sense to not just use the spec definitions rather than defining our own.
> It doesn't do much harm, but it's extra typing everytime the spec adds new flags
> and I don't really see the upside.
> 

I think Dan's email in this thread covered this.

snip
> > +
> > +static int cxl_acpi_cfmws_verify(struct device *dev,
> > +				 struct acpi_cedt_cfmws *cfmws)
> > +{

snip

> > +
> > +
> > +	expected_len = struct_size((cfmws), interleave_targets,
> > +				   CFMWS_INTERLEAVE_WAYS(cfmws));
> > +
> > +	if (expected_len != cfmws->header.length) {
> 
> I'd switch this to:
> if (expected_len < cfmws->header.length)
> 
> If it's too big, just print a dev_dbg.
> 

Got it. 

snip

> > +	void *cedt_base;
> > +	int rc;
> > +
> > +	len = cedt_table->length - sizeof(*cedt_table);
> > +	cedt_base = cedt_table + 1;
> 
> naming suggestions per previous patch... up to you though.
>

Ditto w previous patch.

snip
> > +
> > +		}
> > +
> > +		cxld = devm_cxl_add_decoder(dev, root_port,
> > +				CFMWS_INTERLEAVE_WAYS(cfmws),
> > +				cfmws->base_hpa, cfmws->window_size,
> > +				CFMWS_INTERLEAVE_WAYS(cfmws),
> 
> Interesting... this made me question, how can we have a different number of
> targets and ways?
> 

Dan explained this previously:

"nr_targets is the number of possible targets that this decoder can
target. For CFMWS it just equals interleave_ways because the target
list can't be changed. A switch on the other hand could support up to
16 possible targets, but be dynamically configured to only do a 1-way
interleave. So this is an artifact of 'struct cxl_decoder'
representing both fixed CFMWS entries and dynamically programmable
switch entries. nr_targets tells devm_cxl_add_decoder() how much
memory to allocate for its target list, interleave_ways tells
devm_cxl_add_decoder() what the decoder is currently programmed to
decode."


> 
snip
> 

Re: [PATCH v2 1/2] cxl/acpi: Add the Host Bridge base address to CXL port objects

[Dan Williams]

On Wed, Jun 16, 2021 at 4:20 PM Alison Schofield
<alison.schofield@intel.com> wrote:
>
>
> Thanks for the review Jonathan -
>
> On Wed, Jun 16, 2021 at 05:13:40PM +0100, Jonathan Cameron wrote:
> > On Tue, 15 Jun 2021 17:20:38 -0700
> > Alison Schofield <alison.schofield@intel.com> wrote:
> >
> > > The base address for the Host Bridge port component registers is located
> > > in the CXL Host Bridge Structure (CHBS) of the ACPI CXL Early Discovery
> > > Table (CEDT). Retrieve the CHBS for each Host Bridge (ACPI0016 device)
> > > and include that base address in the port object.
> > >
> > > Co-developed-by: Vishal Verma <vishal.l.verma@intel.com>
> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> >
> > Hi Alison,
> >
> > A few small suggestions from me.
> >
> > > ---
> > >  drivers/cxl/acpi.c | 105 ++++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 99 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > > index be357eea552c..b6d9cd45428c 100644
> > > --- a/drivers/cxl/acpi.c
> > > +++ b/drivers/cxl/acpi.c
> > > @@ -8,6 +8,61 @@
> > >  #include <linux/pci.h>
> > >  #include "cxl.h"
> > >
> > > +static struct acpi_table_header *cedt_table;
> > > +
> > > +static struct acpi_cedt_chbs *cxl_acpi_match_chbs(struct device *dev, u32 uid)
> > > +{
> > > +   struct acpi_cedt_chbs *chbs, *chbs_match = NULL;
> > > +   acpi_size len, cur = 0;
> > > +   void *cedt_base;
> > > +   int rc = 0;
> > > +
> > > +   len = cedt_table->length - sizeof(*cedt_table);
> > > +   cedt_base = cedt_table + 1;
> > > +
> > > +   while (cur < len) {
> > > +           struct acpi_cedt_header *c = cedt_base + cur;
> > > +
> > > +           if (c->type != ACPI_CEDT_TYPE_CHBS) {
> > > +                   cur += c->length;
> > > +                   continue;
> > > +           }
> > > +
> > > +           chbs = cedt_base + cur;
> > > +
> > > +           if (chbs->header.length < sizeof(*chbs)) {
> > > +                   dev_err(dev, "Invalid CHBS header length: %u\n",
> > > +                           chbs->header.length);
> > > +                   rc = -EINVAL;
> >
> > As below, direct return would be more obvious to my eyes.
> >
>
> Well....I decided to warn & continue on this case. See the updated flow
> in v3.
>
> > > +                   break;
> > > +           }
> > > +
> > > +           if (chbs->uid == uid && !chbs_match) {
> > > +                   chbs_match = chbs;
> > > +                   cur += c->length;
> > > +                   continue;
> > > +           }
> > > +
> > > +           if (chbs->uid == uid && chbs_match) {
> > > +                   dev_err(dev, "Duplicate CHBS UIDs %u\n", uid);
> >
> > Do we actually care, or should we just drop out on first match?
> > I don't think think there is any obligation to catch broken tables.
> >
>
> Agree on the obligation part, but if things go wrong, this would be
> nice to know. I left it in as a dev warn once. If you think that's
> too strong - let me know.

I do think the driver should care about duplicate UID, but only if
"version, base, or length" mismatch. If the BIOS gives us ambiguous
answers about where the registers are located, the user should be
warned that the driver might be picking the "wrong" one by accident.
If they are identical, the BIOS is being repetitive, but no real harm
that the driver would care about. A dev_warn_once() sounds good as the
first duplicate should be sufficient to say something fishy is afoot,
but it's not an error. The warn_once will also re-trigger when / if
the module is reloaded.