Re: [RFC PATCH 07/25] hw/cxl/device: Implement basic mailbox (8.2.8.4)

[Ben Widawsky]

On 20-11-16 13:46:51, Jonathan Cameron wrote:
> On Tue, 10 Nov 2020 21:47:06 -0800
> Ben Widawsky <ben.widawsky@intel.com> wrote:
> 
> > This is the beginning of implementing mailbox support for CXL 2.0
> > devices.
> > 
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> Mostly patch set cleanup suggestions rather than anything meaningful
> in here.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  hw/cxl/cxl-device-utils.c   | 131 ++++++++++++++++++++++++++++++++++++
> >  hw/cxl/cxl-mailbox-utils.c  |  93 +++++++++++++++++++++++++
> >  hw/cxl/meson.build          |   1 +
> >  include/hw/cxl/cxl.h        |   3 +
> >  include/hw/cxl/cxl_device.h |  10 ++-
> >  5 files changed, 237 insertions(+), 1 deletion(-)
> >  create mode 100644 hw/cxl/cxl-mailbox-utils.c
> > 
> > diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
> > index 78144e103c..aec8b0d421 100644
> > --- a/hw/cxl/cxl-device-utils.c
> > +++ b/hw/cxl/cxl-device-utils.c
> > @@ -55,6 +55,123 @@ static uint64_t dev_reg_read(void *opaque, hwaddr 
> > offset, unsigned size)
> >      return ldn_le_p(&retval, size);
> >  }
> >  
> > +static uint64_t mailbox_reg_read(void *opaque, hwaddr offset, unsigned 
> > size)
> > +{
> > +    CXLDeviceState *cxl_dstate = opaque;
> > +
> > +    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;
> > +    default:
> > +        qemu_log_mask(LOG_UNIMP, "%uB component register read\n", size);
> > +        return 0;
> > +    }
> > +
> > +    return ldn_le_p(cxl_dstate->mbox_reg_state + offset, size);
> > +}
> > +
> > +static void mailbox_mem_writel(uint32_t *reg_state, hwaddr offset,
> > +                               uint64_t value)
> > +{
> > +    switch (offset) {
> > +    case A_CXL_DEV_MAILBOX_CTRL:
> > +        /* fallthrough */
> > +    case A_CXL_DEV_MAILBOX_CAP:
> > +        /* RO register */
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_UNIMP,
> > +                      "%s Unexpected 32-bit access to 0x%" PRIx64 " 
> > (WI)\n",
> > +                      __func__, offset);
> > +        break;
> > +    }
> > +
> > +    stl_le_p((uint8_t *)reg_state + offset, value);
> > +}
> > +
> > +static void mailbox_mem_writeq(uint64_t *reg_state, hwaddr offset,
> > +                               uint64_t value)
> > +{
> > +    switch (offset) {
> > +    case A_CXL_DEV_MAILBOX_CMD:
> > +        break;
> > +    case A_CXL_DEV_BG_CMD_STS:
> > +        /* BG not supported */
> > +        /* fallthrough */
> > +    case A_CXL_DEV_MAILBOX_STS:
> > +        /* Read only register, will get updated by the state machine */
> > +        return;
> > +    case A_CXL_DEV_MAILBOX_CAP:
> > +    case A_CXL_DEV_MAILBOX_CTRL:
> 
> I wouldn't bother listing these here given you don't list the MAILBOX_STS etc 
> in
> the 32 bit version.
> 
> > +    default:
> > +        qemu_log_mask(LOG_UNIMP,
> > +                      "%s Unexpected 64-bit access to 0x%" PRIx64 " 
> > (WI)\n",
> > +                      __func__, offset);
> > +        return;
> > +    }
> > +
> > +    stq_le_p((uint8_t *)reg_state + offset, value);
> > +}
> > +
> > +static void mailbox_reg_write(void *opaque, hwaddr offset, uint64_t value,
> > +                              unsigned size)
> > +{
> > +    CXLDeviceState *cxl_dstate = opaque;
> > +
> > +    /*
> > +     * Lock is needed to prevent concurrent writes as well as to prevent 
> > writes
> > +     * coming in while the firmware is processing. Without background 
> > commands
> > +     * or the second mailbox implemented, this serves no purpose since the
> > +     * memory access is synchronized at a higher level (per memory region).
> > +     */
> > +    RCU_READ_LOCK_GUARD();
> > +
> > +    switch (size) {
> > +    case 4:
> > +        if (unlikely(offset & (sizeof(uint32_t) - 1))) {
> > +            qemu_log_mask(LOG_UNIMP, "Unaligned register read\n");
> > +            return;
> > +        }
> > +        mailbox_mem_writel(cxl_dstate->mbox_reg_state32, offset, value);
> > +        break;
> > +    case 8:
> > +        if (unlikely(offset & (sizeof(uint64_t) - 1))) {
> > +            qemu_log_mask(LOG_UNIMP, "Unaligned register read\n");
> > +            return;
> > +        }
> > +        mailbox_mem_writeq(cxl_dstate->mbox_reg_state64, offset, value);
> > +        break;
> > +    }
> > +
> > +    if (ARRAY_FIELD_EX32(cxl_dstate->mbox_reg_state32, 
> > CXL_DEV_MAILBOX_CTRL,
> > +                         DOORBELL))
> > +        process_mailbox(cxl_dstate);
> > +}
> > +
> > +static const MemoryRegionOps mailbox_ops = {
> > +    .read = mailbox_reg_read,
> > +    .write = mailbox_reg_write,
> > +    .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 dev_ops = {
> >      .read = dev_reg_read,
> >      .write = NULL,
> > @@ -94,12 +211,23 @@ void cxl_device_register_block_init(Object *obj, 
> > CXLDeviceState *cxl_dstate)
> >                            "cap-array", CXL_DEVICE_REGISTERS_OFFSET - 0);
> >      memory_region_init_io(&cxl_dstate->device, obj, &dev_ops, 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_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);
> > +    memory_region_add_subregion(&cxl_dstate->device_registers,
> > +                                CXL_MAILBOX_REGISTERS_OFFSET, 
> > &cxl_dstate->mailbox);
> > +}
> > +
> > +static void mailbox_init_common(uint32_t *mbox_regs)
> > +{
> > +    /* 2048 payload size, with no interrupt or background support */
> > +    ARRAY_FIELD_DP32(mbox_regs, CXL_DEV_MAILBOX_CAP, PAYLOAD_SIZE,
> > +                     CXL_MAILBOX_PAYLOAD_SHIFT);
> >  }
> >  
> >  void cxl_device_register_init_common(CXLDeviceState *cxl_dstate)
> > @@ -113,4 +241,7 @@ void cxl_device_register_init_common(CXLDeviceState 
> > *cxl_dstate)
> >      ARRAY_FIELD_DP32(cap_hdrs, CXL_DEV_CAP_ARRAY2, CAP_COUNT, cap_count);
> >  
> >      cxl_device_cap_init(cxl_dstate, DEVICE, 1);
> > +    cxl_device_cap_init(cxl_dstate, MAILBOX, 2);
> > +
> > +    mailbox_init_common(cxl_dstate->mbox_reg_state32);
> >  }
> > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> > new file mode 100644
> > index 0000000000..2d1b0ef9e4
> > --- /dev/null
> > +++ b/hw/cxl/cxl-mailbox-utils.c
> > @@ -0,0 +1,93 @@
> > +/*
> > + * CXL Utility library for mailbox interface
> > + *
> > + * 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/pci/pci.h"
> > +#include "hw/cxl/cxl.h"
> > +
> > +/* 8.2.8.4.5.1 Command Return Codes */
> > +enum {
> > +    RET_SUCCESS                 = 0x0,
> > +    RET_BG_STARTED              = 0x1, /* Background Command Started */
> > +    RET_EINVAL                  = 0x2, /* Invalid Input */
> > +    RET_ENOTSUP                 = 0x3, /* Unsupported */
> > +    RET_ENODEV                  = 0x4, /* Internal Error */
> > +    RET_ERESTART                = 0x5, /* Retry Required */
> > +    RET_EBUSY                   = 0x6, /* Busy */
> > +    RET_MEDIA_DISABLED          = 0x7, /* Media Disabled */
> > +    RET_FW_EBUSY                = 0x8, /* FW Transfer in Progress */
> > +    RET_FW_OOO                  = 0x9, /* FW Transfer Out of Order */
> > +    RET_FW_AUTH                 = 0xa, /* FW Authentication Failed */
> > +    RET_FW_EBADSLT              = 0xb, /* Invalid Slot */
> > +    RET_FW_ROLLBACK             = 0xc, /* Activation Failed, FW Rolled 
> > Back */
> > +    RET_FW_REBOOT               = 0xd, /* Activation Failed, Cold Reset 
> > Required */
> > +    RET_ENOENT                  = 0xe, /* Invalid Handle */
> > +    RET_EFAULT                  = 0xf, /* Invalid Physical Address */
> > +    RET_POISON_E2BIG            = 0x10, /* Inject Poison Limit Reached */
> > +    RET_EIO                     = 0x11, /* Permanent Media Failure */
> > +    RET_ECANCELED               = 0x12, /* Aborted */
> > +    RET_EACCESS                 = 0x13, /* Invalid Security State */
> > +    RET_EPERM                   = 0x14, /* Incorrect Passphrase */
> > +    RET_EPROTONOSUPPORT         = 0x15, /* Unsupported Mailbox */
> > +    RET_EMSGSIZE                = 0x16, /* Invalid Payload Length */
> > +    RET_MAX                     = 0x17
> > +};
> 
> Ah back again.  Just drop the earlier add and remove of this list and
> we are all good.
> 
> > +
> > +void process_mailbox(CXLDeviceState *cxl_dstate)
> > +{
> > +    uint16_t ret = RET_SUCCESS;
> > +    uint32_t ret_len = 0;
> > +    uint64_t status_reg;
> > +
> > +    /*
> > +     * current state of mailbox interface
> > +     *  uint32_t mbox_cap_reg = 
> > cxl_dstate->reg_state32[R_CXL_DEV_MAILBOX_CAP];
> > +     *  uint32_t mbox_ctrl_reg = 
> > cxl_dstate->reg_state32[R_CXL_DEV_MAILBOX_CTRL];
> > +     *  uint64_t status_reg = *(uint64_t 
> > *)&cxl_dstate->reg_state[A_CXL_DEV_MAILBOX_STS];
> > +     */
> > +    uint64_t command_reg =
> > +        *(uint64_t *)&cxl_dstate->mbox_reg_state[A_CXL_DEV_MAILBOX_CMD];
> > +
> > +    /* Check if we have to do anything */
> > +    if (!ARRAY_FIELD_EX32(cxl_dstate->mbox_reg_state32, 
> > CXL_DEV_MAILBOX_CTRL, DOORBELL)) {
> > +        qemu_log_mask(LOG_UNIMP, "Corrupt internal state for firmware\n");
> > +        return;
> > +    }
> > +
> > +    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;
> 
> ?

I have plans for this, so you can ignore it for now.

> 
> > +    switch (cmd_set) {
> > +    default:
> > +        ret = RET_ENOTSUP;
> > +    }
> > +
> > +    /*
> > +     * Set the return code
> > +     * XXX: it's a 64b register, but we're not setting the vendor, so we 
> > can get
> > +     * away with this
> 
> Also mention not setting background operation bit?
> 
> > +     */
> > +    status_reg = FIELD_DP64(0, CXL_DEV_MAILBOX_STS, ERRNO, ret);
> > +
> > +    /*
> > +     * Set the return length
> > +     */
> > +    command_reg = FIELD_DP64(command_reg, CXL_DEV_MAILBOX_CMD, 
> > COMMAND_SET, 0);
> > +    command_reg = FIELD_DP64(command_reg, CXL_DEV_MAILBOX_CMD, COMMAND, 0);
> > +    command_reg = FIELD_DP64(command_reg, CXL_DEV_MAILBOX_CMD, LENGTH, 
> > ret_len);
> 
> Rather convoluted way of setting just the length field, I assume because there
> are RsvdP fields in there we can't touch.
> 
> > +
> > +    stq_le_p(cxl_dstate->mbox_reg_state + A_CXL_DEV_MAILBOX_CMD, 
> > command_reg);
> > +    stq_le_p(cxl_dstate->mbox_reg_state + A_CXL_DEV_MAILBOX_STS, 
> > status_reg);
> > +
> > +    /* Tell the host we're done */
> > +    ARRAY_FIELD_DP32(cxl_dstate->mbox_reg_state32, CXL_DEV_MAILBOX_CTRL,
> > +                     DOORBELL, 0);
> > +}
> > +
> > diff --git a/hw/cxl/meson.build b/hw/cxl/meson.build
> > index 47154d6850..0eca715d10 100644
> > --- a/hw/cxl/meson.build
> > +++ b/hw/cxl/meson.build
> > @@ -1,4 +1,5 @@
> >  softmmu_ss.add(when: 'CONFIG_CXL', if_true: files(
> >    'cxl-component-utils.c',
> >    'cxl-device-utils.c',
> > +  'cxl-mailbox-utils.c',
> >  ))
> > diff --git a/include/hw/cxl/cxl.h b/include/hw/cxl/cxl.h
> > index 23f52c4cf9..362cda40de 100644
> > --- a/include/hw/cxl/cxl.h
> > +++ b/include/hw/cxl/cxl.h
> > @@ -14,5 +14,8 @@
> >  #include "cxl_component.h"
> >  #include "cxl_device.h"
> >  
> > +#define COMPONENT_REG_BAR_IDX 0
> > +#define DEVICE_REG_BAR_IDX 2
> > +
> >  #endif
> >  
> > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > index 2c674fdc9c..df00998def 100644
> > --- a/include/hw/cxl/cxl_device.h
> > +++ b/include/hw/cxl/cxl_device.h
> > @@ -87,6 +87,11 @@ typedef struct cxl_device_state {
> >          uint8_t caps_reg_state[CXL_DEVICE_CAP_REG_SIZE * 4]; /* ARRAY + 3 
> > CAPS */
> >          uint32_t caps_reg_state32[0];
> >      };
> > +    union {
> > +        uint8_t mbox_reg_state[CXL_MAILBOX_REGISTERS_LENGTH];
> > +        uint32_t mbox_reg_state32[0];
> > +        uint64_t mbox_reg_state64[0];
> > +    };
> >  } CXLDeviceState;
> >  
> >  /* Initialize the register block for a device */
> > @@ -127,6 +132,8 @@ 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)
> >  
> > +void process_mailbox(CXLDeviceState *cxl_dstate);
> > +
> >  #define cxl_device_cap_init(dstate, reg, cap_id)                           
> >         \
> >      do {                                                                   
> >         \
> >          uint32_t *cap_hdrs = dstate->caps_reg_state32;                     
> >         \
> > @@ -155,7 +162,8 @@ REG32(CXL_DEV_MAILBOX_CTRL, 4)
> >      FIELD(CXL_DEV_MAILBOX_CTRL, BG_INT_EN, 2, 1)
> >  
> >  REG32(CXL_DEV_MAILBOX_CMD, 8)
> > -    FIELD(CXL_DEV_MAILBOX_CMD, OP, 0, 16)
> > +    FIELD(CXL_DEV_MAILBOX_CMD, COMMAND, 0, 8)
> 
> Can we fix the original introduction of this so we don't end up modifying it 
> here?
> From spec I can fully see how you ended up with this as you wrote the code
> but nice to get rid of the two step definition now anyway.
> (the field is first defined as 16 bits, then later it says there are two 8 
> bit fields).

I'll change it...

Please see this commit (and branch) for what I'm planning to do here:
https://gitlab.com/bwidawsk/qemu/-/commit/bdb9f9a5337873aedb89558d28968caa130db05e#0d08a7f7c79cf3169f15ad6c4bb7a440c3603af6_47_65

> 
> > +    FIELD(CXL_DEV_MAILBOX_CMD, COMMAND_SET, 8, 8)
> >      FIELD(CXL_DEV_MAILBOX_CMD, LENGTH, 16, 20)
> >  
> >  /* XXX: actually a 64b register */
>