Re: [RFC PATCH v2 05/32] hw/cxl/device: Implement the CAP array (8.2.8.1-2)

[Ben Widawsky]

On 21-01-06 13:28:05, Jonathan Cameron wrote:
> On Tue, 5 Jan 2021 08:52:56 -0800
> Ben Widawsky <ben.widawsky@intel.com> wrote:
> 
> > This implements all device MMIO up to the first capability .That
> > includes the CXL Device Capabilities Array Register, as well as all of
> > the CXL Device Capability Header Registers. The latter are filled in as
> > they are implemented in the following patches.
> > 
> > v2: Break out register alignment checks (Jonathan)
> > 
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> Hi Ben,
> 
> One buglet / inconsistency inline that I spotted whilst chasing that issue
> with size of reads.
> 
> Will get to a full review after messing around ('testing') this a bit more ;)
> 
> Jonathan
> 
> > ---
> >  hw/cxl/cxl-device-utils.c | 72 +++++++++++++++++++++++++++++++++++++++
> >  hw/cxl/meson.build        |  1 +
> >  2 files changed, 73 insertions(+)
> >  create mode 100644 hw/cxl/cxl-device-utils.c
> > 
> > diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
> > new file mode 100644
> > index 0000000000..d1b1371e66
> > --- /dev/null
> > +++ b/hw/cxl/cxl-device-utils.c
> > @@ -0,0 +1,72 @@
> > +/*
> > + * CXL Utility library for devices
> > + *
> > + * 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/cxl/cxl.h"
> > +
> > +static int cxl_device_check_register_alignment(hwaddr offset, unsigned 
> > size)
> > +{
> > +    if (unlikely(offset & (size - 1))) {
> > +        return 1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static uint64_t caps_reg_read(void *opaque, hwaddr offset, unsigned size)
> > +{
> > +    CXLDeviceState *cxl_dstate = opaque;
> > +
> > +    if (cxl_device_check_register_alignment(offset, size)) {
> > +        qemu_log_mask(LOG_UNIMP, "Unaligned register read\n");
> > +        return 0;
> > +    }
> > +
> > +    return ldn_le_p(cxl_dstate->caps_reg_state + offset, size);
> > +}
> > +
> > +static const MemoryRegionOps caps_ops = {
> > +    .read = caps_reg_read,
> > +    .write = NULL,
> > +    .endianness = DEVICE_LITTLE_ENDIAN,
> > +    .valid = {
> > +        .min_access_size = 4,
> > +        .max_access_size = 8,
> > +    },
> > +    .impl = {
> > +        .min_access_size = 4,
> > +        .max_access_size = 8,
> > +    },
> > +};
> > +
> > +void cxl_device_register_block_init(Object *obj, CXLDeviceState 
> > *cxl_dstate)
> > +{
> > +    /* This will be a BAR, so needs to be rounded up to pow2 for PCI spec 
> > */
> > +    memory_region_init(
> > +        &cxl_dstate->device_registers, obj, "device-registers",
> > +        pow2ceil(CXL_MAILBOX_REGISTERS_LENGTH + 
> > CXL_MAILBOX_REGISTERS_OFFSET));
> 
> I can see why you jumped directly to sizing this for the whole region, but 
> the snag
> is that means I think you missed the fact that patch 8 adds a region after 
> the end
> of the mailbox.   Doesn't result in an actual bug because the ceil above takes
> you way past the space needed though (the memory device region is only 8 
> bytes long).
> 
> 

Maybe I misunderstand, but this is the intended behavior.
cxl_dstate->device_registers is the MemoryRegion container for all the
subregions that are the actual device MMIO.

                     +------------------+
                  ^  |                  |
                  |  |      unused      |
                  |  --------------------
                  |  |   memdev regs    |
                  |  --------------------
                  |  |                  |
                  |  | +--------------+ |
 cxl_dstate->     |  | |              | |
 device_registers |  | |              | |
                  |  | |payload       | |
                  |  | |(2k currently)| |
                  |  | |              | |
                  |  | |              | |
                  |  | +--------------+ |
                  |  |   mailbox regs   |
                  |  --------------------
                  |  |    device regs   |
                  v  --------------------
                     |     caps regs    |
         BAR ------> +------------------+


Perhaps I should add this as a comment in the code?

> > +
> > +    memory_region_init_io(&cxl_dstate->caps, obj, &caps_ops, cxl_dstate,
> > +                          "cap-array", CXL_DEVICE_REGISTERS_OFFSET - 0);
> > +
> > +    memory_region_add_subregion(&cxl_dstate->device_registers, 0,
> > +                                &cxl_dstate->caps);
> > +}
> > +
> > +void cxl_device_register_init_common(CXLDeviceState *cxl_dstate)
> > +{
> > +    uint32_t *cap_hdrs = cxl_dstate->caps_reg_state32;
> > +    const int cap_count = 0;
> > +
> > +    /* CXL Device Capabilities Array Register */
> > +    ARRAY_FIELD_DP32(cap_hdrs, CXL_DEV_CAP_ARRAY, CAP_ID, 0);
> > +    ARRAY_FIELD_DP32(cap_hdrs, CXL_DEV_CAP_ARRAY, CAP_VERSION, 1);
> > +    ARRAY_FIELD_DP32(cap_hdrs, CXL_DEV_CAP_ARRAY2, CAP_COUNT, cap_count);
> > +}
> > diff --git a/hw/cxl/meson.build b/hw/cxl/meson.build
> > index 00c3876a0f..47154d6850 100644
> > --- a/hw/cxl/meson.build
> > +++ b/hw/cxl/meson.build
> > @@ -1,3 +1,4 @@
> >  softmmu_ss.add(when: 'CONFIG_CXL', if_true: files(
> >    'cxl-component-utils.c',
> > +  'cxl-device-utils.c',
> >  ))
>