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