Re: [PATCH v6 22/43] hw/cxl/device: Implement get/set Label Storage Area (LSA)

[Jonathan Cameron]

On Fri, 4 Mar 2022 14:16:18 +0000
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> On Wed, 02 Mar 2022 10:03:34 +0000
> Alex Bennée <alex.bennee@linaro.org> wrote:
> 
> > Jonathan Cameron <Jonathan.Cameron@huawei.com> writes:
> >   
> > > From: Ben Widawsky <ben.widawsky@intel.com>
> > >
> > > Implement get and set handlers for the Label Storage Area
> > > used to hold data describing persistent memory configuration
> > > so that it can be ensured it is seen in the same configuration
> > > after reboot.
> > >
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > ---  
> 
> 
> > > +
> > > +static ret_code cmd_ccls_set_lsa(struct cxl_cmd *cmd,
> > > +                                 CXLDeviceState *cxl_dstate,
> > > +                                 uint16_t *len)
> > > +{
> > > +    struct {
> > > +        uint32_t offset;
> > > +        uint32_t rsvd;
> > > +    } __attribute__((packed, __aligned__(8))) *set_lsa = (void 
> > > *)cmd->payload;
> > > +    CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, 
> > > cxl_dstate);
> > > +    CXLType3Class *cvc = CXL_TYPE3_DEV_GET_CLASS(ct3d);
> > > +    uint16_t plen = *len;
> > > +
> > > +    *len = 0;
> > > +    if (!plen) {
> > > +        return CXL_MBOX_SUCCESS;
> > > +    }
> > > +
> > > +    if (set_lsa->offset + plen > cvc->get_lsa_size(ct3d) + 
> > > sizeof(*set_lsa)) {
> > > +        return CXL_MBOX_INVALID_INPUT;
> > > +    }
> > > +
> > > +    cvc->set_lsa(ct3d, (void *)set_lsa + sizeof(*set_lsa),
> > > +                 plen - sizeof(*set_lsa), set_lsa->offset);    
> > 
> > All these sizeof's make me wonder if these structures should be public
> > with a #define? 
> > Failing that having
> > 
> >   const size_t lsa_len = sizeof(*set_lsa);  
> 
> The naming of such a variable is a bit complex and I think perhaps
> we are better off using a 0 length data[] element after the header
> and then offsetof() to compute the header length.
> 
> So (with the addition of some renames that hopefully make things clearer)
> something like (on top of changes as a result of earlier review comments):
> 
> @@ -357,12 +357,15 @@ static ret_code cmd_ccls_set_lsa(struct cxl_cmd *cmd,
>                                   CXLDeviceState *cxl_dstate,
>                                   uint16_t *len)
>  {
> -    struct {
> +    struct set_lsa_pl {
>          uint32_t offset;
>          uint32_t rsvd;
> -    } QEMU_PACKED QEMU_ALIGNED(8) *set_lsa = (void *)cmd->payload;
> +        uint8_t data[];
> +    } QEMU_PACKED;
> +    struct set_lsa_pl *set_lsa_payload = (void *)cmd->payload;
>      CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
>      CXLType3Class *cvc = CXL_TYPE3_DEV_GET_CLASS(ct3d);
> +    const size_t hdr_len = offsetof(struct set_lsa_pl, data);
>      uint16_t plen = *len;
> 
>      *len = 0;
> @@ -370,12 +373,12 @@ static ret_code cmd_ccls_set_lsa(struct cxl_cmd *cmd,
>          return CXL_MBOX_SUCCESS;
>      }
> 
> -    if (set_lsa->offset + plen > cvc->get_lsa_size(ct3d) + sizeof(*set_lsa)) 
> {
> +    if (set_lsa_payload->offset + plen > cvc->get_lsa_size(ct3d) + hdr_len) {
>          return CXL_MBOX_INVALID_INPUT;
>      }
> +    plen -= hdr_len;
> 
> -    cvc->set_lsa(ct3d, (void *)set_lsa + sizeof(*set_lsa),
> -                 plen - sizeof(*set_lsa), set_lsa->offset);
> +    cvc->set_lsa(ct3d, set_lsa_payload->data, plen, set_lsa_payload->offset);
>      return CXL_MBOX_SUCCESS;
>  }
> 
> the aligned marker doesn't make much sense here as we are using
> cmd->payload directly so the compiler doesn't get to mess with the alignment
> anyway.  I've aligned markings from similar places in this patch and others. 

Turned out I was wrong on that - it's necessary in some places (not here)
to avoid the compiler thinking we might be taking addresses of unaligned
elements of a structure when we aren't.

This one is safe to remove however.

> 
> > 
> > and use that might make the flow a bit easier to scan. That said why
> > isn't set_lsa taking a type of *set_lsa instead of having void casts?  
> 
> We could do that, but I think the intent here is to separate the particular
> formatting of the set_lsa command from what it is doing which
> is basically a memcpy so needs an src, size and dst offset.
> 
> It's a bit messy that the size to be written is implicit so we have to
> extract it from the size of the command message accounting for the
> header.
> 
> Hopefully the above changes make that easier to follow?
> 
> Thanks,
> 
> Jonathan
> 
> 
> >   
> > > +    return CXL_MBOX_SUCCESS;
> > > +}
> > > +
> > >  #define IMMEDIATE_CONFIG_CHANGE (1 << 1)
> > > +#define IMMEDIATE_DATA_CHANGE (1 << 2)
> > >  #define IMMEDIATE_POLICY_CHANGE (1 << 3)
> > >  #define IMMEDIATE_LOG_CHANGE (1 << 4)
> > >  
> > > @@ -350,6 +404,9 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
> > >          cmd_identify_memory_device, 0, 0 },
> > >      [CCLS][GET_PARTITION_INFO] = { "CCLS_GET_PARTITION_INFO",
> > >          cmd_ccls_get_partition_info, 0, 0 },
> > > +    [CCLS][GET_LSA] = { "CCLS_GET_LSA", cmd_ccls_get_lsa, 0, 0 },
> > > +    [CCLS][SET_LSA] = { "CCLS_SET_LSA", cmd_ccls_set_lsa,
> > > +        ~0, IMMEDIATE_CONFIG_CHANGE | IMMEDIATE_DATA_CHANGE },
> > >  };
> > >  
> > >  void cxl_process_mailbox(CXLDeviceState *cxl_dstate)
> > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > > index b16262d3cc..b1ba4bf0de 100644
> > > --- a/hw/mem/cxl_type3.c
> > > +++ b/hw/mem/cxl_type3.c
> > > @@ -8,6 +8,7 @@
> > >  #include "qapi/error.h"
> > >  #include "qemu/log.h"
> > >  #include "qemu/module.h"
> > > +#include "qemu/pmem.h"
> > >  #include "qemu/range.h"
> > >  #include "qemu/rcu.h"
> > >  #include "sysemu/hostmem.h"
> > > @@ -114,6 +115,11 @@ static void cxl_setup_memory(CXLType3Dev *ct3d, 
> > > Error **errp)
> > >      memory_region_set_enabled(mr, true);
> > >      host_memory_backend_set_mapped(ct3d->hostmem, true);
> > >      ct3d->cxl_dstate.pmem_size = ct3d->hostmem->size;
> > > +
> > > +    if (!ct3d->lsa) {
> > > +        error_setg(errp, "lsa property must be set");
> > > +        return;
> > > +    }
> > >  }
> > >  
> > >  
> > > @@ -168,12 +174,58 @@ static Property ct3_props[] = {
> > >      DEFINE_PROP_SIZE("size", CXLType3Dev, size, -1),
> > >      DEFINE_PROP_LINK("memdev", CXLType3Dev, hostmem, TYPE_MEMORY_BACKEND,
> > >                       HostMemoryBackend *),
> > > +    DEFINE_PROP_LINK("lsa", CXLType3Dev, lsa, TYPE_MEMORY_BACKEND,
> > > +                     HostMemoryBackend *),
> > >      DEFINE_PROP_END_OF_LIST(),
> > >  };
> > >  
> > >  static uint64_t get_lsa_size(CXLType3Dev *ct3d)
> > >  {
> > > -    return 0;
> > > +    MemoryRegion *mr;
> > > +
> > > +    mr = host_memory_backend_get_memory(ct3d->lsa);
> > > +    return memory_region_size(mr);
> > > +}
> > > +
> > > +static void validate_lsa_access(MemoryRegion *mr, uint64_t size,
> > > +                                uint64_t offset)
> > > +{
> > > +    assert(offset + size <= memory_region_size(mr));
> > > +    assert(offset + size > offset);
> > > +}
> > > +
> > > +static uint64_t get_lsa(CXLType3Dev *ct3d, void *buf, uint64_t size,
> > > +                    uint64_t offset)
> > > +{
> > > +    MemoryRegion *mr;
> > > +    void *lsa;
> > > +
> > > +    mr = host_memory_backend_get_memory(ct3d->lsa);
> > > +    validate_lsa_access(mr, size, offset);
> > > +
> > > +    lsa = memory_region_get_ram_ptr(mr) + offset;
> > > +    memcpy(buf, lsa, size);
> > > +
> > > +    return size;
> > > +}
> > > +
> > > +static void set_lsa(CXLType3Dev *ct3d, const void *buf, uint64_t size,
> > > +                    uint64_t offset)
> > > +{
> > > +    MemoryRegion *mr;
> > > +    void *lsa;
> > > +
> > > +    mr = host_memory_backend_get_memory(ct3d->lsa);
> > > +    validate_lsa_access(mr, size, offset);
> > > +
> > > +    lsa = memory_region_get_ram_ptr(mr) + offset;
> > > +    memcpy(lsa, buf, size);
> > > +    memory_region_set_dirty(mr, offset, size);
> > > +
> > > +    /*
> > > +     * Just like the PMEM, if the guest is not allowed to exit 
> > > gracefully, label
> > > +     * updates will get lost.
> > > +     */
> > >  }
> > >  
> > >  static void ct3_class_init(ObjectClass *oc, void *data)
> > > @@ -194,6 +246,8 @@ static void ct3_class_init(ObjectClass *oc, void 
> > > *data)
> > >      device_class_set_props(dc, ct3_props);
> > >  
> > >      cvc->get_lsa_size = get_lsa_size;
> > > +    cvc->get_lsa = get_lsa;
> > > +    cvc->set_lsa = set_lsa;
> > >  }
> > >  
> > >  static const TypeInfo ct3d_info = {
> > > diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> > > index ebb391153a..43908f161b 100644
> > > --- a/include/hw/cxl/cxl_device.h
> > > +++ b/include/hw/cxl/cxl_device.h
> > > @@ -257,6 +257,11 @@ struct CXLType3Class {
> > >  
> > >      /* public */
> > >      uint64_t (*get_lsa_size)(CXLType3Dev *ct3d);
> > > +
> > > +    uint64_t (*get_lsa)(CXLType3Dev *ct3d, void *buf, uint64_t size,
> > > +                        uint64_t offset);
> > > +    void (*set_lsa)(CXLType3Dev *ct3d, const void *buf, uint64_t size,
> > > +                    uint64_t offset);
> > >  };
> > >  
> > >  #endif    
> > 
> >   
>