Re: [RFC PATCH 06/25] hw/cxl/device: Add device status (8.2.8.3)

[Ben Widawsky]

On 20-11-16 13:16:08, Jonathan Cameron wrote:
> On Tue, 10 Nov 2020 21:47:05 -0800
> Ben Widawsky <ben.widawsky@intel.com> wrote:
> 
> > This implements the CXL device status registers from 8.2.8.3.1 in the
> > CXL 2.0 specification. It is capability ID 0001h.
> > 
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> 
> It does some other stuff it shouldn't as well.  Please tidy that up before
> v2.  A few other passing comments inline.
> 
> Thanks,
> 
> Jonathan
> 
> 
> > ---
> >  hw/cxl/cxl-device-utils.c   | 45 +++++++++++++++++++++++++++++++++-
> >  include/hw/cxl/cxl_device.h | 49 ++++++++++++-------------------------
> >  2 files changed, 60 insertions(+), 34 deletions(-)
> > 
> > diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
> > index a391bb15c6..78144e103c 100644
> > --- a/hw/cxl/cxl-device-utils.c
> > +++ b/hw/cxl/cxl-device-utils.c
> > @@ -33,6 +33,42 @@ static uint64_t caps_reg_read(void *opaque, hwaddr 
> > offset, unsigned size)
> >      return ldn_le_p(cxl_dstate->caps_reg_state + offset, size);
> >  }
> >  
> > +static uint64_t dev_reg_read(void *opaque, hwaddr offset, unsigned size)
> > +{
> > +    uint64_t retval = 0;
> 
> Doesn't seem to be used.
> 

It's required for ldn_le_p, or did you mean something else?

> > +
> 
> Perhaps break the alignment check out to a utility function given this sanity 
> check
> is same as in previous patch.
> 
> > +    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 dev_ops = {
> > +    .read = dev_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 caps_ops = {
> >      .read = caps_reg_read,
> >      .write = NULL,
> > @@ -56,18 +92,25 @@ void cxl_device_register_block_init(Object *obj, 
> > CXLDeviceState *cxl_dstate)
> >  
> >      memory_region_init_io(&cxl_dstate->caps, obj, &caps_ops, 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_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);
> >  }
> >  
> >  void cxl_device_register_init_common(CXLDeviceState *cxl_dstate)
> >  {
> >      uint32_t *cap_hdrs = cxl_dstate->caps_reg_state32;
> > -    const int cap_count = 0;
> > +    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);
> >  }
> > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > index 491eca6e05..2c674fdc9c 100644
> > --- a/include/hw/cxl/cxl_device.h
> > +++ b/include/hw/cxl/cxl_device.h
> > @@ -127,6 +127,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)
> > @@ -138,43 +154,10 @@ REG32(CXL_DEV_MAILBOX_CTRL, 4)
> >      FIELD(CXL_DEV_MAILBOX_CTRL, INT_EN, 1, 2)
> >      FIELD(CXL_DEV_MAILBOX_CTRL, BG_INT_EN, 2, 1)
> >  
> > -enum {
> > -    CXL_CMD_EVENTS              = 0x1,
> > -    CXL_CMD_IDENTIFY            = 0x40,
> > -};
> > -
> >  REG32(CXL_DEV_MAILBOX_CMD, 8)
> >      FIELD(CXL_DEV_MAILBOX_CMD, OP, 0, 16)
> >      FIELD(CXL_DEV_MAILBOX_CMD, LENGTH, 16, 20)
> >  
> > -/* 8.2.8.4.5.1 Command Return Codes */
> 
> Umm. We only just introduced this a few patches ago.  Please tidy that
> up so we don't end up bringing things in and out again.
> 
> > -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
> > -};
> > -
> >  /* XXX: actually a 64b register */
> >  REG32(CXL_DEV_MAILBOX_STS, 0x10)
> >      FIELD(CXL_DEV_MAILBOX_STS, BG_OP, 0, 1)
>