Re: [RFC PATCH v3 04/31] hw/cxl/device: Implement the CAP array (8.2.8.1-2)

[Jonathan Cameron]

On Mon, 1 Feb 2021 16:59:21 -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.
> 
> Endianness and alignment are managed by softmmu memory core.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
A few trivials
> ---
>  hw/cxl/cxl-device-utils.c   | 105 ++++++++++++++++++++++++++++++++++++
>  hw/cxl/meson.build          |   1 +
>  include/hw/cxl/cxl_device.h |  27 +++++++++-
>  3 files changed, 132 insertions(+), 1 deletion(-)
>  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..bb15ad9a0f
> --- /dev/null
> +++ b/hw/cxl/cxl-device-utils.c
> @@ -0,0 +1,105 @@
> +/*
> + * 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"
> +
> +/*
> + * Device registers have no restrictions per the spec, and so fall back to 
> the
> + * default memory mapped register rules in 8.2:
> + *   Software shall use CXL.io Memory Read and Write to access memory mapped
> + *   register defined in this section. Unless otherwise specified, software
> + *   shall restrict the accesses width based on the following:
> + *   • A 32 bit register shall   be accessed as a 1 Byte, 2 Bytes or 4 Bytes

odd spacing

> + *     quantity.
> + *   • A 64 bit register shall be accessed as a 1 Byte, 2 Bytes, 4 Bytes or 8
> + *     Bytes
> + *   • The address shall be a multiple of the access width, e.g. when
> + *     accessing a register as a 4 Byte quantity, the address shall be
> + *     multiple of 4.
> + *   • The accesses shall map to contiguous bytes.If these rules are not
> + *     followed, the behavior is undefined
> + */
> +
> +static uint64_t caps_reg_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    CXLDeviceState *cxl_dstate = opaque;
> +
> +    return cxl_dstate->caps_reg_state32[offset / 4];
> +}
> +
> +static uint64_t dev_reg_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    return 0;
> +}
> +
> +static const MemoryRegionOps dev_ops = {
> +    .read = dev_reg_read,
> +    .write = NULL, /* status register is read only */
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 8,
> +        .unaligned = false,
> +    },
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 8,
> +    },
> +};
> +
> +static const MemoryRegionOps caps_ops = {
> +    .read = caps_reg_read,
> +    .write = NULL, /* caps registers are read only */
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 8,
> +        .unaligned = false,
> +    },
> +    .impl = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +};
> +
> +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_MMIO_SIZE));
> +
> +    memory_region_init_io(&cxl_dstate->caps, obj, &caps_ops, cxl_dstate,
> +                          "cap-array", CXL_DEVICE_REGISTERS_OFFSET - 0);

Specifying a size in terms of the offset of another region isn't exactly 
intuitive so perhaps a comment on why or better yet actually use a size
parameter covering what is there rather than simply the region below
the CXL_DEVICE_REGISTERS_OFFSET.


