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

[Ben Widawsky <ben.widawsky@intel.com>]

On 20-11-16 16:37:22, Jonathan Cameron wrote:
> 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.  
> 

It sounds like maybe you read "identify" as "identity"?

You're correct, this represents the firmware running on the memory device that
is receiving the identify command from the host. I've been thinking about
renaming these based on what the underlying device is. For instance, this might
become:

mem_dev_identify()

> > +{
> > +    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
>