On Fri, 4 Mar 2022 14:16:18 +0000
Jonathan Cameron <jonathan.came...@huawei.com> wrote:
> On Wed, 02 Mar 2022 10:03:34 +0000
> Alex Bennée <alex.ben...@linaro.org> wrote:
>
> > Jonathan Cameron <jonathan.came...@huawei.com> writes:
> >
> > > From: Ben Widawsky <ben.widaw...@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.widaw...@intel.com>
> > > Signed-off-by: Jonathan Cameron <jonathan.came...@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
> >
> >
>
