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

[Davidlohr Bueso]

On Mon, 10 Oct 2022, Davidlohr Bueso wrote:

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

Minimally this is along the lines I was thinking of. I rebased some of my 
original
patches on top of yours. It builds and passes tests/qtest/cxl-test, but 
certainly
untested otherwise. The original code did show the volatile support as per 
cxl-list.

As such users can still use memdev which will map to the pmemdev. One thing 
which I
had not explored was the lsa + vmem thing, so the below prevents this for the 
time
being, fyi.

Thanks,
Davidlohr

----8<----------------------------------------------------

diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index e8341a818467..cd079dbddd9a 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -18,14 +18,21 @@ static void build_dvsecs(CXLType3Dev *ct3d)
 {
     CXLComponentState *cxl_cstate = &ct3d->cxl_cstate;
     uint8_t *dvsec;
+    uint64_t size = 0;
+
+    if (ct3d->hostvmem) {
+        size += ct3d->hostvmem->size;
+    }
+    if (ct3d->hostpmem) {
+        size += ct3d->hostpmem->size;
+    }

     dvsec = (uint8_t *)&(CXLDVSECDevice){
-        .cap = 0x1e,
+        .cap = 0x1e, /* one HDM range */
         .ctrl = 0x2,
         .status2 = 0x2,
-        .range1_size_hi = ct3d->hostmem->size >> 32,
-        .range1_size_lo = (2 << 5) | (2 << 2) | 0x3 |
-        (ct3d->hostmem->size & 0xF0000000),
+        .range1_size_hi = size >> 32,
+        .range1_size_lo = (2 << 5) | (2 << 2) | 0x3 | (size & 0xF0000000),
         .range1_base_hi = 0,
         .range1_base_lo = 0,
     };
@@ -98,70 +105,60 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, 
uint64_t value,
 static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
 {
     DeviceState *ds = DEVICE(ct3d);
-    MemoryRegion *mr;
     char *name;
-    bool is_pmem = false;

-    /*
-     * 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) {
+    if (!ct3d->hostvmem && !ct3d->hostpmem) {
         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;
     }

-    /*
-     * for now, since there is only one memdev, we can set the type
-     * based on whether this was a ram region or file region
-     */
-    mr = host_memory_backend_get_memory(ct3d->hostmem);
-    if (!mr) {
-        error_setg(errp, "memdev property must be set");
+    /* TODO: volatile devices may have LSA */
+    if (ct3d->hostvmem && ct3d->lsa) {
+        error_setg(errp, "lsa property must be set");
         return false;
     }

-    /*
-     * FIXME: This code should eventually enumerate each memory region and
-     * report vmem and pmem capacity separate, but for now just set to one
-     */
-    memory_region_set_nonvolatile(mr, is_pmem);
-    memory_region_set_enabled(mr, true);
-    host_memory_backend_set_mapped(ct3d->hostmem, true);
-
     if (ds->id) {
         name = g_strdup_printf("cxl-type3-dpa-space:%s", ds->id);
     } else {
         name = g_strdup("cxl-type3-dpa-space");
     }
-    address_space_init(&ct3d->hostmem_as, mr, name);
-    g_free(name);

-    /* FIXME: When multiple regions are supported, this needs to aggregate */
-    ct3d->cxl_dstate.mem_size = ct3d->hostmem->size;
-    ct3d->cxl_dstate.vmem_size = is_pmem ? 0 : ct3d->hostmem->size;
-    ct3d->cxl_dstate.pmem_size = is_pmem ? ct3d->hostmem->size : 0;
+    if (ct3d->hostvmem) {
+        MemoryRegion *vmr;

-    if (!ct3d->lsa) {
-        error_setg(errp, "lsa property must be set");
-        return false;
+        vmr = host_memory_backend_get_memory(ct3d->hostvmem);
+        if (!vmr) {
+            error_setg(errp, "volatile-memdev property must be set");
+            return false;
+        }
+
+        memory_region_set_nonvolatile(vmr, false);
+        memory_region_set_enabled(vmr, true);
+        host_memory_backend_set_mapped(ct3d->hostvmem, true);
+        address_space_init(&ct3d->hostvmem_as, vmr, name);
+        ct3d->cxl_dstate.vmem_size = ct3d->hostvmem->size;
+        ct3d->cxl_dstate.mem_size += ct3d->hostvmem->size;
     }

+    if (ct3d->hostpmem) {
+        MemoryRegion *pmr;
+
+        pmr = host_memory_backend_get_memory(ct3d->hostpmem);
+        if (!pmr) {
+            error_setg(errp, "legacy memdev or persistent-memdev property must be 
set");
+            return false;
+        }
+
+        memory_region_set_nonvolatile(pmr, true);
+        memory_region_set_enabled(pmr, true);
+        host_memory_backend_set_mapped(ct3d->hostpmem, true);
+        address_space_init(&ct3d->hostpmem_as, pmr, name);
+        ct3d->cxl_dstate.pmem_size = ct3d->hostpmem->size;
+        ct3d->cxl_dstate.mem_size += ct3d->hostpmem->size;
+    }
+    g_free(name);
+
     return true;
 }

@@ -210,7 +207,13 @@ static void ct3_exit(PCIDevice *pci_dev)
     ComponentRegisters *regs = &cxl_cstate->crb;

     g_free(regs->special_ops);
-    address_space_destroy(&ct3d->hostmem_as);
+
+    if (ct3d->hostvmem) {
+        address_space_destroy(&ct3d->hostvmem_as);
+    }
+    if (ct3d->hostpmem) {
+        address_space_destroy(&ct3d->hostpmem_as);
+    }
 }

 /* TODO: Support multiple HDM decoders and DPA skip */
@@ -249,47 +252,86 @@ MemTxResult cxl_type3_read(PCIDevice *d, hwaddr 
host_addr, uint64_t *data,
                            unsigned size, MemTxAttrs attrs)
 {
     CXLType3Dev *ct3d = CXL_TYPE3(d);
-    uint64_t dpa_offset;
-    MemoryRegion *mr;
+    uint64_t total_size = 0, dpa_offset;
+    MemoryRegion *vmr, *pmr;

-    /* TODO support volatile region */
-    mr = host_memory_backend_get_memory(ct3d->hostmem);
-    if (!mr) {
+    vmr = host_memory_backend_get_memory(ct3d->hostvmem);
+    pmr = host_memory_backend_get_memory(ct3d->hostpmem);
+    if (!vmr && !pmr) {
         return MEMTX_ERROR;
     }

+    if (vmr) {
+        total_size += vmr->size;
+    }
+    if (pmr) {
+        total_size += pmr->size;
+    }
     if (!cxl_type3_dpa(ct3d, host_addr, &dpa_offset)) {
         return MEMTX_ERROR;
     }
-
-    if (dpa_offset > int128_get64(mr->size)) {
+    if (dpa_offset > total_size) {
         return MEMTX_ERROR;
     }

-    return address_space_read(&ct3d->hostmem_as, dpa_offset, attrs, data, 
size);
+    if (vmr) {
+        /* volatile starts at DPA 0 */
+        if (dpa_offset <= int128_get64(vmr->size)) {
+            return address_space_read(&ct3d->hostvmem_as,
+                                  dpa_offset, attrs, data, size);
+        }
+    }
+    if (pmr) {
+        if (dpa_offset > int128_get64(pmr->size)) {
+            return MEMTX_ERROR;
+        }
+        return address_space_read(&ct3d->hostpmem_as, dpa_offset,
+                                  attrs, data, size);
+    }
+
+    return MEMTX_ERROR;
 }

 MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
                             unsigned size, MemTxAttrs attrs)
 {
     CXLType3Dev *ct3d = CXL_TYPE3(d);
-    uint64_t dpa_offset;
-    MemoryRegion *mr;
+    uint64_t total_size = 0, dpa_offset;
+    MemoryRegion *vmr, *pmr;

-    mr = host_memory_backend_get_memory(ct3d->hostmem);
-    if (!mr) {
+    vmr = host_memory_backend_get_memory(ct3d->hostpmem);
+    pmr = host_memory_backend_get_memory(ct3d->hostpmem);
+    if (!vmr && !pmr) {
         return MEMTX_OK;
     }
-
+    if (vmr) {
+        total_size += vmr->size;
+    }
+    if (pmr) {
+        total_size += pmr->size;
+    }
     if (!cxl_type3_dpa(ct3d, host_addr, &dpa_offset)) {
         return MEMTX_OK;
     }
+    if (dpa_offset > total_size) {
+        return MEMTX_ERROR;
+    }

-    if (dpa_offset > int128_get64(mr->size)) {
-        return MEMTX_OK;
+    if (vmr) {
+        if (dpa_offset <= int128_get64(vmr->size)) {
+                return address_space_write(&ct3d->hostvmem_as,
+                                  dpa_offset, attrs, &data, size);
+        }
     }
-    return address_space_write(&ct3d->hostmem_as, dpa_offset, attrs,
+    if (pmr) {
+        if (dpa_offset > int128_get64(pmr->size)) {
+            return MEMTX_OK;
+        }
+        return address_space_write(&ct3d->hostpmem_as, dpa_offset, attrs,
                                &data, size);
+    }
+
+    return MEMTX_ERROR;
 }

 static void ct3d_reset(DeviceState *dev)
@@ -303,11 +345,11 @@ static void ct3d_reset(DeviceState *dev)
 }

 static Property ct3_props[] = {
-    DEFINE_PROP_LINK("memdev", CXLType3Dev, hostmem, TYPE_MEMORY_BACKEND,
-                     HostMemoryBackend *),
-    DEFINE_PROP_LINK("persistent-memdev", CXLType3Dev, host_pmem,
+    DEFINE_PROP_LINK("memdev", CXLType3Dev, hostpmem, TYPE_MEMORY_BACKEND,
+                     HostMemoryBackend *), /* for backward-compatibility */
+    DEFINE_PROP_LINK("persistent-memdev", CXLType3Dev, hostpmem,
                      TYPE_MEMORY_BACKEND, HostMemoryBackend *),
-    DEFINE_PROP_LINK("volatile-memdev", CXLType3Dev, host_vmem,
+    DEFINE_PROP_LINK("volatile-memdev", CXLType3Dev, hostvmem,
                      TYPE_MEMORY_BACKEND, HostMemoryBackend *),
     DEFINE_PROP_LINK("lsa", CXLType3Dev, lsa, TYPE_MEMORY_BACKEND,
                      HostMemoryBackend *),
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index fd96a5ea4e47..c81f92ecf093 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -237,14 +237,13 @@ struct CXLType3Dev {
     PCIDevice parent_obj;

     /* Properties */
-    /* TODO: remove hostmem when multi-dev is implemented */
-    HostMemoryBackend *hostmem;
-    HostMemoryBackend *host_vmem;
-    HostMemoryBackend *host_pmem;
+    HostMemoryBackend *hostvmem;
+    HostMemoryBackend *hostpmem;
     HostMemoryBackend *lsa;

     /* State */
-    AddressSpace hostmem_as;
+    AddressSpace hostvmem_as;
+    AddressSpace hostpmem_as;
     CXLComponentState cxl_cstate;
     CXLDeviceState cxl_dstate;
 };