[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH 08/25] hw/cxl/device: Add memory devices (8.2.8.5)
From: |
Jonathan Cameron |
Subject: |
Re: [RFC PATCH 08/25] hw/cxl/device: Add memory devices (8.2.8.5) |
Date: |
Tue, 17 Nov 2020 14:31:00 +0000 |
On Mon, 16 Nov 2020 13:45:05 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:
> 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"?
yup. I guess my mind didn't want to parse it.
>
> 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()
Maybe a little more to make the point that it is filling in values that will get
sent back from here. Maybe something like:
mem_dev_fillresp_identify()?
Meh. Bike-shedding time - can always fall back to a comment to clarify what
it is doing if we can't find a magic non-confusing name.
Jonathan
>
> > > +{
> > > + 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
> >
- Re: [RFC PATCH 06/25] hw/cxl/device: Add device status (8.2.8.3), (continued)
[RFC PATCH 07/25] hw/cxl/device: Implement basic mailbox (8.2.8.4), Ben Widawsky, 2020/11/11
[RFC PATCH 09/25] hw/pxb: Use a type for realizing expanders, Ben Widawsky, 2020/11/11
[RFC PATCH 08/25] hw/cxl/device: Add memory devices (8.2.8.5), Ben Widawsky, 2020/11/11
[RFC PATCH 10/25] hw/pci/cxl: Create a CXL bus type, Ben Widawsky, 2020/11/11
[RFC PATCH 11/25] hw/pxb: Allow creation of a CXL PXB (host bridge), Ben Widawsky, 2020/11/11
[RFC PATCH 12/25] acpi/pci: Consolidate host bridge setup, Ben Widawsky, 2020/11/11
[RFC PATCH 13/25] hw/pci: Plumb _UID through host bridges, Ben Widawsky, 2020/11/11
[RFC PATCH 14/25] hw/cxl/component: Implement host bridge MMIO (8.2.5, table 142), Ben Widawsky, 2020/11/11