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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

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. 

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