Re: [PATCH v5 6/6] cxl/acpi: Introduce cxl_decoder objects

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

On Tue, Jun 8, 2021 at 6:06 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Sat, 5 Jun 2021 23:05:27 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > A cxl_decoder is a child of a cxl_port. It represents a hardware
> > decoder configuration of an upstream port to one or more of its
> > downstream ports. The decoder is either represented in standard HDM
> > decoder registers (see CXL 2.0 section 8.2.5.12 CXL HDM Decoder
> > Capability Structure), or it is a static decode configuration
> > communicated by platform firmware (see the CXL Early Discovery Table:
> > Fixed Memory Window Structure).
> >
> > The firmware described and hardware described decoders differ slightly
> > leading to 2 different sub-types of decoders, cxl_decoder_root and
> > cxl_decoder_switch. At the root level the decode capabilities restrict
> > what can be mapped beneath them. Mid-level switch decoders are
> > configured for either acclerator (type-2) or memory-expander (type-3)
> > operation, but they are otherwise agnostic to the type of memory
> > (volatile vs persistent) being mapped.
> >
> > Here is an example topology from a single-ported host-bridge environment
> > without CFMWS decodes enumerated.
> >
> > /sys/bus/cxl/devices/root0
> > ├── devtype
> > ├── dport0 -> ../LNXSYSTM:00/LNXSYBUS:00/ACPI0016:00
> > ├── port1
> > │   ├── decoder1.0
> > │   │   ├── devtype
> > │   │   ├── end
> > │   │   ├── locked
> > │   │   ├── start
> > │   │   ├── subsystem -> ../../../../bus/cxl
> > │   │   ├── target_list
> > │   │   ├── target_type
> > │   │   └── uevent
> > │   ├── devtype
> > │   ├── dport0 -> ../../pci0000:34/0000:34:00.0
> > │   ├── subsystem -> ../../../bus/cxl
> > │   ├── uevent
> > │   └── uport -> ../../LNXSYSTM:00/LNXSYBUS:00/ACPI0016:00
> > ├── subsystem -> ../../bus/cxl
> > ├── uevent
> > └── uport -> ../platform/ACPI0017:00
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> One trivial docs issue and a suggestion that -2 is a bit too magic.
> Otherwise looks good to me.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-cxl |   70 ++++++++
> >  drivers/cxl/acpi.c                      |   21 ++
> >  drivers/cxl/core.c                      |  265 +++++++++++++++++++++++++++++++
> >  drivers/cxl/cxl.h                       |   48 ++++++
> >  4 files changed, 403 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> > index 0cb31b7ad17b..f16f18e77578 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-cxl
> > +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> > @@ -48,3 +48,73 @@ Description:
> >               decode of CXL memory resources.  The 'Y' integer reflects the
> >               hardware port unique-id used in the hardware decoder target
> >               list.
> > +
> > +What:                /sys/bus/cxl/devices/decoderX.Y
> > +Date:                June, 2021
> > +KernelVersion:       v5.14
> > +Contact:     linux-cxl@vger.kernel.org
> > +Description:
> > +             CXL decoder objects are enumerated from either a platform
> > +             firmware description, or a CXL HDM decoder register set in a
> > +             PCIe device (see CXL 2.0 section 8.2.5.12 CXL HDM Decoder
> > +             Capability Structure). The 'X' in decoderX.Y represents the
> > +             cxl_port container of this decoder, and 'Y' represents the
> > +             instance id of a given decoder resource.
> > +
> > +What:                /sys/bus/cxl/devices/decoderX.Y/{start,end}
> > +Date:                June, 2021
> > +KernelVersion:       v5.14
> > +Contact:     linux-cxl@vger.kernel.org
> > +Description:
> > +             The 'start' and 'end' attributes together convey the physical
> > +             address base and last addressable byte of the decoder's decode
> > +             window. For decoders of devtype "cxl_decoder_root" the address
> > +             range is fixed. For decoders of devtype "cxl_decoder_switch" the
> > +             address is bounded by the decode range of the cxl_port ancestor
> > +             of the decoder's cxl_port, and dynamically updates based on the
> > +             active memory regions in that address space.
> > +
> > +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.
> > +
> > +What:                /sys/bus/cxl/devices/decoderX.Y/target_list
> > +Date:                June, 2021
> > +KernelVersion:       v5.14
> > +Contact:     linux-cxl@vger.kernel.org
> > +Description:
> > +             Display a comma separated list of the current decoder target
> > +             configuration. The list is ordered by the current configured
> > +             interleave order of the decoder's dport instances. Each entry in
> > +             the list is a dport id.
> > +
> > +What:                /sys/bus/cxl/devices/decoderX.Y/cap_{pmem,ram,type2,type3}
> > +Date:                June, 2021
> > +KernelVersion:       v5.14
> > +Contact:     linux-cxl@vger.kernel.org
> > +Description:
> > +             When a CXL decoder is of devtype "cxl_decoder_root", it
> > +             represents a fixed memory window identified by platform
> > +             firmware. A fixed window may only support a subset of memory
> > +             types. The 'cap_*' attributes indicate whether persistent
> > +             memory, volatile memory, accelerator memory, and / or expander
> > +             memory may be mapped behind this decoder's memory window.
> > +
> > +What:                /sys/bus/cxl/devices/decoderX.Y/target_type
> > +Date:                June, 2021
> > +KernelVersion:       v5.14
> > +Contact:     linux-cxl@vger.kernel.org
> > +Description:
> > +             When a CXL decoder is of devtype "cxl_decoder_switch", it can
> > +             optionally decode either accelerator memory (type-2) or expander
> > +             memory (type-3). The 'target_type' attribute indicates the
> > +             current setting which may dynamically change based on what
> > +             memory regions are activated in this decode hierarchy.
>
> Nice docs.
>
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index ec324bf063b8..6f203ba7fc1d 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -70,6 +70,7 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> >       struct device *host = root_port->dev.parent;
> >       struct acpi_pci_root *pci_root;
> >       struct cxl_walk_context ctx;
> > +     struct cxl_decoder *cxld;
> >       struct cxl_port *port;
> >
> >       if (!bridge)
> > @@ -94,7 +95,25 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> >
> >       if (ctx.count == 0)
> >               return -ENODEV;
> > -     return ctx.error;
> > +     if (ctx.error)
> > +             return ctx.error;
> > +
> > +     /* TODO: Scan CHBCR for HDM Decoder resources */
> > +
> > +     /*
> > +      * In the single-port host-bridge case there are no HDM decoders
> > +      * in the CHBCR and a 1:1 passthrough decode is implied.
> > +      */
> > +     if (ctx.count == 1) {
> > +             cxld = devm_cxl_add_decoder(host, port, 1, 0, -2, 1, 0,
>
> -2?  I'm guessing that has some special meaning and perhaps warrants
> a nice define to let us know what that is.

Um, yes, I think I went back and forth on whether this should be a
zero-length range starting at zero or a zero-length range starting at
UINT64_MAX, and I ended up botching it with a range of 0 to UINT64_MAX
- 1. I'll fix this up to provide a "passthrough" version of
devm_cxl_add_decoder() for the cases like this where a port does not
adjust the decode from the parent port down to the next level.

>
> Given this interface is a bit long perhaps even wroth a comment here on
> why the values are what they are?

The passthrough helper will address this.