On Mon, 28 Nov 2022 10:01:57 -0500
Gregory Price <gourry.memve...@gmail.com> wrote:
> From: Gregory Price <gourry.memve...@gmail.com>
>
> This commit enables each CXL Type-3 device to contain one volatile
> memory region and one persistent region.
>
> Two new properties have been added to cxl-type3 device initialization:
> [volatile-memdev] and [persistent-memdev]
>
> 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). It cannot be used in
> combination with the new [persistent-memdev] property.
>
> Partitioning volatile memory from persistent memory is not yet supported.
>
> Volatile memory is mapped at DPA(0x0), while Persistent memory is mapped
> at DPA(vmem->size), per CXL Spec 8.2.9.8.2.0 - Get Partition Info.
>
> Signed-off-by: Gregory Price <gregory.pr...@memverge.com>
> Signed-off-by: Jonathan Cameron <jonathan.came...@huawei.com>
I'm taking another look at these and tweaking what I'm carrying as I go
with a plan to post them for merging shortly.
Anyhow, a few more changes I wanted to call out here so they don't come
as a surprise / are things to focus on in reviewing that version
(hopefully post it later today or maybe tomorrow).
Most of this is reducing the diff / duplication of code to make review a
tiny bit easier (hopefully)
...
> /* TODO: Support multiple HDM decoders and DPA skip */
> @@ -663,11 +769,17 @@ MemTxResult cxl_type3_read(PCIDevice *d, hwaddr
> host_addr, uint64_t *data,
> {
> CXLType3Dev *ct3d = CXL_TYPE3(d);
> uint64_t dpa_offset;
> - MemoryRegion *mr;
> + MemoryRegion *vmr = NULL, *pmr = NULL;
> + AddressSpace *as;
We end up with a lot of duplication between cxl_type3_read and cxl_type3_write()
so I've factored out a _get_as_and_dpa() function that deals with establish
where the memory we then read or write is.
>
> - /* TODO support volatile region */
> - mr = host_memory_backend_get_memory(ct3d->hostmem);
> - if (!mr) {
> + if (ct3d->hostvmem) {
> + vmr = host_memory_backend_get_memory(ct3d->hostvmem);
> + }
> + if (ct3d->hostpmem) {
> + pmr = host_memory_backend_get_memory(ct3d->hostpmem);
> + }
> +
> + if (!vmr && !pmr) {
> return MEMTX_ERROR;
> }
>
> @@ -675,11 +787,22 @@ MemTxResult cxl_type3_read(PCIDevice *d, hwaddr
> host_addr, uint64_t *data,
> return MEMTX_ERROR;
> }
>
> - if (dpa_offset > int128_get64(mr->size)) {
> + if (dpa_offset > int128_get64(ct3d->cxl_dstate.mem_size)) {
> return MEMTX_ERROR;
> }
>
> - return address_space_read(&ct3d->hostmem_as, dpa_offset, attrs, data,
> size);
> + if (vmr) {
> + if (dpa_offset <= int128_get64(vmr->size)) {
> + as = &ct3d->hostvmem_as;
> + } else {
> + as = &ct3d->hostpmem_as;
> + dpa_offset -= vmr->size;
> + }
> + }
> + else {
> + as = &ct3d->hostpmem_as;
> + }
> + return address_space_read(as, dpa_offset, attrs, data, size);
> }
>
> MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
> @@ -687,10 +810,17 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr
> host_addr, uint64_t data,
> {
> CXLType3Dev *ct3d = CXL_TYPE3(d);
> uint64_t dpa_offset;
> - MemoryRegion *mr;
> + MemoryRegion *vmr = NULL, *pmr = NULL;
> + AddressSpace *as;
>
> - mr = host_memory_backend_get_memory(ct3d->hostmem);
> - if (!mr) {
> + if (ct3d->hostvmem) {
> + vmr = host_memory_backend_get_memory(ct3d->hostvmem);
> + }
> + if (ct3d->hostpmem) {
> + pmr = host_memory_backend_get_memory(ct3d->hostpmem);
> + }
> +
> + if (!vmr && !pmr) {
> return MEMTX_OK;
> }
>
> @@ -698,11 +828,22 @@ MemTxResult cxl_type3_write(PCIDevice *d, hwaddr
> host_addr, uint64_t data,
> return MEMTX_OK;
> }
>
> - if (dpa_offset > int128_get64(mr->size)) {
> + if (dpa_offset > int128_get64(ct3d->cxl_dstate.mem_size)) {
> return MEMTX_OK;
> }
> - return address_space_write(&ct3d->hostmem_as, dpa_offset, attrs,
> - &data, size);
> +
> + if (vmr) {
> + if (dpa_offset <= int128_get64(vmr->size)) {
> + as = &ct3d->hostvmem_as;
> + } else {
> + as = &ct3d->hostpmem_as;
> + dpa_offset -= vmr->size;
> + }
> + }
> + else {
> + as = &ct3d->hostpmem_as;
> + }
> + return address_space_write(as, dpa_offset, attrs, &data, size);
> }
>
> static void ct3d_reset(DeviceState *dev)
> @@ -717,7 +858,11 @@ static void ct3d_reset(DeviceState *dev)
>
> static Property ct3_props[] = {
> DEFINE_PROP_LINK("memdev", CXLType3Dev, hostmem, TYPE_MEMORY_BACKEND,
> - HostMemoryBackend *),
> + HostMemoryBackend *), /* for backward compatibility */
> + DEFINE_PROP_LINK("persistent-memdev", CXLType3Dev, hostpmem,
> + TYPE_MEMORY_BACKEND, HostMemoryBackend *),
> + DEFINE_PROP_LINK("volatile-memdev", CXLType3Dev, hostvmem,
> + TYPE_MEMORY_BACKEND, HostMemoryBackend *),
> DEFINE_PROP_LINK("lsa", CXLType3Dev, lsa, TYPE_MEMORY_BACKEND,
> HostMemoryBackend *),
> DEFINE_PROP_UINT64("sn", CXLType3Dev, sn, UI64_NULL),
> @@ -728,10 +873,12 @@ static Property ct3_props[] = {
>
> static uint64_t get_lsa_size(CXLType3Dev *ct3d)
> {
> - MemoryRegion *mr;
> -
> - mr = host_memory_backend_get_memory(ct3d->lsa);
> - return memory_region_size(mr);
> + MemoryRegion *mr = NULL;
> + if (ct3d->lsa) {
I flipped this logic as reduces the resulting diff and makes patch a little
easier to read.
> + mr = host_memory_backend_get_memory(ct3d->lsa);
> + return memory_region_size(mr);
> + }
> + return 0;
> }
>
> static void validate_lsa_access(MemoryRegion *mr, uint64_t size,
> @@ -744,30 +891,35 @@ static void validate_lsa_access(MemoryRegion *mr,
> uint64_t size,
> static uint64_t get_lsa(CXLType3Dev *ct3d, void *buf, uint64_t size,
> uint64_t offset)
> {
> - MemoryRegion *mr;
> + MemoryRegion *mr = NULL;
> void *lsa;
>
> - mr = host_memory_backend_get_memory(ct3d->lsa);
> - validate_lsa_access(mr, size, offset);
> + if (ct3d->lsa) {
Flipped this one as well.
> + mr = host_memory_backend_get_memory(ct3d->lsa);
> + validate_lsa_access(mr, size, offset);
>
> - lsa = memory_region_get_ram_ptr(mr) + offset;
> - memcpy(buf, lsa, size);
> + lsa = memory_region_get_ram_ptr(mr) + offset;
> + memcpy(buf, lsa, size);
> + return size;
> + }
>
> - return size;
> + return 0;
> }
>
> static void set_lsa(CXLType3Dev *ct3d, const void *buf, uint64_t size,
> uint64_t offset)
> {
> - MemoryRegion *mr;
> - void *lsa;
> + MemoryRegion *mr = NULL;
> + void *lsa = NULL;
>
> - mr = host_memory_backend_get_memory(ct3d->lsa);
> - validate_lsa_access(mr, size, offset);
> + if (ct3d->lsa) {
> + mr = host_memory_backend_get_memory(ct3d->lsa);
> + validate_lsa_access(mr, size, offset);
>
> - lsa = memory_region_get_ram_ptr(mr) + offset;
> - memcpy(lsa, buf, size);
> - memory_region_set_dirty(mr, offset, size);
> + lsa = memory_region_get_ram_ptr(mr) + offset;
> + memcpy(lsa, buf, size);
> + memory_region_set_dirty(mr, offset, size);
> + }
>
> /*
> * Just like the PMEM, if the guest is not allowed to exit gracefully,
> label
> @@ -978,7 +1130,7 @@ static void ct3_class_init(ObjectClass *oc, void *data)
> pc->config_read = ct3d_config_read;
>
> 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);
>
>
