On 21-01-06 17:06:41, Jonathan Cameron wrote:
> On Wed, 6 Jan 2021 08:49:48 -0800
> Ben Widawsky <[email protected]> wrote:
>
> > On 21-01-06 13:28:05, Jonathan Cameron wrote:
> > > On Tue, 5 Jan 2021 08:52:56 -0800
> > > Ben Widawsky <[email protected]> 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 <[email protected]>
> > > 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?
>
> I agree with intent but above it is setting limit to the top of
> the mailbox regs, not the memdev regs which I'd expect to see.
>
oops :D - you're correct
> >
> > > > +
> > > > + 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',
> > > > ))
> > >
>
