* [PATCH 0/2] CXL ACPI tables for object creation @ 2021-06-14 22:52 Alison Schofield 2021-06-14 22:52 ` [PATCH 1/2] cxl/acpi: Add the Host Bridge base address to CXL port objects Alison Schofield 2021-06-14 22:52 ` [PATCH 2/2] cxl/acpi: Use the ACPI CFMWS to create static decoder objects Alison Schofield 0 siblings, 2 replies; 7+ messages in thread From: Alison Schofield @ 2021-06-14 22:52 UTC (permalink / raw) To: Ben Widawsky, Dan Williams, Ira Weiny, Alison Schofield, Vishal Verma Cc: linux-cxl, linux-kernel 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 | 211 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 205 insertions(+), 6 deletions(-) base-commit: 195d5a63f0f9a47aa128a5050fe4ad7f5d27a901 -- 2.26.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] cxl/acpi: Add the Host Bridge base address to CXL port objects 2021-06-14 22:52 [PATCH 0/2] CXL ACPI tables for object creation Alison Schofield @ 2021-06-14 22:52 ` Alison Schofield 2021-06-14 22:52 ` [PATCH 2/2] cxl/acpi: Use the ACPI CFMWS to create static decoder objects Alison Schofield 1 sibling, 0 replies; 7+ messages in thread From: Alison Schofield @ 2021-06-14 22:52 UTC (permalink / raw) To: Ben Widawsky, Dan Williams, Ira Weiny, Alison Schofield, Vishal Verma Cc: linux-cxl, linux-kernel 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..16f60bc6801f 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; } +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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] cxl/acpi: Use the ACPI CFMWS to create static decoder objects 2021-06-14 22:52 [PATCH 0/2] CXL ACPI tables for object creation Alison Schofield 2021-06-14 22:52 ` [PATCH 1/2] cxl/acpi: Add the Host Bridge base address to CXL port objects Alison Schofield @ 2021-06-14 22:52 ` Alison Schofield 2021-06-15 18:48 ` Dan Williams 1 sibling, 1 reply; 7+ messages in thread From: Alison Schofield @ 2021-06-14 22:52 UTC (permalink / raw) To: Ben Widawsky, Dan Williams, Ira Weiny, Alison Schofield, Vishal Verma Cc: linux-cxl, linux-kernel 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 | 106 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c index 16f60bc6801f..ac4b3e37e294 100644 --- a/drivers/cxl/acpi.c +++ b/drivers/cxl/acpi.c @@ -8,8 +8,112 @@ #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) + +/* + * CFMWS Restrictions mapped to CXL Decoder Flags + * Restrictions defined in CXL 2.0 ECN CEDT CFMWS + * Decoder Flags defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register + */ +#define CFMWS_TO_DECODE_TYPE2(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE2) << 2) +#define CFMWS_TO_DECODE_TYPE3(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE3) << 2) +#define CFMWS_TO_DECODE_RAM(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE) >> 2) +#define CFMWS_TO_DECODE_PMEM(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_PMEM) >> 2) +#define CFMWS_TO_DECODE_FIXED(x) (x & ACPI_CEDT_CFMWS_RESTRICT_FIXED) + +#define CFMWS_TO_DECODER_FLAGS(x) (CFMWS_TO_DECODE_TYPE2(x) | \ + CFMWS_TO_DECODE_TYPE3(x) | \ + CFMWS_TO_DECODE_RAM(x) | \ + CFMWS_TO_DECODE_PMEM(x) | \ + CFMWS_TO_DECODE_FIXED(x)) + static struct acpi_table_header *cedt_table; +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) { + 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\n"); + else + dev_dbg(dev, "add: %s\n", dev_name(&cxld->dev)); + + 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 +355,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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] cxl/acpi: Use the ACPI CFMWS to create static decoder objects 2021-06-14 22:52 ` [PATCH 2/2] cxl/acpi: Use the ACPI CFMWS to create static decoder objects Alison Schofield @ 2021-06-15 18:48 ` Dan Williams 2021-06-15 21:05 ` Alison Schofield 0 siblings, 1 reply; 7+ messages in thread From: Dan Williams @ 2021-06-15 18:48 UTC (permalink / raw) To: Alison Schofield Cc: Ben Widawsky, Ira Weiny, Vishal Verma, linux-cxl, Linux Kernel Mailing List, Linux ACPI [ add linu-acpi for variable length array question below ] On Mon, Jun 14, 2021 at 3:57 PM 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> > --- > drivers/cxl/acpi.c | 106 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 106 insertions(+) > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index 16f60bc6801f..ac4b3e37e294 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -8,8 +8,112 @@ > #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) > + > +/* > + * CFMWS Restrictions mapped to CXL Decoder Flags > + * Restrictions defined in CXL 2.0 ECN CEDT CFMWS > + * Decoder Flags defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register > + */ > +#define CFMWS_TO_DECODE_TYPE2(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE2) << 2) > +#define CFMWS_TO_DECODE_TYPE3(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE3) << 2) > +#define CFMWS_TO_DECODE_RAM(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE) >> 2) > +#define CFMWS_TO_DECODE_PMEM(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_PMEM) >> 2) > +#define CFMWS_TO_DECODE_FIXED(x) (x & ACPI_CEDT_CFMWS_RESTRICT_FIXED) > + > +#define CFMWS_TO_DECODER_FLAGS(x) (CFMWS_TO_DECODE_TYPE2(x) | \ > + CFMWS_TO_DECODE_TYPE3(x) | \ > + CFMWS_TO_DECODE_RAM(x) | \ > + CFMWS_TO_DECODE_PMEM(x) | \ > + CFMWS_TO_DECODE_FIXED(x)) I don't understand the approach taken above. It seems to assume that the CXL_DECODER_F_* values are fixed. Those flag values are arbitrary and mutable. There is no guarantee that today's CXL_DECODER_F_* values match tomorrow's so I'd rather not have 2 places to check when / if that happens. > + > static struct acpi_table_header *cedt_table; > > +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"); I expect the user will want to know more about which decode range is not being registered. So, for all of these error messages please print out the entry, at least the base and end address. > + 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)); Oh interesting, I was about to say "unfortunately struct_size() can't be used", becuase I thought ACPICA could not support variable length array. It turns out 'struct acpi_cedt_cfmws' got away with this. Not sure if that is going to change in the future, but it's a positive sign otherwise. > + > + 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) { > + 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\n"); This would be another place to print out the CFMWS entry so that the user has some record of which address range is offline. > + else > + dev_dbg(dev, "add: %s\n", dev_name(&cxld->dev)); > + > + 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 +355,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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] cxl/acpi: Use the ACPI CFMWS to create static decoder objects 2021-06-15 18:48 ` Dan Williams @ 2021-06-15 21:05 ` Alison Schofield 2021-06-15 22:01 ` Dan Williams 0 siblings, 1 reply; 7+ messages in thread From: Alison Schofield @ 2021-06-15 21:05 UTC (permalink / raw) To: Dan Williams Cc: Ben Widawsky, Ira Weiny, Vishal Verma, linux-cxl, Linux Kernel Mailing List, Linux ACPI Thanks for the review Dan... On Tue, Jun 15, 2021 at 11:48:43AM -0700, Dan Williams wrote: > [ add linu-acpi for variable length array question below ] > > > On Mon, Jun 14, 2021 at 3:57 PM 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> > > --- > > drivers/cxl/acpi.c | 106 +++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 106 insertions(+) > > > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > > index 16f60bc6801f..ac4b3e37e294 100644 > > --- a/drivers/cxl/acpi.c > > +++ b/drivers/cxl/acpi.c > > @@ -8,8 +8,112 @@ > > #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) > > + > > +/* > > + * CFMWS Restrictions mapped to CXL Decoder Flags > > + * Restrictions defined in CXL 2.0 ECN CEDT CFMWS > > + * Decoder Flags defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register > > + */ > > +#define CFMWS_TO_DECODE_TYPE2(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE2) << 2) > > +#define CFMWS_TO_DECODE_TYPE3(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE3) << 2) > > +#define CFMWS_TO_DECODE_RAM(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE) >> 2) > > +#define CFMWS_TO_DECODE_PMEM(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_PMEM) >> 2) > > +#define CFMWS_TO_DECODE_FIXED(x) (x & ACPI_CEDT_CFMWS_RESTRICT_FIXED) > > + > > +#define CFMWS_TO_DECODER_FLAGS(x) (CFMWS_TO_DECODE_TYPE2(x) | \ > > + CFMWS_TO_DECODE_TYPE3(x) | \ > > + CFMWS_TO_DECODE_RAM(x) | \ > > + CFMWS_TO_DECODE_PMEM(x) | \ > > + CFMWS_TO_DECODE_FIXED(x)) > > I don't understand the approach taken above. It seems to assume that > the CXL_DECODER_F_* values are fixed. Those flag values are arbitrary > and mutable. There is no guarantee that today's CXL_DECODER_F_* values > match tomorrow's so I'd rather not have 2 places to check when / if > that happens. > Here's my next take - making the handling resilient. Not so sure on gracefulness. Open for suggestions. -#define CFMWS_TO_DECODE_TYPE2(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE2) << 2) -#define CFMWS_TO_DECODE_TYPE3(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE3) << 2) -#define CFMWS_TO_DECODE_RAM(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE) >> 2) -#define CFMWS_TO_DECODE_PMEM(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_PMEM) >> 2) -#define CFMWS_TO_DECODE_FIXED(x) (x & ACPI_CEDT_CFMWS_RESTRICT_FIXED) - -#define CFMWS_TO_DECODER_FLAGS(x) (CFMWS_TO_DECODE_TYPE2(x) | \ - CFMWS_TO_DECODE_TYPE3(x) | \ - CFMWS_TO_DECODE_RAM(x) | \ - CFMWS_TO_DECODE_PMEM(x) | \ - CFMWS_TO_DECODE_FIXED(x)) +#define FLAG_TYPE2(x) \ + ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE2) ? CXL_DECODER_F_TYPE2 : 0) +#define FLAG_TYPE3(x) \ + ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE3) ? CXL_DECODER_F_TYPE3 : 0) +#define FLAG_RAM(x) \ + ((x & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE) ? CXL_DECODER_F_RAM : 0) +#define FLAG_PMEM(x) \ + ((x & ACPI_CEDT_CFMWS_RESTRICT_PMEM) ? CXL_DECODER_F_PMEM : 0) +#define FLAG_FIXED(x) \ + ((x & ACPI_CEDT_CFMWS_RESTRICT_FIXED) ? CXL_DECODER_F_LOCK : 0) + +#define CFMWS_TO_DECODER_FLAGS(x) (FLAG_TYPE2(x) | FLAG_TYPE3(x) | \ + FLAG_RAM(x) | FLAG_PMEM(x)| FLAG_FIXED(x)) + > > + > > static struct acpi_table_header *cedt_table; > > > > +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"); > > I expect the user will want to know more about which decode range is > not being registered. So, for all of these error messages please print > out the entry, at least the base and end address. > Will do. > > + 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)); > > Oh interesting, I was about to say "unfortunately struct_size() can't > be used", becuase I thought ACPICA could not support variable length > array. It turns out 'struct acpi_cedt_cfmws' got away with this. Not > sure if that is going to change in the future, but it's a positive > sign otherwise. > Noted. Will watch. > > + snip > > + > > + 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\n"); > > This would be another place to print out the CFMWS entry so that the > user has some record of which address range is offline. > Will do. snip ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] cxl/acpi: Use the ACPI CFMWS to create static decoder objects 2021-06-15 21:05 ` Alison Schofield @ 2021-06-15 22:01 ` Dan Williams 2021-06-16 0:01 ` Alison Schofield 0 siblings, 1 reply; 7+ messages in thread From: Dan Williams @ 2021-06-15 22:01 UTC (permalink / raw) To: Alison Schofield Cc: Ben Widawsky, Ira Weiny, Vishal Verma, linux-cxl, Linux Kernel Mailing List, Linux ACPI On Tue, Jun 15, 2021 at 2:09 PM Alison Schofield <alison.schofield@intel.com> wrote: > > Thanks for the review Dan... > > On Tue, Jun 15, 2021 at 11:48:43AM -0700, Dan Williams wrote: > > [ add linu-acpi for variable length array question below ] > > > > > > On Mon, Jun 14, 2021 at 3:57 PM 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> > > > --- > > > drivers/cxl/acpi.c | 106 +++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 106 insertions(+) > > > > > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > > > index 16f60bc6801f..ac4b3e37e294 100644 > > > --- a/drivers/cxl/acpi.c > > > +++ b/drivers/cxl/acpi.c > > > @@ -8,8 +8,112 @@ > > > #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) > > > + > > > +/* > > > + * CFMWS Restrictions mapped to CXL Decoder Flags > > > + * Restrictions defined in CXL 2.0 ECN CEDT CFMWS > > > + * Decoder Flags defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register > > > + */ > > > +#define CFMWS_TO_DECODE_TYPE2(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE2) << 2) > > > +#define CFMWS_TO_DECODE_TYPE3(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE3) << 2) > > > +#define CFMWS_TO_DECODE_RAM(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE) >> 2) > > > +#define CFMWS_TO_DECODE_PMEM(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_PMEM) >> 2) > > > +#define CFMWS_TO_DECODE_FIXED(x) (x & ACPI_CEDT_CFMWS_RESTRICT_FIXED) > > > + > > > +#define CFMWS_TO_DECODER_FLAGS(x) (CFMWS_TO_DECODE_TYPE2(x) | \ > > > + CFMWS_TO_DECODE_TYPE3(x) | \ > > > + CFMWS_TO_DECODE_RAM(x) | \ > > > + CFMWS_TO_DECODE_PMEM(x) | \ > > > + CFMWS_TO_DECODE_FIXED(x)) > > > > I don't understand the approach taken above. It seems to assume that > > the CXL_DECODER_F_* values are fixed. Those flag values are arbitrary > > and mutable. There is no guarantee that today's CXL_DECODER_F_* values > > match tomorrow's so I'd rather not have 2 places to check when / if > > that happens. > > > > Here's my next take - making the handling resilient. > Not so sure on gracefulness. Open for suggestions. > > -#define CFMWS_TO_DECODE_TYPE2(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE2) << 2) > -#define CFMWS_TO_DECODE_TYPE3(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE3) << 2) > -#define CFMWS_TO_DECODE_RAM(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE) >> 2) > -#define CFMWS_TO_DECODE_PMEM(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_PMEM) >> 2) > -#define CFMWS_TO_DECODE_FIXED(x) (x & ACPI_CEDT_CFMWS_RESTRICT_FIXED) > - > -#define CFMWS_TO_DECODER_FLAGS(x) (CFMWS_TO_DECODE_TYPE2(x) | \ > - CFMWS_TO_DECODE_TYPE3(x) | \ > - CFMWS_TO_DECODE_RAM(x) | \ > - CFMWS_TO_DECODE_PMEM(x) | \ > - CFMWS_TO_DECODE_FIXED(x)) > +#define FLAG_TYPE2(x) \ > + ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE2) ? CXL_DECODER_F_TYPE2 : 0) > +#define FLAG_TYPE3(x) \ > + ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE3) ? CXL_DECODER_F_TYPE3 : 0) > +#define FLAG_RAM(x) \ > + ((x & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE) ? CXL_DECODER_F_RAM : 0) > +#define FLAG_PMEM(x) \ > + ((x & ACPI_CEDT_CFMWS_RESTRICT_PMEM) ? CXL_DECODER_F_PMEM : 0) > +#define FLAG_FIXED(x) \ > + ((x & ACPI_CEDT_CFMWS_RESTRICT_FIXED) ? CXL_DECODER_F_LOCK : 0) > + > +#define CFMWS_TO_DECODER_FLAGS(x) (FLAG_TYPE2(x) | FLAG_TYPE3(x) | \ > + FLAG_RAM(x) | FLAG_PMEM(x)| FLAG_FIXED(x)) Hmm, why the macros? Just make CFMWS_TO_DECODER_FLAGS a proper function. if (cfmws->restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE2) flags |= CXL_DECODER_F_TYPE2; ...etc Unless you foresee where macros were going to be reused somewhere else I would just as soon open code them like above. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] cxl/acpi: Use the ACPI CFMWS to create static decoder objects 2021-06-15 22:01 ` Dan Williams @ 2021-06-16 0:01 ` Alison Schofield 0 siblings, 0 replies; 7+ messages in thread From: Alison Schofield @ 2021-06-16 0:01 UTC (permalink / raw) To: Dan Williams Cc: Ben Widawsky, Ira Weiny, Vishal Verma, linux-cxl, Linux Kernel Mailing List, Linux ACPI On Tue, Jun 15, 2021 at 03:01:06PM -0700, Dan Williams wrote: > On Tue, Jun 15, 2021 at 2:09 PM Alison Schofield > <alison.schofield@intel.com> wrote: > > > > Thanks for the review Dan... > > > > On Tue, Jun 15, 2021 at 11:48:43AM -0700, Dan Williams wrote: > > > [ add linu-acpi for variable length array question below ] > > > > > > > > > On Mon, Jun 14, 2021 at 3:57 PM 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> > > > > --- > > > > drivers/cxl/acpi.c | 106 +++++++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 106 insertions(+) > > > > > > > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > > > > index 16f60bc6801f..ac4b3e37e294 100644 > > > > --- a/drivers/cxl/acpi.c > > > > +++ b/drivers/cxl/acpi.c > > > > @@ -8,8 +8,112 @@ > > > > #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) > > > > + > > > > +/* > > > > + * CFMWS Restrictions mapped to CXL Decoder Flags > > > > + * Restrictions defined in CXL 2.0 ECN CEDT CFMWS > > > > + * Decoder Flags defined in CXL 2.0 8.2.5.12.7 HDM Decoder Control Register > > > > + */ > > > > +#define CFMWS_TO_DECODE_TYPE2(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE2) << 2) > > > > +#define CFMWS_TO_DECODE_TYPE3(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE3) << 2) > > > > +#define CFMWS_TO_DECODE_RAM(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE) >> 2) > > > > +#define CFMWS_TO_DECODE_PMEM(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_PMEM) >> 2) > > > > +#define CFMWS_TO_DECODE_FIXED(x) (x & ACPI_CEDT_CFMWS_RESTRICT_FIXED) > > > > + > > > > +#define CFMWS_TO_DECODER_FLAGS(x) (CFMWS_TO_DECODE_TYPE2(x) | \ > > > > + CFMWS_TO_DECODE_TYPE3(x) | \ > > > > + CFMWS_TO_DECODE_RAM(x) | \ > > > > + CFMWS_TO_DECODE_PMEM(x) | \ > > > > + CFMWS_TO_DECODE_FIXED(x)) > > > > > > I don't understand the approach taken above. It seems to assume that > > > the CXL_DECODER_F_* values are fixed. Those flag values are arbitrary > > > and mutable. There is no guarantee that today's CXL_DECODER_F_* values > > > match tomorrow's so I'd rather not have 2 places to check when / if > > > that happens. > > > > > > > Here's my next take - making the handling resilient. > > Not so sure on gracefulness. Open for suggestions. > > > > -#define CFMWS_TO_DECODE_TYPE2(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE2) << 2) > > -#define CFMWS_TO_DECODE_TYPE3(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE3) << 2) > > -#define CFMWS_TO_DECODE_RAM(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE) >> 2) > > -#define CFMWS_TO_DECODE_PMEM(x) ((x & ACPI_CEDT_CFMWS_RESTRICT_PMEM) >> 2) > > -#define CFMWS_TO_DECODE_FIXED(x) (x & ACPI_CEDT_CFMWS_RESTRICT_FIXED) > > - > > -#define CFMWS_TO_DECODER_FLAGS(x) (CFMWS_TO_DECODE_TYPE2(x) | \ > > - CFMWS_TO_DECODE_TYPE3(x) | \ > > - CFMWS_TO_DECODE_RAM(x) | \ > > - CFMWS_TO_DECODE_PMEM(x) | \ > > - CFMWS_TO_DECODE_FIXED(x)) > > +#define FLAG_TYPE2(x) \ > > + ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE2) ? CXL_DECODER_F_TYPE2 : 0) > > +#define FLAG_TYPE3(x) \ > > + ((x & ACPI_CEDT_CFMWS_RESTRICT_TYPE3) ? CXL_DECODER_F_TYPE3 : 0) > > +#define FLAG_RAM(x) \ > > + ((x & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE) ? CXL_DECODER_F_RAM : 0) > > +#define FLAG_PMEM(x) \ > > + ((x & ACPI_CEDT_CFMWS_RESTRICT_PMEM) ? CXL_DECODER_F_PMEM : 0) > > +#define FLAG_FIXED(x) \ > > + ((x & ACPI_CEDT_CFMWS_RESTRICT_FIXED) ? CXL_DECODER_F_LOCK : 0) > > + > > +#define CFMWS_TO_DECODER_FLAGS(x) (FLAG_TYPE2(x) | FLAG_TYPE3(x) | \ > > + FLAG_RAM(x) | FLAG_PMEM(x)| FLAG_FIXED(x)) > > Hmm, why the macros? Just make CFMWS_TO_DECODER_FLAGS a proper function. > > if (cfmws->restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE2) > flags |= CXL_DECODER_F_TYPE2; > > ...etc > > Unless you foresee where macros were going to be reused somewhere else > I would just as soon open code them like above. Open code it is! Thanks ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-06-16 0:05 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-06-14 22:52 [PATCH 0/2] CXL ACPI tables for object creation Alison Schofield 2021-06-14 22:52 ` [PATCH 1/2] cxl/acpi: Add the Host Bridge base address to CXL port objects Alison Schofield 2021-06-14 22:52 ` [PATCH 2/2] cxl/acpi: Use the ACPI CFMWS to create static decoder objects Alison Schofield 2021-06-15 18:48 ` Dan Williams 2021-06-15 21:05 ` Alison Schofield 2021-06-15 22:01 ` Dan Williams 2021-06-16 0:01 ` Alison Schofield
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).