| Message ID | 20221006233702.18532-2-gregory.price@memverge.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] hw/cxl: set cxl-type3 device type to PCI_CLASS_MEMORY_CXL | expand |
On Thu, 6 Oct 2022 19:37:02 -0400 Gregory Price <gourry.memverge@gmail.com> wrote: > This commit enables setting one memory region for a type-3 device, but > that region may now be either a persistent region or volatile region. > > Future work may enable setting both regions simultaneously, as this is > a possible configuration on a real type-3 device. The scaffolding was > put in for this, but is left for further work. > > The existing [memdev] property has been deprecated and will default the > memory region to a persistent memory region (although a user may assign > the region to a ram or file backed region). > > There is presently no interface with which to expose a MemoryRegion's > real backing type (HostMemoryBackendRam/File), otherwise we could have > used File to imply pmem (or inspected HostMemoryBackendFile->is_pmem) to > deterine whether the backing device supported pmem mode. This should be > considered for future work, as it would make creating an array of > HostMemory devices to represent DIMMs on a Single-Logical-Device > MemoryExpander fairly straightforward. > > Signed-off-by: Gregory Price <gregory.price@memverge.com> > --- Looks good to me, though I haven't tested it yet. A few trivial comments inline. > *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. TODO maybe? FIXME tends to lead to reviewers asking why we haven't fixed it yet! > + * 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."); I'd suggest we don't warn on this yet. There is limited advantage in doing so given it's easy to carry on supporting by just treating it as another name for persistent-memdev > + 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; > } >
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
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; };
Hang tight, I'm whipping up a multi-region patch that will support a vmem and pmem region and such. Finally got oriented enough to figure out the DPA decoding a bit. I will probably need some help validating the decoder logic and the CDAT table logic. I will integrate the suggestions below into that patch set. Jonathan i'm building on top of your gitlab branch and will make a branch available for review when done. On Mon, Oct 10, 2022 at 12:36:54PM -0700, Davidlohr Bueso wrote: > 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; > };
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)) { 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)) || + (!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 */ 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; } + /* + * 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"); return false; } - memory_region_set_nonvolatile(mr, true); + + /* + * 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); @@ -123,7 +152,10 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp) address_space_init(&ct3d->hostmem_as, mr, name); g_free(name); - ct3d->cxl_dstate.pmem_size = ct3d->hostmem->size; + /* 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->lsa) { error_setg(errp, "lsa property must be set"); @@ -272,6 +304,10 @@ 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, + TYPE_MEMORY_BACKEND, HostMemoryBackend *), + DEFINE_PROP_LINK("volatile-memdev", CXLType3Dev, host_vmem, + TYPE_MEMORY_BACKEND, HostMemoryBackend *), DEFINE_PROP_LINK("lsa", CXLType3Dev, lsa, TYPE_MEMORY_BACKEND, HostMemoryBackend *), DEFINE_PROP_END_OF_LIST(), @@ -340,7 +376,7 @@ static void ct3_class_init(ObjectClass *oc, void *data) pc->revision = 1; set_bit(DEVICE_CATEGORY_STORAGE, dc->categories); - dc->desc = "CXL PMEM Device (Type 3)"; + dc->desc = "CXL Memory Device (Type 3)"; dc->reset = ct3d_reset; device_class_set_props(dc, ct3_props); diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h index 1e141b6621..fd96a5ea4e 100644 --- a/include/hw/cxl/cxl_device.h +++ b/include/hw/cxl/cxl_device.h @@ -117,8 +117,10 @@ typedef struct cxl_device_state { uint64_t host_set; } timestamp; - /* memory region for persistent memory, HDM */ + /* memory region size, HDM */ + uint64_t mem_size; uint64_t pmem_size; + uint64_t vmem_size; } CXLDeviceState; /* Initialize the register block for a device */ @@ -235,7 +237,10 @@ struct CXLType3Dev { PCIDevice parent_obj; /* Properties */ + /* TODO: remove hostmem when multi-dev is implemented */ HostMemoryBackend *hostmem; + HostMemoryBackend *host_vmem; + HostMemoryBackend *host_pmem; HostMemoryBackend *lsa; /* State */
This commit enables setting one memory region for a type-3 device, but that region may now be either a persistent region or volatile region. Future work may enable setting both regions simultaneously, as this is a possible configuration on a real type-3 device. The scaffolding was put in for this, but is left for further work. The existing [memdev] property has been deprecated and will default the memory region to a persistent memory region (although a user may assign the region to a ram or file backed region). There is presently no interface with which to expose a MemoryRegion's real backing type (HostMemoryBackendRam/File), otherwise we could have used File to imply pmem (or inspected HostMemoryBackendFile->is_pmem) to deterine whether the backing device supported pmem mode. This should be considered for future work, as it would make creating an array of HostMemory devices to represent DIMMs on a Single-Logical-Device MemoryExpander fairly straightforward. Signed-off-by: Gregory Price <gregory.price@memverge.com> --- hw/cxl/cxl-mailbox-utils.c | 22 ++++++++++-------- hw/mem/cxl_type3.c | 46 +++++++++++++++++++++++++++++++++---- include/hw/cxl/cxl_device.h | 7 +++++- 3 files changed, 59 insertions(+), 16 deletions(-)