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