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

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

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.