diff mbox series

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

Message ID 20220211120747.3074-23-Jonathan.Cameron@huawei.com (mailing list archive)
State New, archived
Headers show
Series CXl 2.0 emulation Support | expand

Commit Message

Jonathan Cameron Feb. 11, 2022, 12:07 p.m. UTC 
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(-)

Comments

Alex Bennée March 2, 2022, 10:03 a.m. UTC | #1 
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
Jonathan Cameron March 4, 2022, 2:16 p.m. UTC | #2 
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  
> 
>
Jonathan Cameron March 4, 2022, 2:26 p.m. UTC | #3 
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    
> > 
> >   
>
diff mbox series

Patch

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