From: Jonathan Cameron <Jonathan.Cameron@huawei.com> To: Gregory Price <gourry.memverge@gmail.com> Cc: <qemu-devel@nongnu.org>, <linux-cxl@vger.kernel.org>, <alison.schofield@intel.com>, <dave@stgolabs.net>, <a.manzanares@samsung.com>, <bwidawsk@kernel.org>, <gregory.price@memverge.com>, <hchkuo@avery-design.com.tw>, <cbrowy@avery-design.com>, <ira.weiny@intel.com> Subject: Re: [RFC v4 3/3] hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent) Date: Tue, 31 Jan 2023 11:53:14 +0000 [thread overview] Message-ID: <20230131115314.00004f7e@huawei.com> (raw) In-Reply-To: <20221128150157.97724-4-gregory.price@memverge.com> On Mon, 28 Nov 2022 10:01:57 -0500 Gregory Price <gourry.memverge@gmail.com> wrote: > From: Gregory Price <gourry.memverge@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.price@memverge.com> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@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); > >
WARNING: multiple messages have this Message-ID (diff)
From: Jonathan Cameron via <qemu-devel@nongnu.org> To: Gregory Price <gourry.memverge@gmail.com> Cc: <qemu-devel@nongnu.org>, <linux-cxl@vger.kernel.org>, <alison.schofield@intel.com>, <dave@stgolabs.net>, <a.manzanares@samsung.com>, <bwidawsk@kernel.org>, <gregory.price@memverge.com>, <hchkuo@avery-design.com.tw>, <cbrowy@avery-design.com>, <ira.weiny@intel.com> Subject: Re: [RFC v4 3/3] hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent) Date: Tue, 31 Jan 2023 11:53:14 +0000 [thread overview] Message-ID: <20230131115314.00004f7e@huawei.com> (raw) In-Reply-To: <20221128150157.97724-4-gregory.price@memverge.com> On Mon, 28 Nov 2022 10:01:57 -0500 Gregory Price <gourry.memverge@gmail.com> wrote: > From: Gregory Price <gourry.memverge@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.price@memverge.com> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@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); > >
next prev parent reply other threads:[~2023-01-31 11:53 UTC|newest] Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-11-28 15:01 [RFC v4 0/3] CXL Type-3 Volatile Memory Support Gregory Price 2022-11-28 15:01 ` [RFC v4 1/3] hw/cxl: Add CXL_CAPACITY_MULTIPLIER definition Gregory Price 2022-11-28 15:01 ` [RFC v4 2/3] tests/qtest/cxl-test: whitespace, line ending cleanup Gregory Price 2023-01-05 14:38 ` Jonathan Cameron 2023-01-05 14:38 ` Jonathan Cameron via 2023-01-30 13:11 ` Jonathan Cameron 2023-01-30 13:11 ` Jonathan Cameron via 2023-01-30 14:38 ` Gregory Price 2022-11-28 15:01 ` [RFC v4 3/3] hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent) Gregory Price [not found] ` <CGME20221208225559uscas1p1e9e2c7c8f9a1654a5f41cef2c47859a8@uscas1p1.samsung.com> 2022-12-08 22:55 ` Fan Ni 2022-12-08 23:06 ` Gregory Price 2022-12-19 12:42 ` Jonathan Cameron 2022-12-19 12:42 ` Jonathan Cameron via 2022-12-19 16:12 ` Gregory Price 2022-12-19 17:25 ` Jonathan Cameron via 2022-12-19 17:25 ` Jonathan Cameron 2022-12-19 17:55 ` Gregory Price 2022-12-20 15:34 ` Jonathan Cameron 2022-12-20 15:34 ` Jonathan Cameron via 2022-12-20 19:27 ` Gregory Price 2023-01-03 15:56 ` Jonathan Cameron 2023-01-03 15:56 ` Jonathan Cameron via 2023-01-03 16:02 ` Gregory Price 2023-01-03 18:15 ` Jonathan Cameron 2023-01-03 18:15 ` Jonathan Cameron via 2023-01-19 17:15 ` Gregory Price 2023-01-19 17:31 ` Jonathan Cameron 2023-01-19 17:31 ` Jonathan Cameron via 2023-01-19 22:13 ` Gregory Price 2023-01-20 10:59 ` Jonathan Cameron 2023-01-20 10:59 ` Jonathan Cameron via 2023-01-30 13:24 ` Jonathan Cameron 2023-01-30 13:24 ` Jonathan Cameron via 2023-01-30 14:37 ` Gregory Price 2023-01-31 11:53 ` Jonathan Cameron [this message] 2023-01-31 11:53 ` Jonathan Cameron via
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20230131115314.00004f7e@huawei.com \ --to=jonathan.cameron@huawei.com \ --cc=a.manzanares@samsung.com \ --cc=alison.schofield@intel.com \ --cc=bwidawsk@kernel.org \ --cc=cbrowy@avery-design.com \ --cc=dave@stgolabs.net \ --cc=gourry.memverge@gmail.com \ --cc=gregory.price@memverge.com \ --cc=hchkuo@avery-design.com.tw \ --cc=ira.weiny@intel.com \ --cc=linux-cxl@vger.kernel.org \ --cc=qemu-devel@nongnu.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.