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