On 21-01-06 13:28:05, Jonathan Cameron wrote:
> On Tue, 5 Jan 2021 08:52:56 -0800
> Ben Widawsky <ben.widaw...@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.widaw...@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',
> > ))
>
