| Message ID | 0246fe923945ba2b8d885f45279d7d3956c46950.1623705308.git.alison.schofield@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | CXL ACPI tables for object creation | expand |
[ 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 >
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
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.
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
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.
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(+)