Re: [RFC PATCH 08/25] hw/cxl/device: Add memory devices (8.2.8.5)

[Jonathan Cameron]

On Tue, 10 Nov 2020 21:47:07 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> Memory devices implement extra capabilities on top of CXL devices. This
> adds support for that.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
>  hw/cxl/cxl-device-utils.c   | 48 ++++++++++++++++++++++++++++++++++++-
>  hw/cxl/cxl-mailbox-utils.c  | 48 ++++++++++++++++++++++++++++++++++++-
>  include/hw/cxl/cxl_device.h | 15 ++++++++++++
>  3 files changed, 109 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
> index aec8b0d421..6544a68567 100644
> --- a/hw/cxl/cxl-device-utils.c
> +++ b/hw/cxl/cxl-device-utils.c
> @@ -158,6 +158,45 @@ static void mailbox_reg_write(void *opaque, hwaddr 
> offset, uint64_t value,
>          process_mailbox(cxl_dstate);
>  }
>  
> +static uint64_t mdev_reg_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    uint64_t retval = 0;
> +
> +    retval = FIELD_DP64(retval, CXL_MEM_DEV_STS, MEDIA_STATUS, 1);
> +    retval = FIELD_DP64(retval, CXL_MEM_DEV_STS, MBOX_READY, 1);
> +
> +    switch (size) {
> +    case 4:
> +        if (unlikely(offset & (sizeof(uint32_t) - 1))) {
> +            qemu_log_mask(LOG_UNIMP, "Unaligned register read\n");
> +            return 0;
> +        }
> +        break;
> +    case 8:
> +        if (unlikely(offset & (sizeof(uint64_t) - 1))) {
> +            qemu_log_mask(LOG_UNIMP, "Unaligned register read\n");
> +            return 0;
> +        }
> +        break;
> +    }
> +
> +    return ldn_le_p(&retval, size);
> +}
> +
> +static const MemoryRegionOps mdev_ops = {
> +    .read = mdev_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,
> +    },
> +};
> +
>  static const MemoryRegionOps mailbox_ops = {
>      .read = mailbox_reg_read,
>      .write = mailbox_reg_write,
> @@ -213,6 +252,9 @@ void cxl_device_register_block_init(Object *obj, 
> CXLDeviceState *cxl_dstate)
>                            "device-status", CXL_DEVICE_REGISTERS_LENGTH);
>      memory_region_init_io(&cxl_dstate->mailbox, obj, &mailbox_ops, 
> cxl_dstate,
>                            "mailbox", CXL_MAILBOX_REGISTERS_LENGTH);
> +    memory_region_init_io(&cxl_dstate->memory_device, obj, &mdev_ops,
> +                          cxl_dstate, "memory device caps",
> +                          CXL_MEMORY_DEVICE_REGISTERS_LENGTH);
>  
>      memory_region_add_subregion(&cxl_dstate->device_registers, 0,
>                                  &cxl_dstate->caps);
> @@ -221,6 +263,9 @@ void cxl_device_register_block_init(Object *obj, 
> CXLDeviceState *cxl_dstate)
>                                  &cxl_dstate->device);
>      memory_region_add_subregion(&cxl_dstate->device_registers,
>                                  CXL_MAILBOX_REGISTERS_OFFSET, 
> &cxl_dstate->mailbox);
> +    memory_region_add_subregion(&cxl_dstate->device_registers,
> +                                CXL_MEMORY_DEVICE_REGISTERS_OFFSET,
> +                                &cxl_dstate->memory_device);
>  }
>  
>  static void mailbox_init_common(uint32_t *mbox_regs)
> @@ -233,7 +278,7 @@ static void mailbox_init_common(uint32_t *mbox_regs)
>  void cxl_device_register_init_common(CXLDeviceState *cxl_dstate)
>  {
>      uint32_t *cap_hdrs = cxl_dstate->caps_reg_state32;
> -    const int cap_count = 1;

Guessing this should previously have been 2?

> +    const int cap_count = 3;
>  
>      /* CXL Device Capabilities Array Register */
>      ARRAY_FIELD_DP32(cap_hdrs, CXL_DEV_CAP_ARRAY, CAP_ID, 0);
> @@ -242,6 +287,7 @@ void cxl_device_register_init_common(CXLDeviceState 
> *cxl_dstate)
>  
>      cxl_device_cap_init(cxl_dstate, DEVICE, 1);
>      cxl_device_cap_init(cxl_dstate, MAILBOX, 2);
> +    cxl_device_cap_init(cxl_dstate, MEMORY_DEVICE, 0x4000);
>  
>      mailbox_init_common(cxl_dstate->mbox_reg_state32);
>  }
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 2d1b0ef9e4..5d2579800e 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -12,6 +12,12 @@
>  #include "hw/pci/pci.h"
>  #include "hw/cxl/cxl.h"
>  
> +enum cxl_opcode {
> +    CXL_EVENTS      = 0x1,
> +    CXL_IDENTIFY    = 0x40,
> +        #define CXL_IDENTIFY_MEMORY_DEVICE = 0x0
> +};
> +
>  /* 8.2.8.4.5.1 Command Return Codes */
>  enum {
>      RET_SUCCESS                 = 0x0,
> @@ -40,6 +46,43 @@ enum {
>      RET_MAX                     = 0x17
>  };
>  
> +/* 8.2.9.5.1.1 */
> +static int cmd_set_identify(CXLDeviceState *cxl_dstate, uint8_t cmd,
> +                            uint32_t *ret_size)

I'm a bit confused on naming here, perhaps rsp_set_identity makes
it clearer which direction this is going in?  I think this is
filling in the reply for a command from software running on the
system. Naming seems to me to suggest we are setting the identity
of the hardware.  

> +{
> +    struct identify {
> +        char fw_revision[0x10];
> +        uint64_t total_capacity;
> +        uint64_t volatile_capacity;
> +        uint64_t persistent_capacity;
> +        uint64_t partition_align;
> +        uint16_t info_event_log_size;
> +        uint16_t warning_event_log_size;
> +        uint16_t failure_event_log_size;
> +        uint16_t fatal_event_log_size;
> +        uint32_t lsa_size;
> +        uint8_t poison_list_max_mer[3];
> +        uint16_t inject_poison_limit;
> +        uint8_t poison_caps;
> +        uint8_t qos_telemetry_caps;
> +    } __attribute__((packed)) *id;
> +    _Static_assert(sizeof(struct identify) == 0x43, "Bad identify size");
> +
> +    if (memory_region_size(cxl_dstate->pmem) < (256 << 20)) {
> +        return RET_ENODEV;
> +    }
> +
> +    /* PMEM only */
> +    id = (struct identify *)((void *)cxl_dstate->mbox_reg_state +
> +                             A_CXL_DEV_CMD_PAYLOAD);
> +    snprintf(id->fw_revision, 0x10, "BWFW VERSION %02d", 0);
> +    id->total_capacity = memory_region_size(cxl_dstate->pmem);
> +    id->persistent_capacity = memory_region_size(cxl_dstate->pmem);
> +
> +    *ret_size = 0x43;
> +    return RET_SUCCESS;
> +}
> +
>  void process_mailbox(CXLDeviceState *cxl_dstate)
>  {
>      uint16_t ret = RET_SUCCESS;
> @@ -63,8 +106,11 @@ void process_mailbox(CXLDeviceState *cxl_dstate)
>  
>      uint8_t cmd_set = FIELD_EX64(command_reg, CXL_DEV_MAILBOX_CMD, 
> COMMAND_SET);
>      uint8_t cmd = FIELD_EX64(command_reg, CXL_DEV_MAILBOX_CMD, COMMAND);
> -    (void)cmd;

Clean this stuff up before v2.

>      switch (cmd_set) {
> +    case CXL_IDENTIFY:
> +        ret = cmd_set_identify(cxl_dstate, cmd, &ret_len);
> +        /* Fill in payload here */
> +        break;
>      default:
>          ret = RET_ENOTSUP;
>      }
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index df00998def..2cb2a9af3c 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -69,6 +69,10 @@
>  #define CXL_MAILBOX_REGISTERS_LENGTH \
>      (CXL_MAILBOX_REGISTERS_SIZE + CXL_MAILBOX_MAX_PAYLOAD_SIZE)
>  
> +#define CXL_MEMORY_DEVICE_REGISTERS_OFFSET \
> +    (CXL_MAILBOX_REGISTERS_OFFSET + CXL_MAILBOX_REGISTERS_LENGTH)
> +#define CXL_MEMORY_DEVICE_REGISTERS_LENGTH 0x8
> +
>  typedef struct cxl_device_state {
>      /* Boss container and caps registers */
>      MemoryRegion device_registers;
> @@ -76,6 +80,7 @@ typedef struct cxl_device_state {
>      MemoryRegion caps;
>      MemoryRegion device;
>      MemoryRegion mailbox;
> +    MemoryRegion memory_device;
>  
>      MemoryRegion *pmem;
>      MemoryRegion *vmem;
> @@ -131,6 +136,8 @@ REG32(CXL_DEV_CAP_ARRAY2, 4) /* We're going to pretend 
> it's 64b */
>  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)
> +CXL_DEVICE_CAPABILITY_HEADER_REGISTER(MEMORY_DEVICE, 
> CXL_DEVICE_CAP_HDR1_OFFSET + \
> +                                                     CXL_DEVICE_CAP_REG_SIZE 
> * 2)
>  
>  void process_mailbox(CXLDeviceState *cxl_dstate);
>  
> @@ -181,4 +188,12 @@ REG32(CXL_DEV_BG_CMD_STS, 0x18)
>  
>  REG32(CXL_DEV_CMD_PAYLOAD, 0x20)
>  
> +/* XXX: actually a 64b registers */
> +REG32(CXL_MEM_DEV_STS, 0)
> +    FIELD(CXL_MEM_DEV_STS, FATAL, 0, 1)
> +    FIELD(CXL_MEM_DEV_STS, FW_HALT, 1, 1)
> +    FIELD(CXL_MEM_DEV_STS, MEDIA_STATUS, 2, 2)
> +    FIELD(CXL_MEM_DEV_STS, MBOX_READY, 4, 1)
> +    FIELD(CXL_MEM_DEV_STS, RESET_NEEDED, 5, 3)
> +
>  #endif