> +    memory_region_init_io(&cxl_dstate->device, obj, &dev_ops, cxl_dstate,
> +                          "device-status", CXL_DEVICE_REGISTERS_LENGTH);
> +
> +    memory_region_add_subregion(&cxl_dstate->device_registers, 0,
> +                                &cxl_dstate->caps);
> +    memory_region_add_subregion(&cxl_dstate->device_registers,
> +                                CXL_DEVICE_REGISTERS_OFFSET,
> +                                &cxl_dstate->device);
> +}
> +
> +static void device_reg_init_common(CXLDeviceState *cxl_dstate) { }
> +
> +void cxl_device_register_init_common(CXLDeviceState *cxl_dstate)
> +{
> +    uint32_t *cap_hdrs = cxl_dstate->caps_reg_state32;
> +    const int cap_count = 1;
> +
> +    /* 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);
> +
> +    cxl_device_cap_init(cxl_dstate, DEVICE, 1);
> +    device_reg_init_common(cxl_dstate);
> +}
> 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',
>  ))
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index a85f250503..f3bcf19410 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -58,6 +58,8 @@
>  #define CXL_DEVICE_CAP_HDR1_OFFSET 0x10 /* Figure 138 */
>  #define CXL_DEVICE_CAP_REG_SIZE 0x10 /* 8.2.8.2 */
>  #define CXL_DEVICE_CAPS_MAX 4 /* 8.2.8.2.1 + 8.2.8.5 */
> +#define CXL_CAPS_SIZE \
> +    (CXL_DEVICE_CAP_REG_SIZE * CXL_DEVICE_CAPS_MAX + 1) /* +1 for header */
>  
>  #define CXL_DEVICE_REGISTERS_OFFSET 0x80 /* Read comment above */
>  #define CXL_DEVICE_REGISTERS_LENGTH 0x8 /* 8.2.8.3.1 */
> @@ -70,11 +72,18 @@
>  #define CXL_MAILBOX_REGISTERS_LENGTH \
>      (CXL_MAILBOX_REGISTERS_SIZE + CXL_MAILBOX_MAX_PAYLOAD_SIZE)
>  
> +#define CXL_MMIO_SIZE                                       \
> +    CXL_DEVICE_CAP_REG_SIZE + CXL_DEVICE_REGISTERS_LENGTH + \
> +        CXL_MAILBOX_REGISTERS_LENGTH
> +
>  typedef struct cxl_device_state {
>      MemoryRegion device_registers;
>  
>      /* mmio for device capabilities array - 8.2.8.2 */
> -    MemoryRegion caps;
> +    struct {
> +        MemoryRegion caps;
> +        uint32_t caps_reg_state32[CXL_CAPS_SIZE / 4];
> +    };

With this unnamed,w hat is the benefit of having these two in a
struct?  The naming makes it clear they are related anyway.

>  
>      /* mmio for the device status registers 8.2.8.3 */
>      MemoryRegion device;
> @@ -126,6 +135,22 @@ CXL_DEVICE_CAPABILITY_HEADER_REGISTER(DEVICE, 
> CXL_DEVICE_CAP_HDR1_OFFSET)
>  CXL_DEVICE_CAPABILITY_HEADER_REGISTER(MAILBOX, CXL_DEVICE_CAP_HDR1_OFFSET + \
>                                                 CXL_DEVICE_CAP_REG_SIZE)
>  
> +#define cxl_device_cap_init(dstate, reg, cap_id)                             
>       \
> +    do {                                                                     
>       \
> +        uint32_t *cap_hdrs = dstate->caps_reg_state32;                       
>       \
> +        int which = R_CXL_DEV_##reg##_CAP_HDR0;                              
>       \
> +        cap_hdrs[which] =                                                    
>       \
> +            FIELD_DP32(cap_hdrs[which], CXL_DEV_##reg##_CAP_HDR0, CAP_ID, 
> cap_id); \
> +        cap_hdrs[which] = FIELD_DP32(                                        
>       \
> +            cap_hdrs[which], CXL_DEV_##reg##_CAP_HDR0, CAP_VERSION, 1);      
>       \
> +        cap_hdrs[which + 1] =                                                
>       \
> +            FIELD_DP32(cap_hdrs[which + 1], CXL_DEV_##reg##_CAP_HDR1,        
>       \
> +                       CAP_OFFSET, CXL_##reg##_REGISTERS_OFFSET);            
>       \
> +        cap_hdrs[which + 2] =                                                
>       \
> +            FIELD_DP32(cap_hdrs[which + 2], CXL_DEV_##reg##_CAP_HDR2,        
>       \
> +                       CAP_LENGTH, CXL_##reg##_REGISTERS_LENGTH);            
>       \
> +    } while (0)
> +
>  REG32(CXL_DEV_MAILBOX_CAP, 0)
>      FIELD(CXL_DEV_MAILBOX_CAP, PAYLOAD_SIZE, 0, 5)
>      FIELD(CXL_DEV_MAILBOX_CAP, INT_CAP, 5, 1)