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

[Jonathan Cameron]

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).


> +
> +    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',
>  ))