Re: [PATCH 2/2] hw/cxl: Allow CXL type-3 devices to be persistent or volatile

[Davidlohr Bueso <dave@stgolabs.net>]

On Thu, 06 Oct 2022, Gregory Price wrote:

>diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
>index bc1bb18844..dfec11a1b5 100644
>--- a/hw/cxl/cxl-mailbox-utils.c
>+++ b/hw/cxl/cxl-mailbox-utils.c
>@@ -138,7 +138,7 @@ static ret_code cmd_firmware_update_get_info(struct cxl_cmd *cmd,
>     } QEMU_PACKED *fw_info;
>     QEMU_BUILD_BUG_ON(sizeof(*fw_info) != 0x50);
>
>-    if (cxl_dstate->pmem_size < (256 << 20)) {
>+    if (cxl_dstate->mem_size < (256 << 20)) {

Nit but we probably want to abstract this out (in a pre-patch), just like in the
kernel side. Ie:

#define CXL_CAPACITY_MULTIPLIER   0x10000000 /* SZ_256M */

>         return CXL_MBOX_INTERNAL_ERROR;
>     }
>
>@@ -281,9 +281,10 @@ static ret_code cmd_identify_memory_device(struct cxl_cmd *cmd,
>
>     CXLType3Dev *ct3d = container_of(cxl_dstate, CXLType3Dev, cxl_dstate);
>     CXLType3Class *cvc = CXL_TYPE3_GET_CLASS(ct3d);
>-    uint64_t size = cxl_dstate->pmem_size;
>
>-    if (!QEMU_IS_ALIGNED(size, 256 << 20)) {
>+    if ((!QEMU_IS_ALIGNED(cxl_dstate->mem_size, 256 << 20)) ||

is the full mem_size check here really needed?

>+        (!QEMU_IS_ALIGNED(cxl_dstate->vmem_size, 256 << 20)) ||
>+        (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, 256 << 20))) {
>         return CXL_MBOX_INTERNAL_ERROR;
>     }
>
>@@ -293,8 +294,9 @@ static ret_code cmd_identify_memory_device(struct cxl_cmd *cmd,
>     /* PMEM only */

This comment wants removed.

>     snprintf(id->fw_revision, 0x10, "BWFW VERSION %02d", 0);
>
>-    id->total_capacity = size / (256 << 20);
>-    id->persistent_capacity = size / (256 << 20);
>+    id->total_capacity = cxl_dstate->mem_size / (256 << 20);
>+    id->persistent_capacity = cxl_dstate->pmem_size / (256 << 20);
>+    id->volatile_capacity = cxl_dstate->vmem_size / (256 << 20);
>     id->lsa_size = cvc->get_lsa_size(ct3d);
>
>     *len = sizeof(*id);
>@@ -312,16 +314,16 @@ static ret_code cmd_ccls_get_partition_info(struct cxl_cmd *cmd,
>         uint64_t next_pmem;
>     } QEMU_PACKED *part_info = (void *)cmd->payload;
>     QEMU_BUILD_BUG_ON(sizeof(*part_info) != 0x20);
>-    uint64_t size = cxl_dstate->pmem_size;
>
>-    if (!QEMU_IS_ALIGNED(size, 256 << 20)) {
>+    if ((!QEMU_IS_ALIGNED(cxl_dstate->mem_size, 256 << 20)) ||
>+        (!QEMU_IS_ALIGNED(cxl_dstate->vmem_size, 256 << 20)) ||
>+        (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, 256 << 20))) {
>         return CXL_MBOX_INTERNAL_ERROR;
>     }
>
>-    /* PMEM only */
>-    part_info->active_vmem = 0;
>+    part_info->active_vmem = cxl_dstate->vmem_size / (256 << 20);
>     part_info->next_vmem = 0;
>-    part_info->active_pmem = size / (256 << 20);
>+    part_info->active_pmem = cxl_dstate->pmem_size / (256 << 20);
>     part_info->next_pmem = 0;
>
>     *len = sizeof(*part_info);
>diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
>index 1837c1c83a..998461dac1 100644
>--- a/hw/mem/cxl_type3.c
>+++ b/hw/mem/cxl_type3.c
>@@ -100,18 +100,47 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
>     DeviceState *ds = DEVICE(ct3d);
>     MemoryRegion *mr;
>     char *name;
>+    bool is_pmem = false;
>
>-    if (!ct3d->hostmem) {
>-        error_setg(errp, "memdev property must be set");
>+    /*
>+     * FIXME: For now we only allow a single host memory region.
>+     * Handle the deprecated memdev property usage cases
>+     */
>+    if (!ct3d->hostmem && !ct3d->host_vmem && !ct3d->host_pmem) {
>+        error_setg(errp, "at least one memdev property must be set");
>         return false;
>+    } else if (ct3d->hostmem && (ct3d->host_vmem || ct3d->host_pmem)) {
>+        error_setg(errp, "deprecated [memdev] cannot be used with new "
>+                         "persistent and volatile memdev properties");
>+        return false;
>+    } else if (ct3d->hostmem) {
>+        warn_report("memdev is deprecated and defaults to pmem. "
>+                    "Use (persistent|volatile)-memdev instead.");
>+        is_pmem = true;
>+    } else {
>+        if (ct3d->host_vmem && ct3d->host_pmem) {
>+            error_setg(errp, "Multiple memory devices not supported yet");
>+            return false;
>+        }
>+        is_pmem = !!ct3d->host_pmem;
>+        ct3d->hostmem = ct3d->host_pmem ? ct3d->host_pmem : ct3d->host_vmem;

This hides requirement details as to the necessary changes that are needed for
volatile support - for example, build_dvsecs(). Imo using two backends (without
breaking current configs, of course) should be the initial version, not something
to leave pending.

Thanks,
Davidlohr