On Wed, 26 Jan 2022 12:32:01 +0000
Alex Bennée <alex.ben...@linaro.org> wrote:
> Jonathan Cameron <jonathan.came...@huawei.com> writes:
>
> > From: Ben Widawsky <ben.widaw...@intel.com>
> >
> > A CXL 2.0 component is any entity in the CXL topology. All components
> > have a analogous function in PCIe. Except for the CXL host bridge, all
> > have a PCIe config space that is accessible via the common PCIe
> > mechanisms. CXL components are enumerated via DVSEC fields in the
> > extended PCIe header space. CXL components will minimally implement some
> > subset of CXL.mem and CXL.cache registers defined in 8.2.5 of the CXL
> > 2.0 specification. Two headers and a utility library are introduced to
> > support the minimum functionality needed to enumerate components.
> >
> > The cxl_pci header manages bits associated with PCI, specifically the
> > DVSEC and related fields. The cxl_component.h variant has data
> > structures and APIs that are useful for drivers implementing any of the
> > CXL 2.0 components. The library takes care of making use of the DVSEC
> > bits and the CXL.[mem|cache] registers. Per spec, the registers are
> > little endian.
> >
> > None of the mechanisms required to enumerate a CXL capable hostbridge
> > are introduced at this point.
> >
> > Note that the CXL.mem and CXL.cache registers used are always 4B wide.
> > It's possible in the future that this constraint will not hold.
> >
> > Signed-off-by: Ben Widawsky <ben.widaw...@intel.com>
> > Signed-off-by: Jonathan Cameron <jonathan.came...@huawei.com>
> > ---
> > hw/Kconfig | 1 +
> > hw/cxl/Kconfig | 3 +
> > hw/cxl/cxl-component-utils.c | 212 +++++++++++++++++++++++++++++++++
> > hw/cxl/meson.build | 3 +
> > hw/meson.build | 1 +
> > include/hw/cxl/cxl.h | 16 +++
> > include/hw/cxl/cxl_component.h | 196 ++++++++++++++++++++++++++++++
> > include/hw/cxl/cxl_pci.h | 138 +++++++++++++++++++++
> > 8 files changed, 570 insertions(+)
> >
> > diff --git a/hw/Kconfig b/hw/Kconfig
> > index ad20cce0a9..50e0952889 100644
> > --- a/hw/Kconfig
> > +++ b/hw/Kconfig
> > @@ -6,6 +6,7 @@ source audio/Kconfig
> > source block/Kconfig
> > source char/Kconfig
> > source core/Kconfig
> > +source cxl/Kconfig
> > source display/Kconfig
> > source dma/Kconfig
> > source gpio/Kconfig
> > diff --git a/hw/cxl/Kconfig b/hw/cxl/Kconfig
> > new file mode 100644
> > index 0000000000..8e67519b16
> > --- /dev/null
> > +++ b/hw/cxl/Kconfig
> > @@ -0,0 +1,3 @@
> > +config CXL
> > + bool
> > + default y if PCI_EXPRESS
> > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > new file mode 100644
> > index 0000000000..5007b29ebb
> > --- /dev/null
> > +++ b/hw/cxl/cxl-component-utils.c
> > @@ -0,0 +1,212 @@
> > +/*
> > + * CXL Utility library for components
> > + *
> > + * Copyright(C) 2020 Intel Corporation.
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2. See the
> > + * COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/log.h"
> > +#include "hw/pci/pci.h"
> > +#include "hw/cxl/cxl.h"
> > +
> > +static uint64_t cxl_cache_mem_read_reg(void *opaque, hwaddr offset,
> > + unsigned size)
> > +{
> > + CXLComponentState *cxl_cstate = opaque;
> > + ComponentRegisters *cregs = &cxl_cstate->crb;
> > +
> > + assert(size == 4);
>
> You assert here but bellow:
>
> > +
> > +/*
> > + * 8.2.3
> > + * The access restrictions specified in Section 8.2.2 also apply to CXL
> > 2.0
> > + * Component Registers.
> > + *
> > + * 8.2.2
> > + * • A 32 bit register shall be accessed as a 4 Bytes quantity. Partial
> > + * reads are not permitted.
> > + * • A 64 bit register shall be accessed as a 8 Bytes quantity. Partial
> > + * reads are not permitted.
> > + *
> > + * As of the spec defined today, only 4 byte registers exist.
> > + */
> > +static const MemoryRegionOps cache_mem_ops = {
> > + .read = cxl_cache_mem_read_reg,
> > + .write = cxl_cache_mem_write_reg,
> > + .endianness = DEVICE_LITTLE_ENDIAN,
> > + .valid = {
> > + .min_access_size = 4,
> > + .max_access_size = 8,
> > + .unaligned = false,
> > + },
> > + .impl = {
> > + .min_access_size = 4,
> > + .max_access_size = 4,
> > + },
> > +};
>
> You have constrained the access to 4 so you will only see 4 bytes
> accesses. If it is valid for the guest to access 64bit words then it
> would be better to no-op that case and maybe LOG_UNIMP the fact.
>
Ugh. This looks suspiciously like a work around for a kernel bug (possibly
an accidental one).
If the comment above is correct (I've checked the spec and agree with it)..
https://elixir.bootlin.com/linux/v5.17-rc1/source/drivers/cxl/core/regs.c#L48
cap_array = readq(base + CXL_CM_CAP_HDR_OFFSET);
is undefined behavior as CAP_HDR is a 32bit register.
I guess the undefined choice on all the hardware people actually have
is to service the 8 byte read, but there is no reason to believe future
hardware will.
... and this is why we like having emulation in Qemu :)
+ eagle eyed reviewers!
Guess I'd better send a kernel fix sometime soon.
Jonathan
> Otherwise the rest looks ok to me:
>
> Reviewed-by: Alex Bennée <alex.ben...@linaro.org>
>
