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

["Alex Bennée" <alex.bennee@linaro.org>]

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>
> ---
>  hw/cxl/cxl-mailbox-utils.c  | 57 +++++++++++++++++++++++++++++++++++++
>  hw/mem/cxl_type3.c          | 56 +++++++++++++++++++++++++++++++++++-
>  include/hw/cxl/cxl_device.h |  5 ++++
>  3 files changed, 117 insertions(+), 1 deletion(-)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index ccf9c3d794..f4a309ddbf 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -56,6 +56,8 @@ enum {
>          #define MEMORY_DEVICE 0x0
>      CCLS        = 0x41,
>          #define GET_PARTITION_INFO     0x0
> +        #define GET_LSA       0x2
> +        #define SET_LSA       0x3
>  };
>  
>  /* 8.2.8.4.5.1 Command Return Codes */
> @@ -327,7 +329,59 @@ static ret_code cmd_ccls_get_partition_info(struct cxl_cmd *cmd,
>      return CXL_MBOX_SUCCESS;
>  }
>  
> +static ret_code cmd_ccls_get_lsa(struct cxl_cmd *cmd,
> +                                 CXLDeviceState *cxl_dstate,
> +                                 uint16_t *len)
> +{
> +    struct {
> +        uint32_t offset;
> +        uint32_t length;
> +    } __attribute__((packed, __aligned__(8))) *get_lsa;
> +    CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
> +    CXLType3Class *cvc = CXL_TYPE3_DEV_GET_CLASS(ct3d);
> +    uint32_t offset, length;
> +
> +    get_lsa = (void *)cmd->payload;
> +    offset = get_lsa->offset;
> +    length = get_lsa->length;
> +
> +    *len = 0;

This can drop into the failure leg.

> +    if (offset + length > cvc->get_lsa_size(ct3d)) {
> +        return CXL_MBOX_INVALID_INPUT;
> +    }
> +
> +    *len = cvc->get_lsa(ct3d, get_lsa, length, offset);
> +    return CXL_MBOX_SUCCESS;
> +}
> +
> +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);

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?

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


-- 
Alex Bennée