Re: [PATCH v4 02/42] hw/cxl/component: Introduce CXL components (8.1.x, 8.2.5)

[Jonathan Cameron]

On Fri, 28 Jan 2022 14:22:51 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Wed, 26 Jan 2022 12:32:01 +0000
> Alex Bennée <alex.bennee@linaro.org> wrote:
> 
> > Jonathan Cameron <Jonathan.Cameron@huawei.com> writes:
> >   
> > > From: Ben Widawsky <ben.widawsky@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.widawsky@intel.com>
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@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.

However, after another read of the spec there are link related registers
which we currently don't implement emulation for but do advertise as
being present (as they are required) are 8 bytes long.

Linux doesn't use them yet, but it may come so I'll need to leave
max_access_size = 8 enabled and as you suggested LOG_UNIMP.

Probably makes sense to implement the link registers in a follow up
patch set along with (possibly) some kernel support to expose
the information available in those registers.

Jonathan

> 
> Jonathan
> 
> > Otherwise the rest looks ok to me:
> > 
> > Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> >   
>