Re: [PATCH 12/13] cxl/core/bus: Enumerate all HDM decoders

[Dan Williams <dan.j.williams@intel.com>]

On Fri, Sep 3, 2021 at 10:44 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Thu, 2 Sep 2021 12:50:16 -0700
> Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> > As of the CXL 2.0 specification, every port will have between 1 and 10
> > HDM decoders available in hardware. These exist in the endpoint, switch,
> > and top level hostbridges. HDM decoders are required for configuration
> > CXL regions, and therefore enumerating them is an important first step.
> >
> > As an example, the below has 4 decoders, a top level CFMWS decoder
> > (0.0), a single decoder in a single host bridge (1.0), and two devices
> > each with 1 decoder (2.0 and 3.0)
> >
> > ├── decoder0.0 -> ../../../devices/platform/ACPI0017:00/root0/decoder0.0
> > ├── decoder1.0 -> ../../../devices/platform/ACPI0017:00/root0/port1/decoder1.0
> > ├── decoder2.0 -> ../../../devices/platform/ACPI0017:00/root0/port1/devport2/decoder2.0
> > ├── decoder3.0 -> ../../../devices/platform/ACPI0017:00/root0/port1/devport3/decoder3.0
> >
> > Additionally, attributes are added for a port:
> >
> > /sys/bus/cxl/devices/port1
> > ├── active_decoders
> > ├── decoder_count
> > ├── decoder_enabled
> > ├── max_target_count
> > ...
> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
>
> Documentation/ABI/testing/sysfs-bus-cxl  needs updating.
>
> Various minor things inline.
>
>
> > ---
> >  drivers/cxl/core/bus.c | 161 ++++++++++++++++++++++++++++++++++++++++-
> >  drivers/cxl/cxl.h      |  54 ++++++++++++--
> >  2 files changed, 209 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> > index d056dbd794a4..b75e42965e89 100644
> > --- a/drivers/cxl/core/bus.c
> > +++ b/drivers/cxl/core/bus.c
[..]
> > +             ctrl = readl(hdm_decoder + CXL_HDM_DECODER0_CTRL_OFFSET(i));
> > +             cxld->decoder_enabled =
> > +                     !!FIELD_GET(CXL_HDM_DECODER0_CTRL_COMMITTED, ctrl);
> > +             /* If the decoder is already active, parse info */
> > +             if (cxld->decoder_enabled) {
> > +                     set_bit(i, port->used_decoders);
> > +                     iw = cxl_hdm_decoder_iw(ctrl);
> > +                     ig = cxl_hdm_decoder_ig(ctrl);
> > +                     if (FIELD_GET(CXL_HDM_DECODER0_CTRL_TYPE, ctrl) == 0)
> > +                             type = CXL_DECODER_ACCELERATOR;
> > +                     res.start = readl(hdm_decoder +
> > +                                       CXL_HDM_DECODER0_BASE_LOW_OFFSET(i));
> > +                     res.start |=
> > +                             (u64)readl(hdm_decoder +
> > +                                        CXL_HDM_DECODER0_BASE_HIGH_OFFSET(i))
> > +                             << 32;
>
> Perhaps worth thinking about exposing if the decoder is locked as well?
> Might be good to let userspace know that..

In fact...

What:           /sys/bus/cxl/devices/decoderX.Y/locked
Date:           June, 2021
KernelVersion:  v5.14
Contact:        linux-cxl@vger.kernel.org
Description:
                CXL HDM decoders have the capability to lock the configuration
                until the next device reset. For decoders of devtype
                "cxl_decoder_root" there is no standard facility to unlock them.
                For decoders of devtype "cxl_decoder_switch" a secondary bus
                reset, of the PCIe bridge that provides the bus for this
                decoders uport, unlocks / resets the decoder.


>
> > +             }
> > +
> > +             cxld->target_type = type;
> > +             cxld->res = res;
> > +             cxld->interleave_ways = iw;
> > +             cxld->interleave_granularity = ig;
> > +
> > +             rc = cxl_decoder_add(host, cxld, NULL);
> > +             if (rc) {
> > +                     dev_warn(host, "Failed to add decoder (%d)\n", rc);
> > +                     kfree(cxld);
> > +                     goto out;
> > +             }
> > +     }
> > +
> > +     /*
> > +      * Enable CXL.mem decoding via MMIO for endpoint devices
> > +      *
> > +      * TODO: If a memory device was configured to participate in a region by
> > +      * system firmware via DVSEC, this will break that region.

Spec reference please (8.2.5.12.2 CXL HDM Decoder Global Control
Register) . Also the spec calls this "HDM Base registers in DVSEC ID
0".

>
> That seems to me like fatal for this patch set until fixed.
> I'm not sure I understand why it will break a region though as I'd assume it
> would be already on?

The spec recommends that implementations set the HDM Base registers to
match the CXL HDM Decoder 0 value. The expectation is that platform
firmware is locking any HDM decoders for anything that it puts into
the platform firmware memory map. So I think a reasonable heuristic
here is decline to touch / support any device with HDM Base registers
pointing at a live entry in the system memory map as an indication
that the platform firmware is not prepared to interoperate with a CXL
2.0+ aware OS. CXL 2.0+ capable platform firmware should always use
HDM Decoders.

[..]
> >  /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */
> >  #define CXLDEV_CAP_ARRAY_OFFSET 0x0
> >  #define   CXLDEV_CAP_ARRAY_CAP_ID 0
> > @@ -188,6 +210,12 @@ enum cxl_decoder_type {
> >   */
> >  #define CXL_DECODER_MAX_INTERLEAVE 16
> >
> > +/*
> > + * Current specification goes up to 10 double that seems a reasonable
> > + * software max for the foreseeable future
>
> I'd stick to 10 until we have evidence otherwise... If you happen to
> have a mysterious strong reason to go with 20 though I'm not that fussed
> but I suspect others may have equally strong reasons to pick another number ;)

That's a good point. This is different from max interleave. More
decoders significantly increases the operational complexity of
managing the configuration. If someone thinks they need more than 10,
I would beg to differ, and it helps that Linux pushes back against
that decoder creep by default until someone can come up with an
exceedingly good reason.

>
> > + */
> > +#define CXL_DECODER_MAX_COUNT 20
> > +
> >  /**
> >   * struct cxl_decoder - CXL address range decode configuration
> >   * @dev: this decoder's device
> > @@ -197,6 +225,7 @@ enum cxl_decoder_type {
> >   * @interleave_granularity: data stride per dport
>
> Is it currently documented anywhere that ig is in 2**(ig) bytes?
> Might be worth adding if not.

This structure field would carry the nominal value, not the encoded one, right?

>
> >   * @target_type: accelerator vs expander (type2 vs type3) selector
> >   * @flags: memory type capabilities and locking
> > + * @decoder_enabled: Is this decoder currently decoding
> >   * @nr_targets: number of elements in @target
> >   * @target: active ordered target list in current decoder configuration
> >   */
> > @@ -208,6 +237,7 @@ struct cxl_decoder {
> >       int interleave_granularity;
> >       enum cxl_decoder_type target_type;
> >       unsigned long flags;
> > +     bool decoder_enabled;
> >       int nr_targets;
> >       struct cxl_dport *target[];
> >  };
> > @@ -255,6 +285,12 @@ struct cxl_walk_context {
> >   * @decoder_ida: allocator for decoder ids
> >   * @component_reg_phys: component register capability base address (optional)
> >   * @regs: Mapped version of @component_reg_phys
> > + * @used_decoders: Bitmap of currently active decoders for the port
> > + * @decoder_cap: Capabilities of all decoders contained by the port
> > + * @decoder_cap.count: Count of HDM decoders for the port
> > + * @decoder_cap.target_count: Max number of interleaved downstream ports
> > + * @decoder_cap.interleave11_8: Are address bits 11-8 available for interleave
> > + * @decoder_cap.interleave14_12: Are address bits 14-12 available for interleave
> >   */
> >  struct cxl_port {
> >       struct device dev;
> > @@ -264,6 +300,14 @@ struct cxl_port {
> >       struct ida decoder_ida;
> >       resource_size_t component_reg_phys;
> >       struct cxl_component_regs regs;
> > +
> > +     unsigned long *used_decoders;
>
> Given it's not huge use appropriate macro to allocate
> it directly here with the maximum of 10.
> DECLARE_BITMAP() in similar fashion to patch 19 in Dan's
> recent series.
>
> https://lore.kernel.org/all/162982122744.1124374.6742215706893563515.stgit@dwillia2-desk3.amr.corp.intel.com/
>
> (I'm liking lore having "all" now !)

Mmmm.

>
> > +     struct {
> > +             int count;
> > +             int target_count;
> > +             bool interleave11_8;
> > +             bool interleave14_12;
> > +     } decoder_cap;
> >  };
> >
> >  /**
>