Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <cover.1623705308.git.alison.schofield@intel.com> <0246fe923945ba2b8d885f45279d7d3956c46950.1623705308.git.alison.schofield@intel.com>
In-Reply-To: <0246fe923945ba2b8d885f45279d7d3956c46950.1623705308.git.alison.schofield@intel.com>
From:   Dan Williams <dan.j.williams@intel.com>
Date:   Tue, 15 Jun 2021 11:48:43 -0700
Message-ID: <CAPcyv4houyD9PGG4PKiO7OiHkKEZD81=tVSzr_PTShHBzx956A@mail.gmail.com>
Subject: Re: [PATCH 2/2] cxl/acpi: Use the ACPI CFMWS to create static decoder objects
To:     Alison Schofield <alison.schofield@intel.com>
Cc:     Ben Widawsky <ben.widawsky@intel.com>,
        Ira Weiny <ira.weiny@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        linux-cxl@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux ACPI <linux-acpi@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

[ 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
>