All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH 1/2] tests/qtest/cxl-test: whitespace, line ending cleanup
  2023-01-31 16:38   ` Jonathan Cameron via
  (?)
@ 2023-01-31 16:08   ` Gregory Price
  -1 siblings, 0 replies; 18+ messages in thread
From: Gregory Price @ 2023-01-31 16:08 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: qemu-devel, Michael Tsirkin, Ben Widawsky, linux-cxl, linuxarm,
	Ira Weiny, Gregory Price, Philippe Mathieu-Daudé,
	Mike Maslenkin

On Tue, Jan 31, 2023 at 04:38:46PM +0000, Jonathan Cameron wrote:
> From: Gregory Price <gourry.memverge@gmail.com>
> 
> Defines are starting to exceed line length limits, align them for
> cleanliness before making modifications.
> 
> Signed-off-by: Gregory Price <gregory.price@memverge.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> Changes since RFC v4:
>   Naming consistency improvements.
>  
>  tests/qtest/cxl-test.c | 84 +++++++++++++++++++++++-------------------
>  1 file changed, 46 insertions(+), 38 deletions(-)
> 
> // ... snip ...
>

changes lgtm

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent)
  2023-01-31 16:38   ` Jonathan Cameron via
  (?)
@ 2023-01-31 16:25   ` Gregory Price
  -1 siblings, 0 replies; 18+ messages in thread
From: Gregory Price @ 2023-01-31 16:25 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: qemu-devel, Michael Tsirkin, Ben Widawsky, linux-cxl, linuxarm,
	Ira Weiny, Gregory Price, Philippe Mathieu-Daudé,
	Mike Maslenkin

On Tue, Jan 31, 2023 at 04:38:47PM +0000, Jonathan Cameron 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.
>
> ... snip ...
> 

I have no objections to the changes made.  I'll test when I finish up a
few other tasks.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 0/2] hw/mem: CXL Type-3 Volatile Memory Support
@ 2023-01-31 16:38 ` Jonathan Cameron via
  0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2023-01-31 16:38 UTC (permalink / raw)
  To: qemu-devel, Michael Tsirkin
  Cc: Ben Widawsky, linux-cxl, linuxarm, Ira Weiny, Gregory Price,
	Philippe Mathieu-Daudé,
	Mike Maslenkin

Now we have some kernel code to test this against (and it looks good)
I'd like to propose this series for upstream following 3 other series
already proposed for inclusion:

a) https://lore.kernel.org/linux-cxl/20230130143705.11758-1-Jonathan.Cameron@huawei.com/
   [PATCH v3 00/10] hw/cxl: CXL emulation cleanups and minor fixes for upstream
b) https://lore.kernel.org/linux-cxl/20230130155251.3430-1-Jonathan.Cameron@huawei.com/
   [PATCH v3 0/8] hw/cxl: RAS error emulation and injection
c) https://lore.kernel.org/linux-cxl/20230125152703.9928-1-Jonathan.Cameron@huawei.com/
   [PATCH 0/2] hw/cxl: Passthrough HDM decoder emulation

Changes since RFC V4 called out in individual patches.
 - Some minor fixes.
 - Refactors to simplify patch / resulting code as relevant.
 - Update the bios-tables-test for CXL to use non deprecated interfaces.

Whilst the more significant changes have been discussed on list
Gregory, please take a look at these and check your are happy with
my tweaks.

Kernel support used to test this:
https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/log/?h=for-6.3/cxl-ram-region

Previous cover letter updated to reflect pulling one patch out
as a precursor cleanup in set (a) above.

This patches provides 2 features to the CXL Type-3 Device:
    1) Volatile Memory Region Support
    2) Multi-Region support (1 Volatile, 1 Persistent)

Summary of Changes per-commit:
1) Whitespace updates to docs and tests
2) Refactor CDAT DSMAS Initialization for multi-region initialization
   Multi-Region and Volatile Memory support for CXL Type-3 Devices
   Test and Documentation updates

The final patch in this series makes 6 major changes to the type-3
device in order to implement multi-region and volatile region support
    1) The HostMemoryBackend [hostmem] has been replaced by two
       [hostvmem] and [hostpmem] to store volatile and persistent memory
       respectively
    2) The single AddressSpace has been replaced by two AddressSpaces
       [hostvmem_as] and [hostpmem_as] to map respective memdevs.
    3) Each memory region size and total region are stored separately
    4) The CDAT and DVSEC memory map entries have been updated:
       a) if vmem is present, vmem is mapped at DPA(0)
       b) if pmem is present
          i)  and vmem is present, pmem is mapped at DPA(vmem->size)
          ii) else, pmem is mapped at DPA(0)
       c) partitioning of pmem is not supported in this patch set but
          has been discussed and this design should suffice.
    5) Read/Write functions have been updated to access AddressSpaces
       according to the mapping described in #4.  Access to the
       persistent address space is calculated by (dpa-vmem_len)
    6) cxl-mailbox has been updated to report the respective size of
       volatile and persistent memory regions

Gregory Price (2):
  tests/qtest/cxl-test: whitespace, line ending cleanup
  hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent)

 docs/system/devices/cxl.rst    |  49 ++++--
 hw/cxl/cxl-mailbox-utils.c     |  26 +--
 hw/mem/cxl_type3.c             | 300 +++++++++++++++++++++++++--------
 include/hw/cxl/cxl_device.h    |  11 +-
 tests/qtest/bios-tables-test.c |   8 +-
 tests/qtest/cxl-test.c         | 146 +++++++++++-----
 6 files changed, 398 insertions(+), 142 deletions(-)

-- 
2.37.2


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 0/2] hw/mem: CXL Type-3 Volatile Memory Support
@ 2023-01-31 16:38 ` Jonathan Cameron via
  0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron via @ 2023-01-31 16:38 UTC (permalink / raw)
  To: qemu-devel, Michael Tsirkin
  Cc: Ben Widawsky, linux-cxl, linuxarm, Ira Weiny, Gregory Price,
	Philippe Mathieu-Daudé,
	Mike Maslenkin

Now we have some kernel code to test this against (and it looks good)
I'd like to propose this series for upstream following 3 other series
already proposed for inclusion:

a) https://lore.kernel.org/linux-cxl/20230130143705.11758-1-Jonathan.Cameron@huawei.com/
   [PATCH v3 00/10] hw/cxl: CXL emulation cleanups and minor fixes for upstream
b) https://lore.kernel.org/linux-cxl/20230130155251.3430-1-Jonathan.Cameron@huawei.com/
   [PATCH v3 0/8] hw/cxl: RAS error emulation and injection
c) https://lore.kernel.org/linux-cxl/20230125152703.9928-1-Jonathan.Cameron@huawei.com/
   [PATCH 0/2] hw/cxl: Passthrough HDM decoder emulation

Changes since RFC V4 called out in individual patches.
 - Some minor fixes.
 - Refactors to simplify patch / resulting code as relevant.
 - Update the bios-tables-test for CXL to use non deprecated interfaces.

Whilst the more significant changes have been discussed on list
Gregory, please take a look at these and check your are happy with
my tweaks.

Kernel support used to test this:
https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/log/?h=for-6.3/cxl-ram-region

Previous cover letter updated to reflect pulling one patch out
as a precursor cleanup in set (a) above.

This patches provides 2 features to the CXL Type-3 Device:
    1) Volatile Memory Region Support
    2) Multi-Region support (1 Volatile, 1 Persistent)

Summary of Changes per-commit:
1) Whitespace updates to docs and tests
2) Refactor CDAT DSMAS Initialization for multi-region initialization
   Multi-Region and Volatile Memory support for CXL Type-3 Devices
   Test and Documentation updates

The final patch in this series makes 6 major changes to the type-3
device in order to implement multi-region and volatile region support
    1) The HostMemoryBackend [hostmem] has been replaced by two
       [hostvmem] and [hostpmem] to store volatile and persistent memory
       respectively
    2) The single AddressSpace has been replaced by two AddressSpaces
       [hostvmem_as] and [hostpmem_as] to map respective memdevs.
    3) Each memory region size and total region are stored separately
    4) The CDAT and DVSEC memory map entries have been updated:
       a) if vmem is present, vmem is mapped at DPA(0)
       b) if pmem is present
          i)  and vmem is present, pmem is mapped at DPA(vmem->size)
          ii) else, pmem is mapped at DPA(0)
       c) partitioning of pmem is not supported in this patch set but
          has been discussed and this design should suffice.
    5) Read/Write functions have been updated to access AddressSpaces
       according to the mapping described in #4.  Access to the
       persistent address space is calculated by (dpa-vmem_len)
    6) cxl-mailbox has been updated to report the respective size of
       volatile and persistent memory regions

Gregory Price (2):
  tests/qtest/cxl-test: whitespace, line ending cleanup
  hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent)

 docs/system/devices/cxl.rst    |  49 ++++--
 hw/cxl/cxl-mailbox-utils.c     |  26 +--
 hw/mem/cxl_type3.c             | 300 +++++++++++++++++++++++++--------
 include/hw/cxl/cxl_device.h    |  11 +-
 tests/qtest/bios-tables-test.c |   8 +-
 tests/qtest/cxl-test.c         | 146 +++++++++++-----
 6 files changed, 398 insertions(+), 142 deletions(-)

-- 
2.37.2



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/2] tests/qtest/cxl-test: whitespace, line ending cleanup
  2023-01-31 16:38 ` Jonathan Cameron via
@ 2023-01-31 16:38   ` Jonathan Cameron via
  -1 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2023-01-31 16:38 UTC (permalink / raw)
  To: qemu-devel, Michael Tsirkin
  Cc: Ben Widawsky, linux-cxl, linuxarm, Ira Weiny, Gregory Price,
	Philippe Mathieu-Daudé,
	Mike Maslenkin

From: Gregory Price <gourry.memverge@gmail.com>

Defines are starting to exceed line length limits, align them for
cleanliness before making modifications.

Signed-off-by: Gregory Price <gregory.price@memverge.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
Changes since RFC v4:
  Naming consistency improvements.
 
 tests/qtest/cxl-test.c | 84 +++++++++++++++++++++++-------------------
 1 file changed, 46 insertions(+), 38 deletions(-)

diff --git a/tests/qtest/cxl-test.c b/tests/qtest/cxl-test.c
index 61f25a72b6..eda2bbbbe6 100644
--- a/tests/qtest/cxl-test.c
+++ b/tests/qtest/cxl-test.c
@@ -8,50 +8,58 @@
 #include "qemu/osdep.h"
 #include "libqtest-single.h"
 
-#define QEMU_PXB_CMD "-machine q35,cxl=on " \
-                     "-device pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=52 "  \
-                     "-M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.size=4G "
+#define QEMU_PXB_CMD \
+    "-machine q35,cxl=on " \
+    "-device pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=52 " \
+    "-M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.size=4G "
 
-#define QEMU_2PXB_CMD "-machine q35,cxl=on "                            \
-                      "-device pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=52 "  \
-                      "-device pxb-cxl,id=cxl.1,bus=pcie.0,bus_nr=53 " \
-                      "-M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.targets.1=cxl.1,cxl-fmw.0.size=4G "
+#define QEMU_2PXB_CMD \
+    "-machine q35,cxl=on " \
+    "-device pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=52 " \
+    "-device pxb-cxl,id=cxl.1,bus=pcie.0,bus_nr=53 " \
+    "-M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.targets.1=cxl.1,cxl-fmw.0.size=4G "
 
-#define QEMU_RP "-device cxl-rp,id=rp0,bus=cxl.0,chassis=0,slot=0 "
+#define QEMU_RP \
+    "-device cxl-rp,id=rp0,bus=cxl.0,chassis=0,slot=0 "
 
 /* Dual ports on first pxb */
-#define QEMU_2RP "-device cxl-rp,id=rp0,bus=cxl.0,chassis=0,slot=0 " \
-                 "-device cxl-rp,id=rp1,bus=cxl.0,chassis=0,slot=1 "
+#define QEMU_2RP \
+    "-device cxl-rp,id=rp0,bus=cxl.0,chassis=0,slot=0 " \
+    "-device cxl-rp,id=rp1,bus=cxl.0,chassis=0,slot=1 "
 
 /* Dual ports on each of the pxb instances */
-#define QEMU_4RP "-device cxl-rp,id=rp0,bus=cxl.0,chassis=0,slot=0 " \
-                 "-device cxl-rp,id=rp1,bus=cxl.0,chassis=0,slot=1 " \
-                 "-device cxl-rp,id=rp2,bus=cxl.1,chassis=0,slot=2 " \
-                 "-device cxl-rp,id=rp3,bus=cxl.1,chassis=0,slot=3 "
-
-#define QEMU_T3D "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
-                 "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M "    \
-                 "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 "
-
-#define QEMU_2T3D "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M "    \
-                  "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M "    \
-                  "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 " \
-                  "-object memory-backend-file,id=cxl-mem1,mem-path=%s,size=256M "    \
-                  "-object memory-backend-file,id=lsa1,mem-path=%s,size=256M "    \
-                  "-device cxl-type3,bus=rp1,memdev=cxl-mem1,lsa=lsa1,id=cxl-pmem1 "
-
-#define QEMU_4T3D "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
-                  "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M "    \
-                  "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 " \
-                  "-object memory-backend-file,id=cxl-mem1,mem-path=%s,size=256M "    \
-                  "-object memory-backend-file,id=lsa1,mem-path=%s,size=256M "    \
-                  "-device cxl-type3,bus=rp1,memdev=cxl-mem1,lsa=lsa1,id=cxl-pmem1 " \
-                  "-object memory-backend-file,id=cxl-mem2,mem-path=%s,size=256M "    \
-                  "-object memory-backend-file,id=lsa2,mem-path=%s,size=256M "    \
-                  "-device cxl-type3,bus=rp2,memdev=cxl-mem2,lsa=lsa2,id=cxl-pmem2 " \
-                  "-object memory-backend-file,id=cxl-mem3,mem-path=%s,size=256M "    \
-                  "-object memory-backend-file,id=lsa3,mem-path=%s,size=256M "    \
-                  "-device cxl-type3,bus=rp3,memdev=cxl-mem3,lsa=lsa3,id=cxl-pmem3 "
+#define QEMU_4RP \
+    "-device cxl-rp,id=rp0,bus=cxl.0,chassis=0,slot=0 " \
+    "-device cxl-rp,id=rp1,bus=cxl.0,chassis=0,slot=1 " \
+    "-device cxl-rp,id=rp2,bus=cxl.1,chassis=0,slot=2 " \
+    "-device cxl-rp,id=rp3,bus=cxl.1,chassis=0,slot=3 "
+
+#define QEMU_T3D \
+    "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
+    "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
+    "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 "
+
+#define QEMU_2T3D \
+    "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
+    "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
+    "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 " \
+    "-object memory-backend-file,id=cxl-mem1,mem-path=%s,size=256M " \
+    "-object memory-backend-file,id=lsa1,mem-path=%s,size=256M " \
+    "-device cxl-type3,bus=rp1,memdev=cxl-mem1,lsa=lsa1,id=cxl-pmem1 "
+
+#define QEMU_4T3D \
+    "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
+    "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
+    "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 " \
+    "-object memory-backend-file,id=cxl-mem1,mem-path=%s,size=256M " \
+    "-object memory-backend-file,id=lsa1,mem-path=%s,size=256M " \
+    "-device cxl-type3,bus=rp1,memdev=cxl-mem1,lsa=lsa1,id=cxl-pmem1 " \
+    "-object memory-backend-file,id=cxl-mem2,mem-path=%s,size=256M " \
+    "-object memory-backend-file,id=lsa2,mem-path=%s,size=256M " \
+    "-device cxl-type3,bus=rp2,memdev=cxl-mem2,lsa=lsa2,id=cxl-pmem2 " \
+    "-object memory-backend-file,id=cxl-mem3,mem-path=%s,size=256M " \
+    "-object memory-backend-file,id=lsa3,mem-path=%s,size=256M " \
+    "-device cxl-type3,bus=rp3,memdev=cxl-mem3,lsa=lsa3,id=cxl-pmem3 "
 
 static void cxl_basic_hb(void)
 {
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 1/2] tests/qtest/cxl-test: whitespace, line ending cleanup
@ 2023-01-31 16:38   ` Jonathan Cameron via
  0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron via @ 2023-01-31 16:38 UTC (permalink / raw)
  To: qemu-devel, Michael Tsirkin
  Cc: Ben Widawsky, linux-cxl, linuxarm, Ira Weiny, Gregory Price,
	Philippe Mathieu-Daudé,
	Mike Maslenkin

From: Gregory Price <gourry.memverge@gmail.com>

Defines are starting to exceed line length limits, align them for
cleanliness before making modifications.

Signed-off-by: Gregory Price <gregory.price@memverge.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
Changes since RFC v4:
  Naming consistency improvements.
 
 tests/qtest/cxl-test.c | 84 +++++++++++++++++++++++-------------------
 1 file changed, 46 insertions(+), 38 deletions(-)

diff --git a/tests/qtest/cxl-test.c b/tests/qtest/cxl-test.c
index 61f25a72b6..eda2bbbbe6 100644
--- a/tests/qtest/cxl-test.c
+++ b/tests/qtest/cxl-test.c
@@ -8,50 +8,58 @@
 #include "qemu/osdep.h"
 #include "libqtest-single.h"
 
-#define QEMU_PXB_CMD "-machine q35,cxl=on " \
-                     "-device pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=52 "  \
-                     "-M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.size=4G "
+#define QEMU_PXB_CMD \
+    "-machine q35,cxl=on " \
+    "-device pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=52 " \
+    "-M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.size=4G "
 
-#define QEMU_2PXB_CMD "-machine q35,cxl=on "                            \
-                      "-device pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=52 "  \
-                      "-device pxb-cxl,id=cxl.1,bus=pcie.0,bus_nr=53 " \
-                      "-M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.targets.1=cxl.1,cxl-fmw.0.size=4G "
+#define QEMU_2PXB_CMD \
+    "-machine q35,cxl=on " \
+    "-device pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=52 " \
+    "-device pxb-cxl,id=cxl.1,bus=pcie.0,bus_nr=53 " \
+    "-M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.targets.1=cxl.1,cxl-fmw.0.size=4G "
 
-#define QEMU_RP "-device cxl-rp,id=rp0,bus=cxl.0,chassis=0,slot=0 "
+#define QEMU_RP \
+    "-device cxl-rp,id=rp0,bus=cxl.0,chassis=0,slot=0 "
 
 /* Dual ports on first pxb */
-#define QEMU_2RP "-device cxl-rp,id=rp0,bus=cxl.0,chassis=0,slot=0 " \
-                 "-device cxl-rp,id=rp1,bus=cxl.0,chassis=0,slot=1 "
+#define QEMU_2RP \
+    "-device cxl-rp,id=rp0,bus=cxl.0,chassis=0,slot=0 " \
+    "-device cxl-rp,id=rp1,bus=cxl.0,chassis=0,slot=1 "
 
 /* Dual ports on each of the pxb instances */
-#define QEMU_4RP "-device cxl-rp,id=rp0,bus=cxl.0,chassis=0,slot=0 " \
-                 "-device cxl-rp,id=rp1,bus=cxl.0,chassis=0,slot=1 " \
-                 "-device cxl-rp,id=rp2,bus=cxl.1,chassis=0,slot=2 " \
-                 "-device cxl-rp,id=rp3,bus=cxl.1,chassis=0,slot=3 "
-
-#define QEMU_T3D "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
-                 "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M "    \
-                 "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 "
-
-#define QEMU_2T3D "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M "    \
-                  "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M "    \
-                  "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 " \
-                  "-object memory-backend-file,id=cxl-mem1,mem-path=%s,size=256M "    \
-                  "-object memory-backend-file,id=lsa1,mem-path=%s,size=256M "    \
-                  "-device cxl-type3,bus=rp1,memdev=cxl-mem1,lsa=lsa1,id=cxl-pmem1 "
-
-#define QEMU_4T3D "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
-                  "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M "    \
-                  "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 " \
-                  "-object memory-backend-file,id=cxl-mem1,mem-path=%s,size=256M "    \
-                  "-object memory-backend-file,id=lsa1,mem-path=%s,size=256M "    \
-                  "-device cxl-type3,bus=rp1,memdev=cxl-mem1,lsa=lsa1,id=cxl-pmem1 " \
-                  "-object memory-backend-file,id=cxl-mem2,mem-path=%s,size=256M "    \
-                  "-object memory-backend-file,id=lsa2,mem-path=%s,size=256M "    \
-                  "-device cxl-type3,bus=rp2,memdev=cxl-mem2,lsa=lsa2,id=cxl-pmem2 " \
-                  "-object memory-backend-file,id=cxl-mem3,mem-path=%s,size=256M "    \
-                  "-object memory-backend-file,id=lsa3,mem-path=%s,size=256M "    \
-                  "-device cxl-type3,bus=rp3,memdev=cxl-mem3,lsa=lsa3,id=cxl-pmem3 "
+#define QEMU_4RP \
+    "-device cxl-rp,id=rp0,bus=cxl.0,chassis=0,slot=0 " \
+    "-device cxl-rp,id=rp1,bus=cxl.0,chassis=0,slot=1 " \
+    "-device cxl-rp,id=rp2,bus=cxl.1,chassis=0,slot=2 " \
+    "-device cxl-rp,id=rp3,bus=cxl.1,chassis=0,slot=3 "
+
+#define QEMU_T3D \
+    "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
+    "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
+    "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 "
+
+#define QEMU_2T3D \
+    "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
+    "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
+    "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 " \
+    "-object memory-backend-file,id=cxl-mem1,mem-path=%s,size=256M " \
+    "-object memory-backend-file,id=lsa1,mem-path=%s,size=256M " \
+    "-device cxl-type3,bus=rp1,memdev=cxl-mem1,lsa=lsa1,id=cxl-pmem1 "
+
+#define QEMU_4T3D \
+    "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
+    "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
+    "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 " \
+    "-object memory-backend-file,id=cxl-mem1,mem-path=%s,size=256M " \
+    "-object memory-backend-file,id=lsa1,mem-path=%s,size=256M " \
+    "-device cxl-type3,bus=rp1,memdev=cxl-mem1,lsa=lsa1,id=cxl-pmem1 " \
+    "-object memory-backend-file,id=cxl-mem2,mem-path=%s,size=256M " \
+    "-object memory-backend-file,id=lsa2,mem-path=%s,size=256M " \
+    "-device cxl-type3,bus=rp2,memdev=cxl-mem2,lsa=lsa2,id=cxl-pmem2 " \
+    "-object memory-backend-file,id=cxl-mem3,mem-path=%s,size=256M " \
+    "-object memory-backend-file,id=lsa3,mem-path=%s,size=256M " \
+    "-device cxl-type3,bus=rp3,memdev=cxl-mem3,lsa=lsa3,id=cxl-pmem3 "
 
 static void cxl_basic_hb(void)
 {
-- 
2.37.2



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/2] hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent)
  2023-01-31 16:38 ` Jonathan Cameron via
@ 2023-01-31 16:38   ` Jonathan Cameron via
  -1 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2023-01-31 16:38 UTC (permalink / raw)
  To: qemu-devel, Michael Tsirkin
  Cc: Ben Widawsky, linux-cxl, linuxarm, Ira Weiny, Gregory Price,
	Philippe Mathieu-Daudé,
	Mike Maslenkin

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>

---
Chances since RFC V4:
- Fixed up issue reported by Gregory of volatile regions being reported
  as non volatile in CDAT.  Fix as suggested by Gregory.
- Report volatile memory as EFI_MEMORY_SP.
- Fixed some naming issues in tests including sticking to existing
  naming for memory device of cxl-memX throughout.
- Update the bios-tables-test.c test to use the new non deprecated
  parameter name. Note this has no impact on generated tables.
- Flipped logic of get_lsa_size() callback to slightly simplify change.
- cxl_setup_memory() reduce scope of local variables and name them
  to make it clear which are volatile and which are persistent related.

 docs/system/devices/cxl.rst    |  49 ++++--
 hw/cxl/cxl-mailbox-utils.c     |  26 +--
 hw/mem/cxl_type3.c             | 300 +++++++++++++++++++++++++--------
 include/hw/cxl/cxl_device.h    |  11 +-
 tests/qtest/bios-tables-test.c |   8 +-
 tests/qtest/cxl-test.c         |  76 +++++++--
 6 files changed, 359 insertions(+), 111 deletions(-)

diff --git a/docs/system/devices/cxl.rst b/docs/system/devices/cxl.rst
index f25783a4ec..89a41cff73 100644
--- a/docs/system/devices/cxl.rst
+++ b/docs/system/devices/cxl.rst
@@ -300,7 +300,7 @@ Example topology involving a switch::
 
 Example command lines
 ---------------------
-A very simple setup with just one directly attached CXL Type 3 device::
+A very simple setup with just one directly attached CXL Type 3 Persistent Memory device::
 
   qemu-system-aarch64 -M virt,gic-version=3,cxl=on -m 4g,maxmem=8G,slots=8 -cpu max \
   ...
@@ -308,7 +308,28 @@ A very simple setup with just one directly attached CXL Type 3 device::
   -object memory-backend-file,id=cxl-lsa1,share=on,mem-path=/tmp/lsa.raw,size=256M \
   -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
   -device cxl-rp,port=0,bus=cxl.1,id=root_port13,chassis=0,slot=2 \
-  -device cxl-type3,bus=root_port13,memdev=cxl-mem1,lsa=cxl-lsa1,id=cxl-pmem0 \
+  -device cxl-type3,bus=root_port13,persistent-memdev=cxl-mem1,lsa=cxl-lsa1,id=cxl-pmem0 \
+  -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.size=4G
+
+A very simple setup with just one directly attached CXL Type 3 Volatile Memory device::
+
+  qemu-system-aarch64 -M virt,gic-version=3,cxl=on -m 4g,maxmem=8G,slots=8 -cpu max \
+  ...
+  -object memory-backend-ram,id=vmem0,share=on,size=256M \
+  -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
+  -device cxl-rp,port=0,bus=cxl.1,id=root_port13,chassis=0,slot=2 \
+  -device cxl-type3,bus=root_port13,volatile-memdev=vmem0,id=cxl-vmem0 \
+  -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.size=4G
+
+The same volatile setup may optionally include an LSA region::
+
+  qemu-system-aarch64 -M virt,gic-version=3,cxl=on -m 4g,maxmem=8G,slots=8 -cpu max \
+  ...
+  -object memory-backend-ram,id=vmem0,share=on,size=256M \
+  -object memory-backend-file,id=cxl-lsa0,share=on,mem-path=/tmp/lsa.raw,size=256M \
+  -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
+  -device cxl-rp,port=0,bus=cxl.1,id=root_port13,chassis=0,slot=2 \
+  -device cxl-type3,bus=root_port13,volatile-memdev=vmem0,lsa=cxl-lsa0,id=cxl-vmem0 \
   -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.size=4G
 
 A setup suitable for 4 way interleave. Only one fixed window provided, to enable 2 way
@@ -328,13 +349,13 @@ the CXL Type3 device directly attached (no switches).::
   -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
   -device pxb-cxl,bus_nr=222,bus=pcie.0,id=cxl.2 \
   -device cxl-rp,port=0,bus=cxl.1,id=root_port13,chassis=0,slot=2 \
-  -device cxl-type3,bus=root_port13,memdev=cxl-mem1,lsa=cxl-lsa1,id=cxl-pmem0 \
+  -device cxl-type3,bus=root_port13,persistent-memdev=cxl-mem1,lsa=cxl-lsa1,id=cxl-pmem0 \
   -device cxl-rp,port=1,bus=cxl.1,id=root_port14,chassis=0,slot=3 \
-  -device cxl-type3,bus=root_port14,memdev=cxl-mem2,lsa=cxl-lsa2,id=cxl-pmem1 \
+  -device cxl-type3,bus=root_port14,persistent-memdev=cxl-mem2,lsa=cxl-lsa2,id=cxl-pmem1 \
   -device cxl-rp,port=0,bus=cxl.2,id=root_port15,chassis=0,slot=5 \
-  -device cxl-type3,bus=root_port15,memdev=cxl-mem3,lsa=cxl-lsa3,id=cxl-pmem2 \
+  -device cxl-type3,bus=root_port15,persistent-memdev=cxl-mem3,lsa=cxl-lsa3,id=cxl-pmem2 \
   -device cxl-rp,port=1,bus=cxl.2,id=root_port16,chassis=0,slot=6 \
-  -device cxl-type3,bus=root_port16,memdev=cxl-mem4,lsa=cxl-lsa4,id=cxl-pmem3 \
+  -device cxl-type3,bus=root_port16,persistent-memdev=cxl-mem4,lsa=cxl-lsa4,id=cxl-pmem3 \
   -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.targets.1=cxl.2,cxl-fmw.0.size=4G,cxl-fmw.0.interleave-granularity=8k
 
 An example of 4 devices below a switch suitable for 1, 2 or 4 way interleave::
@@ -354,15 +375,23 @@ An example of 4 devices below a switch suitable for 1, 2 or 4 way interleave::
   -device cxl-rp,port=1,bus=cxl.1,id=root_port1,chassis=0,slot=1 \
   -device cxl-upstream,bus=root_port0,id=us0 \
   -device cxl-downstream,port=0,bus=us0,id=swport0,chassis=0,slot=4 \
-  -device cxl-type3,bus=swport0,memdev=cxl-mem0,lsa=cxl-lsa0,id=cxl-pmem0,size=256M \
+  -device cxl-type3,bus=swport0,persistent-memdev=cxl-mem0,lsa=cxl-lsa0,id=cxl-pmem0,size=256M \
   -device cxl-downstream,port=1,bus=us0,id=swport1,chassis=0,slot=5 \
-  -device cxl-type3,bus=swport1,memdev=cxl-mem1,lsa=cxl-lsa1,id=cxl-pmem1,size=256M \
+  -device cxl-type3,bus=swport1,persistent-memdev=cxl-mem1,lsa=cxl-lsa1,id=cxl-pmem1,size=256M \
   -device cxl-downstream,port=2,bus=us0,id=swport2,chassis=0,slot=6 \
-  -device cxl-type3,bus=swport2,memdev=cxl-mem2,lsa=cxl-lsa2,id=cxl-pmem2,size=256M \
+  -device cxl-type3,bus=swport2,persistent-memdev=cxl-mem2,lsa=cxl-lsa2,id=cxl-pmem2,size=256M \
   -device cxl-downstream,port=3,bus=us0,id=swport3,chassis=0,slot=7 \
-  -device cxl-type3,bus=swport3,memdev=cxl-mem3,lsa=cxl-lsa3,id=cxl-pmem3,size=256M \
+  -device cxl-type3,bus=swport3,persistent-memdev=cxl-mem3,lsa=cxl-lsa3,id=cxl-pmem3,size=256M \
   -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.size=4G,cxl-fmw.0.interleave-granularity=4k
 
+Deprecations
+------------
+
+The Type 3 device [memdev] attribute has been deprecated in favor of the
+[persistent-memdev] attributes. [memdev] will default to a persistent memory
+device for backward compatibility and is incapable of being used in combination
+with [persistent-memdev].
+
 Kernel Configuration Options
 ----------------------------
 
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 206e04a4b8..cc9c8b7380 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -141,7 +141,8 @@ 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 < CXL_CAPACITY_MULTIPLIER) {
+    if ((cxl_dstate->vmem_size < CXL_CAPACITY_MULTIPLIER) ||
+        (cxl_dstate->pmem_size < CXL_CAPACITY_MULTIPLIER)) {
         return CXL_MBOX_INTERNAL_ERROR;
     }
 
@@ -288,20 +289,20 @@ 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, CXL_CAPACITY_MULTIPLIER)) {
+    if ((!QEMU_IS_ALIGNED(cxl_dstate->vmem_size, CXL_CAPACITY_MULTIPLIER)) ||
+        (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, CXL_CAPACITY_MULTIPLIER))) {
         return CXL_MBOX_INTERNAL_ERROR;
     }
 
     id = (void *)cmd->payload;
     memset(id, 0, sizeof(*id));
 
-    /* PMEM only */
     snprintf(id->fw_revision, 0x10, "BWFW VERSION %02d", 0);
 
-    id->total_capacity = size / CXL_CAPACITY_MULTIPLIER;
-    id->persistent_capacity = size / CXL_CAPACITY_MULTIPLIER;
+    id->total_capacity = cxl_dstate->mem_size / CXL_CAPACITY_MULTIPLIER;
+    id->persistent_capacity = cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER;
+    id->volatile_capacity = cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER;
     id->lsa_size = cvc->get_lsa_size(ct3d);
 
     *len = sizeof(*id);
@@ -319,16 +320,19 @@ 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, CXL_CAPACITY_MULTIPLIER)) {
+    if ((!QEMU_IS_ALIGNED(cxl_dstate->vmem_size, CXL_CAPACITY_MULTIPLIER)) ||
+        (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, CXL_CAPACITY_MULTIPLIER))) {
         return CXL_MBOX_INTERNAL_ERROR;
     }
 
-    /* PMEM only */
-    part_info->active_vmem = 0;
+    part_info->active_vmem = cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER;
+    /*
+     * When both next_vmem and next_pmem are 0, there is no pending change to
+     * partitioning.
+     */
     part_info->next_vmem = 0;
-    part_info->active_pmem = size / CXL_CAPACITY_MULTIPLIER;
+    part_info->active_pmem = cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER;
     part_info->next_pmem = 0;
 
     *len = sizeof(*part_info);
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index e32bbac966..beb931d59a 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -31,7 +31,8 @@ enum {
 };
 
 static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
-                                         int dsmad_handle, MemoryRegion *mr)
+                                         int dsmad_handle, MemoryRegion *mr,
+                                         bool is_pmem, uint64_t dpa_base)
 {
     g_autofree CDATDsmas *dsmas = NULL;
     g_autofree CDATDslbis *dslbis0 = NULL;
@@ -50,8 +51,8 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
             .length = sizeof(*dsmas),
         },
         .DSMADhandle = dsmad_handle,
-        .flags = CDAT_DSMAS_FLAG_NV,
-        .DPA_base = 0,
+        .flags = is_pmem ? CDAT_DSMAS_FLAG_NV : 0,
+        .DPA_base = dpa_base,
         .DPA_length = int128_get64(mr->size),
     };
 
@@ -130,8 +131,11 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
             .length = sizeof(*dsemts),
         },
         .DSMAS_handle = dsmad_handle,
-        /* Reserved - the non volatile from DSMAS matters */
-        .EFI_memory_type_attr = 2,
+        /*
+         * NV: Reserved - the non volatile from DSMAS matters
+         * V: EFI_MEMORY_SP
+         */
+        .EFI_memory_type_attr = is_pmem ? 2 : 1,
         .DPA_offset = 0,
         .DPA_length = int128_get64(mr->size),
     };
@@ -150,33 +154,66 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
 static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
 {
     g_autofree CDATSubHeader **table = NULL;
-    MemoryRegion *nonvolatile_mr;
     CXLType3Dev *ct3d = priv;
+    MemoryRegion *volatile_mr = NULL, *nonvolatile_mr = NULL;
     int dsmad_handle = 0;
-    int rc;
+    int cur_ent = 0;
+    int len = 0;
+    int rc, i;
 
-    if (!ct3d->hostmem) {
+    if (!ct3d->hostpmem && !ct3d->hostvmem) {
         return 0;
     }
 
-    nonvolatile_mr = host_memory_backend_get_memory(ct3d->hostmem);
-    if (!nonvolatile_mr) {
-        return -EINVAL;
+    if (ct3d->hostvmem) {
+        volatile_mr = host_memory_backend_get_memory(ct3d->hostvmem);
+        if (!volatile_mr) {
+            return -EINVAL;
+        }
+        len += CT3_CDAT_NUM_ENTRIES;
+    }
+
+    if (ct3d->hostpmem) {
+        nonvolatile_mr = host_memory_backend_get_memory(ct3d->hostpmem);
+        if (!nonvolatile_mr) {
+            return -EINVAL;
+        }
+        len += CT3_CDAT_NUM_ENTRIES;
     }
 
-    table = g_malloc0(CT3_CDAT_NUM_ENTRIES * sizeof(*table));
+    table = g_malloc0(len * sizeof(*table));
     if (!table) {
         return -ENOMEM;
     }
 
-    rc = ct3_build_cdat_entries_for_mr(table, dsmad_handle++, nonvolatile_mr);
-    if (rc < 0) {
-        return rc;
+    /* Now fill them in */
+    if (volatile_mr) {
+        rc = ct3_build_cdat_entries_for_mr(table, dsmad_handle++, volatile_mr,
+                                           false, 0);
+        if (rc < 0) {
+            return rc;
+        }
+        cur_ent = CT3_CDAT_NUM_ENTRIES;
     }
 
+    if (nonvolatile_mr) {
+        rc = ct3_build_cdat_entries_for_mr(&(table[cur_ent]), dsmad_handle++,
+                nonvolatile_mr, true, (volatile_mr ? volatile_mr->size : 0));
+        if (rc < 0) {
+            goto error_cleanup;
+        }
+        cur_ent += CT3_CDAT_NUM_ENTRIES;
+    }
+    assert(len == cur_ent);
+
     *cdat_table = g_steal_pointer(&table);
 
-    return CT3_CDAT_NUM_ENTRIES;
+    return len;
+error_cleanup:
+    for (i = 0; i < cur_ent; i++) {
+        g_free(table[i]);
+    }
+    return rc;
 }
 
 static void ct3_free_cdat_table(CDATSubHeader **cdat_table, int num, void *priv)
@@ -264,16 +301,48 @@ static void build_dvsecs(CXLType3Dev *ct3d)
 {
     CXLComponentState *cxl_cstate = &ct3d->cxl_cstate;
     uint8_t *dvsec;
+    uint32_t range1_size_hi, range1_size_lo,
+             range1_base_hi, range1_base_lo,
+             range2_size_hi = 0, range2_size_lo = 0,
+             range2_base_hi = 0, range2_base_lo = 0;
+
+    /*
+     * Volatile memory is mapped as (0x0)
+     * Persistent memory is mapped at (volatile->size)
+     */
+    if (ct3d->hostvmem) {
+        range1_size_hi = ct3d->hostvmem->size >> 32;
+        range1_size_lo = (2 << 5) | (2 << 2) | 0x3 |
+                         (ct3d->hostvmem->size & 0xF0000000);
+        range1_base_hi = 0;
+        range1_base_lo = 0;
+        if (ct3d->hostpmem) {
+            range2_size_hi = ct3d->hostpmem->size >> 32;
+            range2_size_lo = (2 << 5) | (2 << 2) | 0x3 |
+                             (ct3d->hostpmem->size & 0xF0000000);
+            range2_base_hi = ct3d->hostvmem->size >> 32;
+            range2_base_lo = ct3d->hostvmem->size & 0xF0000000;
+        }
+    } else {
+        range1_size_hi = ct3d->hostpmem->size >> 32;
+        range1_size_lo = (2 << 5) | (2 << 2) | 0x3 |
+                         (ct3d->hostpmem->size & 0xF0000000);
+        range1_base_hi = 0;
+        range1_base_lo = 0;
+    }
 
     dvsec = (uint8_t *)&(CXLDVSECDevice){
         .cap = 0x1e,
         .ctrl = 0x2,
         .status2 = 0x2,
-        .range1_size_hi = ct3d->hostmem->size >> 32,
-        .range1_size_lo = (2 << 5) | (2 << 2) | 0x3 |
-        (ct3d->hostmem->size & 0xF0000000),
-        .range1_base_hi = 0,
-        .range1_base_lo = 0,
+        .range1_size_hi = range1_size_hi,
+        .range1_size_lo = range1_size_lo,
+        .range1_base_hi = range1_base_hi,
+        .range1_base_lo = range1_base_lo,
+        .range2_size_hi = range2_size_hi,
+        .range2_size_lo = range2_size_lo,
+        .range2_base_hi = range2_base_hi,
+        .range2_base_lo = range2_base_lo,
     };
     cxl_component_create_dvsec(cxl_cstate, CXL2_TYPE3_DEVICE,
                                PCIE_CXL_DEVICE_DVSEC_LENGTH,
@@ -492,36 +561,69 @@ 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;
 
-    if (!ct3d->hostmem) {
-        error_setg(errp, "memdev property must be set");
+    if (!ct3d->hostmem && !ct3d->hostvmem && !ct3d->hostpmem) {
+        error_setg(errp, "at least one memdev property must be set");
         return false;
+    } else if (ct3d->hostmem && ct3d->hostpmem) {
+        error_setg(errp, "[memdev] cannot be used with new "
+                         "[persistent-memdev] property");
+        return false;
+    } else if (ct3d->hostmem) {
+        /* Use of hostmem property implies pmem */
+        ct3d->hostpmem = ct3d->hostmem;
+        ct3d->hostmem = NULL;
     }
 
-    mr = host_memory_backend_get_memory(ct3d->hostmem);
-    if (!mr) {
-        error_setg(errp, "memdev property must be set");
+    if (ct3d->hostpmem && !ct3d->lsa) {
+        error_setg(errp, "lsa property must be set for persistent devices");
         return false;
     }
-    memory_region_set_nonvolatile(mr, true);
-    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");
+    if (ct3d->hostvmem) {
+        MemoryRegion *vmr;
+        char *v_name;
+
+        vmr = host_memory_backend_get_memory(ct3d->hostvmem);
+        if (!vmr) {
+            error_setg(errp, "volatile memdev must have backing device");
+            return false;
+        }
+        memory_region_set_nonvolatile(vmr, false);
+        memory_region_set_enabled(vmr, true);
+        host_memory_backend_set_mapped(ct3d->hostvmem, true);
+        if (ds->id) {
+            v_name = g_strdup_printf("cxl-type3-dpa-vmem-space:%s", ds->id);
+        } else {
+            v_name = g_strdup("cxl-type3-dpa-vmem-space");
+        }
+        address_space_init(&ct3d->hostvmem_as, vmr, v_name);
+        ct3d->cxl_dstate.vmem_size = vmr->size;
+        ct3d->cxl_dstate.mem_size += vmr->size;
+        g_free(v_name);
     }
-    address_space_init(&ct3d->hostmem_as, mr, name);
-    g_free(name);
 
-    ct3d->cxl_dstate.pmem_size = ct3d->hostmem->size;
+    if (ct3d->hostpmem) {
+        MemoryRegion *pmr;
+        char *p_name;
 
-    if (!ct3d->lsa) {
-        error_setg(errp, "lsa property must be set");
-        return false;
+        pmr = host_memory_backend_get_memory(ct3d->hostpmem);
+        if (!pmr) {
+            error_setg(errp, "persistent memdev must have backing device");
+            return false;
+        }
+        memory_region_set_nonvolatile(pmr, true);
+        memory_region_set_enabled(pmr, true);
+        host_memory_backend_set_mapped(ct3d->hostpmem, true);
+        if (ds->id) {
+            p_name = g_strdup_printf("cxl-type3-dpa-pmem-space:%s", ds->id);
+        } else {
+            p_name = g_strdup("cxl-type3-dpa-pmem-space");
+        }
+        address_space_init(&ct3d->hostpmem_as, pmr, p_name);
+        ct3d->cxl_dstate.pmem_size = pmr->size;
+        ct3d->cxl_dstate.mem_size += pmr->size;
+        g_free(p_name);
     }
 
     return true;
@@ -607,7 +709,12 @@ err_release_cdat:
     cxl_doe_cdat_release(cxl_cstate);
     g_free(regs->special_ops);
 err_address_space_free:
-    address_space_destroy(&ct3d->hostmem_as);
+    if (ct3d->hostpmem) {
+        address_space_destroy(&ct3d->hostpmem_as);
+    }
+    if (ct3d->hostvmem) {
+        address_space_destroy(&ct3d->hostvmem_as);
+    }
     return;
 }
 
@@ -620,7 +727,12 @@ static void ct3_exit(PCIDevice *pci_dev)
     pcie_aer_exit(pci_dev);
     cxl_doe_cdat_release(cxl_cstate);
     g_free(regs->special_ops);
-    address_space_destroy(&ct3d->hostmem_as);
+    if (ct3d->hostpmem) {
+        address_space_destroy(&ct3d->hostpmem_as);
+    }
+    if (ct3d->hostvmem) {
+        address_space_destroy(&ct3d->hostvmem_as);
+    }
 }
 
 /* TODO: Support multiple HDM decoders and DPA skip */
@@ -655,51 +767,77 @@ static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t *dpa)
     return true;
 }
 
-MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
-                           unsigned size, MemTxAttrs attrs)
+static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d,
+                                       hwaddr host_addr,
+                                       unsigned int size,
+                                       AddressSpace **as,
+                                       uint64_t *dpa_offset)
 {
-    CXLType3Dev *ct3d = CXL_TYPE3(d);
-    uint64_t dpa_offset;
-    MemoryRegion *mr;
+    MemoryRegion *vmr = NULL, *pmr = NULL;
 
-    /* TODO support volatile region */
-    mr = host_memory_backend_get_memory(ct3d->hostmem);
-    if (!mr) {
-        return MEMTX_ERROR;
+    if (ct3d->hostvmem) {
+        vmr = host_memory_backend_get_memory(ct3d->hostvmem);
+    }
+    if (ct3d->hostpmem) {
+        pmr = host_memory_backend_get_memory(ct3d->hostpmem);
     }
 
-    if (!cxl_type3_dpa(ct3d, host_addr, &dpa_offset)) {
-        return MEMTX_ERROR;
+    if (!vmr && !pmr) {
+        return -ENODEV;
+    }
+
+    if (!cxl_type3_dpa(ct3d, host_addr, dpa_offset)) {
+        return -EINVAL;
+    }
+
+    if (*dpa_offset > int128_get64(ct3d->cxl_dstate.mem_size)) {
+        return -EINVAL;
+    }
+
+    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;
     }
 
-    if (dpa_offset > int128_get64(mr->size)) {
+    return 0;
+}
+
+MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
+                           unsigned size, MemTxAttrs attrs)
+{
+    uint64_t dpa_offset = 0;
+    AddressSpace *as = NULL;
+    int res;
+
+    res = cxl_type3_hpa_to_as_and_dpa(CXL_TYPE3(d), host_addr, size,
+                                      &as, &dpa_offset);
+    if (res) {
         return MEMTX_ERROR;
     }
 
-    return address_space_read(&ct3d->hostmem_as, dpa_offset, attrs, data, size);
+    return address_space_read(as, dpa_offset, attrs, data, size);
 }
 
 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;
-
-    mr = host_memory_backend_get_memory(ct3d->hostmem);
-    if (!mr) {
-        return MEMTX_OK;
-    }
+    uint64_t dpa_offset = 0;
+    AddressSpace *as = NULL;
+    int res;
 
-    if (!cxl_type3_dpa(ct3d, host_addr, &dpa_offset)) {
-        return MEMTX_OK;
+    res = cxl_type3_hpa_to_as_and_dpa(CXL_TYPE3(d), host_addr, size,
+                                      &as, &dpa_offset);
+    if (res) {
+        return MEMTX_ERROR;
     }
 
-    if (dpa_offset > int128_get64(mr->size)) {
-        return MEMTX_OK;
-    }
-    return address_space_write(&ct3d->hostmem_as, dpa_offset, attrs,
-                               &data, size);
+    return address_space_write(as, dpa_offset, attrs, &data, size);
 }
 
 static void ct3d_reset(DeviceState *dev)
@@ -714,7 +852,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),
@@ -726,6 +868,10 @@ static uint64_t get_lsa_size(CXLType3Dev *ct3d)
 {
     MemoryRegion *mr;
 
+    if (!ct3d->lsa) {
+        return 0;
+    }
+
     mr = host_memory_backend_get_memory(ct3d->lsa);
     return memory_region_size(mr);
 }
@@ -743,6 +889,10 @@ static uint64_t get_lsa(CXLType3Dev *ct3d, void *buf, uint64_t size,
     MemoryRegion *mr;
     void *lsa;
 
+    if (!ct3d->lsa) {
+        return 0;
+    }
+
     mr = host_memory_backend_get_memory(ct3d->lsa);
     validate_lsa_access(mr, size, offset);
 
@@ -758,6 +908,10 @@ static void set_lsa(CXLType3Dev *ct3d, const void *buf, uint64_t size,
     MemoryRegion *mr;
     void *lsa;
 
+    if (!ct3d->lsa) {
+        return;
+    }
+
     mr = host_memory_backend_get_memory(ct3d->lsa);
     validate_lsa_access(mr, size, offset);
 
@@ -929,7 +1083,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);
 
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index d589f78202..edb9791bab 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -119,8 +119,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 */
@@ -245,12 +247,15 @@ struct CXLType3Dev {
     PCIDevice parent_obj;
 
     /* Properties */
-    HostMemoryBackend *hostmem;
+    HostMemoryBackend *hostmem; /* deprecated */
+    HostMemoryBackend *hostvmem;
+    HostMemoryBackend *hostpmem;
     HostMemoryBackend *lsa;
     uint64_t sn;
 
     /* State */
-    AddressSpace hostmem_as;
+    AddressSpace hostvmem_as;
+    AddressSpace hostpmem_as;
     CXLComponentState cxl_cstate;
     CXLDeviceState cxl_dstate;
 
diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 8608408213..b005c03a92 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -1789,13 +1789,13 @@ static void test_acpi_q35_cxl(void)
                              " -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1"
                              " -device pxb-cxl,bus_nr=222,bus=pcie.0,id=cxl.2"
                              " -device cxl-rp,port=0,bus=cxl.1,id=rp1,chassis=0,slot=2"
-                             " -device cxl-type3,bus=rp1,memdev=cxl-mem1,lsa=lsa1"
+                             " -device cxl-type3,bus=rp1,persistent-memdev=cxl-mem1,lsa=lsa1"
                              " -device cxl-rp,port=1,bus=cxl.1,id=rp2,chassis=0,slot=3"
-                             " -device cxl-type3,bus=rp2,memdev=cxl-mem2,lsa=lsa2"
+                             " -device cxl-type3,bus=rp2,persistent-memdev=cxl-mem2,lsa=lsa2"
                              " -device cxl-rp,port=0,bus=cxl.2,id=rp3,chassis=0,slot=5"
-                             " -device cxl-type3,bus=rp3,memdev=cxl-mem3,lsa=lsa3"
+                             " -device cxl-type3,bus=rp3,persistent-memdev=cxl-mem3,lsa=lsa3"
                              " -device cxl-rp,port=1,bus=cxl.2,id=rp4,chassis=0,slot=6"
-                             " -device cxl-type3,bus=rp4,memdev=cxl-mem4,lsa=lsa4"
+                             " -device cxl-type3,bus=rp4,persistent-memdev=cxl-mem4,lsa=lsa4"
                              " -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.size=4G,cxl-fmw.0.interleave-granularity=8k,"
                              "cxl-fmw.1.targets.0=cxl.1,cxl-fmw.1.targets.1=cxl.2,cxl-fmw.1.size=4G,cxl-fmw.1.interleave-granularity=8k",
                              tmp_path, tmp_path, tmp_path, tmp_path,
diff --git a/tests/qtest/cxl-test.c b/tests/qtest/cxl-test.c
index eda2bbbbe6..edcad4a0ce 100644
--- a/tests/qtest/cxl-test.c
+++ b/tests/qtest/cxl-test.c
@@ -34,32 +34,46 @@
     "-device cxl-rp,id=rp2,bus=cxl.1,chassis=0,slot=2 " \
     "-device cxl-rp,id=rp3,bus=cxl.1,chassis=0,slot=3 "
 
-#define QEMU_T3D \
+#define QEMU_T3D_DEPRECATED \
     "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
     "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
     "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 "
 
+#define QEMU_T3D_PMEM \
+    "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
+    "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
+    "-device cxl-type3,bus=rp0,persistent-memdev=cxl-mem0,lsa=lsa0,id=pmem0 "
+
+#define QEMU_T3D_VMEM \
+    "-object memory-backend-ram,id=cxl-mem0,size=256M " \
+    "-device cxl-type3,bus=rp0,volatile-memdev=cxl-mem0,id=mem0 "
+
+#define QEMU_T3D_VMEM_LSA \
+    "-object memory-backend-ram,id=cxl-mem0,size=256M " \
+    "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
+    "-device cxl-type3,bus=rp0,volatile-memdev=cxl-mem0,lsa=lsa0,id=mem0 "
+
 #define QEMU_2T3D \
     "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
     "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
-    "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 " \
+    "-device cxl-type3,bus=rp0,persistent-memdev=cxl-mem0,lsa=lsa0,id=pmem0 " \
     "-object memory-backend-file,id=cxl-mem1,mem-path=%s,size=256M " \
     "-object memory-backend-file,id=lsa1,mem-path=%s,size=256M " \
-    "-device cxl-type3,bus=rp1,memdev=cxl-mem1,lsa=lsa1,id=cxl-pmem1 "
+    "-device cxl-type3,bus=rp1,persistent-memdev=cxl-mem1,lsa=lsa1,id=pmem1 "
 
 #define QEMU_4T3D \
     "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
     "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
-    "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 " \
+    "-device cxl-type3,bus=rp0,persistent-memdev=cxl-mem0,lsa=lsa0,id=pmem0 " \
     "-object memory-backend-file,id=cxl-mem1,mem-path=%s,size=256M " \
     "-object memory-backend-file,id=lsa1,mem-path=%s,size=256M " \
-    "-device cxl-type3,bus=rp1,memdev=cxl-mem1,lsa=lsa1,id=cxl-pmem1 " \
+    "-device cxl-type3,bus=rp1,persistent-memdev=cxl-mem1,lsa=lsa1,id=pmem1 " \
     "-object memory-backend-file,id=cxl-mem2,mem-path=%s,size=256M " \
     "-object memory-backend-file,id=lsa2,mem-path=%s,size=256M " \
-    "-device cxl-type3,bus=rp2,memdev=cxl-mem2,lsa=lsa2,id=cxl-pmem2 " \
+    "-device cxl-type3,bus=rp2,persistent-memdev=cxl-mem2,lsa=lsa2,id=pmem2 " \
     "-object memory-backend-file,id=cxl-mem3,mem-path=%s,size=256M " \
     "-object memory-backend-file,id=lsa3,mem-path=%s,size=256M " \
-    "-device cxl-type3,bus=rp3,memdev=cxl-mem3,lsa=lsa3,id=cxl-pmem3 "
+    "-device cxl-type3,bus=rp3,persistent-memdev=cxl-mem3,lsa=lsa3,id=pmem3 "
 
 static void cxl_basic_hb(void)
 {
@@ -98,14 +112,53 @@ static void cxl_2root_port(void)
 }
 
 #ifdef CONFIG_POSIX
-static void cxl_t3d(void)
+static void cxl_t3d_deprecated(void)
+{
+    g_autoptr(GString) cmdline = g_string_new(NULL);
+    g_autofree const char *tmpfs = NULL;
+
+    tmpfs = g_dir_make_tmp("cxl-test-XXXXXX", NULL);
+
+    g_string_printf(cmdline, QEMU_PXB_CMD QEMU_RP QEMU_T3D_DEPRECATED,
+                    tmpfs, tmpfs);
+
+    qtest_start(cmdline->str);
+    qtest_end();
+}
+
+static void cxl_t3d_persistent(void)
+{
+    g_autoptr(GString) cmdline = g_string_new(NULL);
+    g_autofree const char *tmpfs = NULL;
+
+    tmpfs = g_dir_make_tmp("cxl-test-XXXXXX", NULL);
+
+    g_string_printf(cmdline, QEMU_PXB_CMD QEMU_RP QEMU_T3D_PMEM,
+                    tmpfs, tmpfs);
+
+    qtest_start(cmdline->str);
+    qtest_end();
+}
+
+static void cxl_t3d_volatile(void)
+{
+    g_autoptr(GString) cmdline = g_string_new(NULL);
+
+    g_string_printf(cmdline, QEMU_PXB_CMD QEMU_RP QEMU_T3D_VMEM);
+
+    qtest_start(cmdline->str);
+    qtest_end();
+}
+
+static void cxl_t3d_volatile_lsa(void)
 {
     g_autoptr(GString) cmdline = g_string_new(NULL);
     g_autofree const char *tmpfs = NULL;
 
     tmpfs = g_dir_make_tmp("cxl-test-XXXXXX", NULL);
 
-    g_string_printf(cmdline, QEMU_PXB_CMD QEMU_RP QEMU_T3D, tmpfs, tmpfs);
+    g_string_printf(cmdline, QEMU_PXB_CMD QEMU_RP QEMU_T3D_VMEM_LSA,
+                    tmpfs);
 
     qtest_start(cmdline->str);
     qtest_end();
@@ -155,7 +208,10 @@ int main(int argc, char **argv)
     qtest_add_func("/pci/cxl/rp", cxl_root_port);
     qtest_add_func("/pci/cxl/rp_x2", cxl_2root_port);
 #ifdef CONFIG_POSIX
-    qtest_add_func("/pci/cxl/type3_device", cxl_t3d);
+    qtest_add_func("/pci/cxl/type3_device", cxl_t3d_deprecated);
+    qtest_add_func("/pci/cxl/type3_device_pmem", cxl_t3d_persistent);
+    qtest_add_func("/pci/cxl/type3_device_vmem", cxl_t3d_volatile);
+    qtest_add_func("/pci/cxl/type3_device_vmem_lsa", cxl_t3d_volatile_lsa);
     qtest_add_func("/pci/cxl/rp_x2_type3_x2", cxl_1pxb_2rp_2t3d);
     qtest_add_func("/pci/cxl/pxb_x2_root_port_x4_type3_x4", cxl_2pxb_4rp_4t3d);
 #endif
-- 
2.37.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/2] hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent)
@ 2023-01-31 16:38   ` Jonathan Cameron via
  0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron via @ 2023-01-31 16:38 UTC (permalink / raw)
  To: qemu-devel, Michael Tsirkin
  Cc: Ben Widawsky, linux-cxl, linuxarm, Ira Weiny, Gregory Price,
	Philippe Mathieu-Daudé,
	Mike Maslenkin

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>

---
Chances since RFC V4:
- Fixed up issue reported by Gregory of volatile regions being reported
  as non volatile in CDAT.  Fix as suggested by Gregory.
- Report volatile memory as EFI_MEMORY_SP.
- Fixed some naming issues in tests including sticking to existing
  naming for memory device of cxl-memX throughout.
- Update the bios-tables-test.c test to use the new non deprecated
  parameter name. Note this has no impact on generated tables.
- Flipped logic of get_lsa_size() callback to slightly simplify change.
- cxl_setup_memory() reduce scope of local variables and name them
  to make it clear which are volatile and which are persistent related.

 docs/system/devices/cxl.rst    |  49 ++++--
 hw/cxl/cxl-mailbox-utils.c     |  26 +--
 hw/mem/cxl_type3.c             | 300 +++++++++++++++++++++++++--------
 include/hw/cxl/cxl_device.h    |  11 +-
 tests/qtest/bios-tables-test.c |   8 +-
 tests/qtest/cxl-test.c         |  76 +++++++--
 6 files changed, 359 insertions(+), 111 deletions(-)

diff --git a/docs/system/devices/cxl.rst b/docs/system/devices/cxl.rst
index f25783a4ec..89a41cff73 100644
--- a/docs/system/devices/cxl.rst
+++ b/docs/system/devices/cxl.rst
@@ -300,7 +300,7 @@ Example topology involving a switch::
 
 Example command lines
 ---------------------
-A very simple setup with just one directly attached CXL Type 3 device::
+A very simple setup with just one directly attached CXL Type 3 Persistent Memory device::
 
   qemu-system-aarch64 -M virt,gic-version=3,cxl=on -m 4g,maxmem=8G,slots=8 -cpu max \
   ...
@@ -308,7 +308,28 @@ A very simple setup with just one directly attached CXL Type 3 device::
   -object memory-backend-file,id=cxl-lsa1,share=on,mem-path=/tmp/lsa.raw,size=256M \
   -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
   -device cxl-rp,port=0,bus=cxl.1,id=root_port13,chassis=0,slot=2 \
-  -device cxl-type3,bus=root_port13,memdev=cxl-mem1,lsa=cxl-lsa1,id=cxl-pmem0 \
+  -device cxl-type3,bus=root_port13,persistent-memdev=cxl-mem1,lsa=cxl-lsa1,id=cxl-pmem0 \
+  -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.size=4G
+
+A very simple setup with just one directly attached CXL Type 3 Volatile Memory device::
+
+  qemu-system-aarch64 -M virt,gic-version=3,cxl=on -m 4g,maxmem=8G,slots=8 -cpu max \
+  ...
+  -object memory-backend-ram,id=vmem0,share=on,size=256M \
+  -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
+  -device cxl-rp,port=0,bus=cxl.1,id=root_port13,chassis=0,slot=2 \
+  -device cxl-type3,bus=root_port13,volatile-memdev=vmem0,id=cxl-vmem0 \
+  -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.size=4G
+
+The same volatile setup may optionally include an LSA region::
+
+  qemu-system-aarch64 -M virt,gic-version=3,cxl=on -m 4g,maxmem=8G,slots=8 -cpu max \
+  ...
+  -object memory-backend-ram,id=vmem0,share=on,size=256M \
+  -object memory-backend-file,id=cxl-lsa0,share=on,mem-path=/tmp/lsa.raw,size=256M \
+  -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
+  -device cxl-rp,port=0,bus=cxl.1,id=root_port13,chassis=0,slot=2 \
+  -device cxl-type3,bus=root_port13,volatile-memdev=vmem0,lsa=cxl-lsa0,id=cxl-vmem0 \
   -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.size=4G
 
 A setup suitable for 4 way interleave. Only one fixed window provided, to enable 2 way
@@ -328,13 +349,13 @@ the CXL Type3 device directly attached (no switches).::
   -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
   -device pxb-cxl,bus_nr=222,bus=pcie.0,id=cxl.2 \
   -device cxl-rp,port=0,bus=cxl.1,id=root_port13,chassis=0,slot=2 \
-  -device cxl-type3,bus=root_port13,memdev=cxl-mem1,lsa=cxl-lsa1,id=cxl-pmem0 \
+  -device cxl-type3,bus=root_port13,persistent-memdev=cxl-mem1,lsa=cxl-lsa1,id=cxl-pmem0 \
   -device cxl-rp,port=1,bus=cxl.1,id=root_port14,chassis=0,slot=3 \
-  -device cxl-type3,bus=root_port14,memdev=cxl-mem2,lsa=cxl-lsa2,id=cxl-pmem1 \
+  -device cxl-type3,bus=root_port14,persistent-memdev=cxl-mem2,lsa=cxl-lsa2,id=cxl-pmem1 \
   -device cxl-rp,port=0,bus=cxl.2,id=root_port15,chassis=0,slot=5 \
-  -device cxl-type3,bus=root_port15,memdev=cxl-mem3,lsa=cxl-lsa3,id=cxl-pmem2 \
+  -device cxl-type3,bus=root_port15,persistent-memdev=cxl-mem3,lsa=cxl-lsa3,id=cxl-pmem2 \
   -device cxl-rp,port=1,bus=cxl.2,id=root_port16,chassis=0,slot=6 \
-  -device cxl-type3,bus=root_port16,memdev=cxl-mem4,lsa=cxl-lsa4,id=cxl-pmem3 \
+  -device cxl-type3,bus=root_port16,persistent-memdev=cxl-mem4,lsa=cxl-lsa4,id=cxl-pmem3 \
   -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.targets.1=cxl.2,cxl-fmw.0.size=4G,cxl-fmw.0.interleave-granularity=8k
 
 An example of 4 devices below a switch suitable for 1, 2 or 4 way interleave::
@@ -354,15 +375,23 @@ An example of 4 devices below a switch suitable for 1, 2 or 4 way interleave::
   -device cxl-rp,port=1,bus=cxl.1,id=root_port1,chassis=0,slot=1 \
   -device cxl-upstream,bus=root_port0,id=us0 \
   -device cxl-downstream,port=0,bus=us0,id=swport0,chassis=0,slot=4 \
-  -device cxl-type3,bus=swport0,memdev=cxl-mem0,lsa=cxl-lsa0,id=cxl-pmem0,size=256M \
+  -device cxl-type3,bus=swport0,persistent-memdev=cxl-mem0,lsa=cxl-lsa0,id=cxl-pmem0,size=256M \
   -device cxl-downstream,port=1,bus=us0,id=swport1,chassis=0,slot=5 \
-  -device cxl-type3,bus=swport1,memdev=cxl-mem1,lsa=cxl-lsa1,id=cxl-pmem1,size=256M \
+  -device cxl-type3,bus=swport1,persistent-memdev=cxl-mem1,lsa=cxl-lsa1,id=cxl-pmem1,size=256M \
   -device cxl-downstream,port=2,bus=us0,id=swport2,chassis=0,slot=6 \
-  -device cxl-type3,bus=swport2,memdev=cxl-mem2,lsa=cxl-lsa2,id=cxl-pmem2,size=256M \
+  -device cxl-type3,bus=swport2,persistent-memdev=cxl-mem2,lsa=cxl-lsa2,id=cxl-pmem2,size=256M \
   -device cxl-downstream,port=3,bus=us0,id=swport3,chassis=0,slot=7 \
-  -device cxl-type3,bus=swport3,memdev=cxl-mem3,lsa=cxl-lsa3,id=cxl-pmem3,size=256M \
+  -device cxl-type3,bus=swport3,persistent-memdev=cxl-mem3,lsa=cxl-lsa3,id=cxl-pmem3,size=256M \
   -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.size=4G,cxl-fmw.0.interleave-granularity=4k
 
+Deprecations
+------------
+
+The Type 3 device [memdev] attribute has been deprecated in favor of the
+[persistent-memdev] attributes. [memdev] will default to a persistent memory
+device for backward compatibility and is incapable of being used in combination
+with [persistent-memdev].
+
 Kernel Configuration Options
 ----------------------------
 
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 206e04a4b8..cc9c8b7380 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -141,7 +141,8 @@ 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 < CXL_CAPACITY_MULTIPLIER) {
+    if ((cxl_dstate->vmem_size < CXL_CAPACITY_MULTIPLIER) ||
+        (cxl_dstate->pmem_size < CXL_CAPACITY_MULTIPLIER)) {
         return CXL_MBOX_INTERNAL_ERROR;
     }
 
@@ -288,20 +289,20 @@ 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, CXL_CAPACITY_MULTIPLIER)) {
+    if ((!QEMU_IS_ALIGNED(cxl_dstate->vmem_size, CXL_CAPACITY_MULTIPLIER)) ||
+        (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, CXL_CAPACITY_MULTIPLIER))) {
         return CXL_MBOX_INTERNAL_ERROR;
     }
 
     id = (void *)cmd->payload;
     memset(id, 0, sizeof(*id));
 
-    /* PMEM only */
     snprintf(id->fw_revision, 0x10, "BWFW VERSION %02d", 0);
 
-    id->total_capacity = size / CXL_CAPACITY_MULTIPLIER;
-    id->persistent_capacity = size / CXL_CAPACITY_MULTIPLIER;
+    id->total_capacity = cxl_dstate->mem_size / CXL_CAPACITY_MULTIPLIER;
+    id->persistent_capacity = cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER;
+    id->volatile_capacity = cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER;
     id->lsa_size = cvc->get_lsa_size(ct3d);
 
     *len = sizeof(*id);
@@ -319,16 +320,19 @@ 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, CXL_CAPACITY_MULTIPLIER)) {
+    if ((!QEMU_IS_ALIGNED(cxl_dstate->vmem_size, CXL_CAPACITY_MULTIPLIER)) ||
+        (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, CXL_CAPACITY_MULTIPLIER))) {
         return CXL_MBOX_INTERNAL_ERROR;
     }
 
-    /* PMEM only */
-    part_info->active_vmem = 0;
+    part_info->active_vmem = cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER;
+    /*
+     * When both next_vmem and next_pmem are 0, there is no pending change to
+     * partitioning.
+     */
     part_info->next_vmem = 0;
-    part_info->active_pmem = size / CXL_CAPACITY_MULTIPLIER;
+    part_info->active_pmem = cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER;
     part_info->next_pmem = 0;
 
     *len = sizeof(*part_info);
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index e32bbac966..beb931d59a 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -31,7 +31,8 @@ enum {
 };
 
 static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
-                                         int dsmad_handle, MemoryRegion *mr)
+                                         int dsmad_handle, MemoryRegion *mr,
+                                         bool is_pmem, uint64_t dpa_base)
 {
     g_autofree CDATDsmas *dsmas = NULL;
     g_autofree CDATDslbis *dslbis0 = NULL;
@@ -50,8 +51,8 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
             .length = sizeof(*dsmas),
         },
         .DSMADhandle = dsmad_handle,
-        .flags = CDAT_DSMAS_FLAG_NV,
-        .DPA_base = 0,
+        .flags = is_pmem ? CDAT_DSMAS_FLAG_NV : 0,
+        .DPA_base = dpa_base,
         .DPA_length = int128_get64(mr->size),
     };
 
@@ -130,8 +131,11 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
             .length = sizeof(*dsemts),
         },
         .DSMAS_handle = dsmad_handle,
-        /* Reserved - the non volatile from DSMAS matters */
-        .EFI_memory_type_attr = 2,
+        /*
+         * NV: Reserved - the non volatile from DSMAS matters
+         * V: EFI_MEMORY_SP
+         */
+        .EFI_memory_type_attr = is_pmem ? 2 : 1,
         .DPA_offset = 0,
         .DPA_length = int128_get64(mr->size),
     };
@@ -150,33 +154,66 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
 static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
 {
     g_autofree CDATSubHeader **table = NULL;
-    MemoryRegion *nonvolatile_mr;
     CXLType3Dev *ct3d = priv;
+    MemoryRegion *volatile_mr = NULL, *nonvolatile_mr = NULL;
     int dsmad_handle = 0;
-    int rc;
+    int cur_ent = 0;
+    int len = 0;
+    int rc, i;
 
-    if (!ct3d->hostmem) {
+    if (!ct3d->hostpmem && !ct3d->hostvmem) {
         return 0;
     }
 
-    nonvolatile_mr = host_memory_backend_get_memory(ct3d->hostmem);
-    if (!nonvolatile_mr) {
-        return -EINVAL;
+    if (ct3d->hostvmem) {
+        volatile_mr = host_memory_backend_get_memory(ct3d->hostvmem);
+        if (!volatile_mr) {
+            return -EINVAL;
+        }
+        len += CT3_CDAT_NUM_ENTRIES;
+    }
+
+    if (ct3d->hostpmem) {
+        nonvolatile_mr = host_memory_backend_get_memory(ct3d->hostpmem);
+        if (!nonvolatile_mr) {
+            return -EINVAL;
+        }
+        len += CT3_CDAT_NUM_ENTRIES;
     }
 
-    table = g_malloc0(CT3_CDAT_NUM_ENTRIES * sizeof(*table));
+    table = g_malloc0(len * sizeof(*table));
     if (!table) {
         return -ENOMEM;
     }
 
-    rc = ct3_build_cdat_entries_for_mr(table, dsmad_handle++, nonvolatile_mr);
-    if (rc < 0) {
-        return rc;
+    /* Now fill them in */
+    if (volatile_mr) {
+        rc = ct3_build_cdat_entries_for_mr(table, dsmad_handle++, volatile_mr,
+                                           false, 0);
+        if (rc < 0) {
+            return rc;
+        }
+        cur_ent = CT3_CDAT_NUM_ENTRIES;
     }
 
+    if (nonvolatile_mr) {
+        rc = ct3_build_cdat_entries_for_mr(&(table[cur_ent]), dsmad_handle++,
+                nonvolatile_mr, true, (volatile_mr ? volatile_mr->size : 0));
+        if (rc < 0) {
+            goto error_cleanup;
+        }
+        cur_ent += CT3_CDAT_NUM_ENTRIES;
+    }
+    assert(len == cur_ent);
+
     *cdat_table = g_steal_pointer(&table);
 
-    return CT3_CDAT_NUM_ENTRIES;
+    return len;
+error_cleanup:
+    for (i = 0; i < cur_ent; i++) {
+        g_free(table[i]);
+    }
+    return rc;
 }
 
 static void ct3_free_cdat_table(CDATSubHeader **cdat_table, int num, void *priv)
@@ -264,16 +301,48 @@ static void build_dvsecs(CXLType3Dev *ct3d)
 {
     CXLComponentState *cxl_cstate = &ct3d->cxl_cstate;
     uint8_t *dvsec;
+    uint32_t range1_size_hi, range1_size_lo,
+             range1_base_hi, range1_base_lo,
+             range2_size_hi = 0, range2_size_lo = 0,
+             range2_base_hi = 0, range2_base_lo = 0;
+
+    /*
+     * Volatile memory is mapped as (0x0)
+     * Persistent memory is mapped at (volatile->size)
+     */
+    if (ct3d->hostvmem) {
+        range1_size_hi = ct3d->hostvmem->size >> 32;
+        range1_size_lo = (2 << 5) | (2 << 2) | 0x3 |
+                         (ct3d->hostvmem->size & 0xF0000000);
+        range1_base_hi = 0;
+        range1_base_lo = 0;
+        if (ct3d->hostpmem) {
+            range2_size_hi = ct3d->hostpmem->size >> 32;
+            range2_size_lo = (2 << 5) | (2 << 2) | 0x3 |
+                             (ct3d->hostpmem->size & 0xF0000000);
+            range2_base_hi = ct3d->hostvmem->size >> 32;
+            range2_base_lo = ct3d->hostvmem->size & 0xF0000000;
+        }
+    } else {
+        range1_size_hi = ct3d->hostpmem->size >> 32;
+        range1_size_lo = (2 << 5) | (2 << 2) | 0x3 |
+                         (ct3d->hostpmem->size & 0xF0000000);
+        range1_base_hi = 0;
+        range1_base_lo = 0;
+    }
 
     dvsec = (uint8_t *)&(CXLDVSECDevice){
         .cap = 0x1e,
         .ctrl = 0x2,
         .status2 = 0x2,
-        .range1_size_hi = ct3d->hostmem->size >> 32,
-        .range1_size_lo = (2 << 5) | (2 << 2) | 0x3 |
-        (ct3d->hostmem->size & 0xF0000000),
-        .range1_base_hi = 0,
-        .range1_base_lo = 0,
+        .range1_size_hi = range1_size_hi,
+        .range1_size_lo = range1_size_lo,
+        .range1_base_hi = range1_base_hi,
+        .range1_base_lo = range1_base_lo,
+        .range2_size_hi = range2_size_hi,
+        .range2_size_lo = range2_size_lo,
+        .range2_base_hi = range2_base_hi,
+        .range2_base_lo = range2_base_lo,
     };
     cxl_component_create_dvsec(cxl_cstate, CXL2_TYPE3_DEVICE,
                                PCIE_CXL_DEVICE_DVSEC_LENGTH,
@@ -492,36 +561,69 @@ 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;
 
-    if (!ct3d->hostmem) {
-        error_setg(errp, "memdev property must be set");
+    if (!ct3d->hostmem && !ct3d->hostvmem && !ct3d->hostpmem) {
+        error_setg(errp, "at least one memdev property must be set");
         return false;
+    } else if (ct3d->hostmem && ct3d->hostpmem) {
+        error_setg(errp, "[memdev] cannot be used with new "
+                         "[persistent-memdev] property");
+        return false;
+    } else if (ct3d->hostmem) {
+        /* Use of hostmem property implies pmem */
+        ct3d->hostpmem = ct3d->hostmem;
+        ct3d->hostmem = NULL;
     }
 
-    mr = host_memory_backend_get_memory(ct3d->hostmem);
-    if (!mr) {
-        error_setg(errp, "memdev property must be set");
+    if (ct3d->hostpmem && !ct3d->lsa) {
+        error_setg(errp, "lsa property must be set for persistent devices");
         return false;
     }
-    memory_region_set_nonvolatile(mr, true);
-    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");
+    if (ct3d->hostvmem) {
+        MemoryRegion *vmr;
+        char *v_name;
+
+        vmr = host_memory_backend_get_memory(ct3d->hostvmem);
+        if (!vmr) {
+            error_setg(errp, "volatile memdev must have backing device");
+            return false;
+        }
+        memory_region_set_nonvolatile(vmr, false);
+        memory_region_set_enabled(vmr, true);
+        host_memory_backend_set_mapped(ct3d->hostvmem, true);
+        if (ds->id) {
+            v_name = g_strdup_printf("cxl-type3-dpa-vmem-space:%s", ds->id);
+        } else {
+            v_name = g_strdup("cxl-type3-dpa-vmem-space");
+        }
+        address_space_init(&ct3d->hostvmem_as, vmr, v_name);
+        ct3d->cxl_dstate.vmem_size = vmr->size;
+        ct3d->cxl_dstate.mem_size += vmr->size;
+        g_free(v_name);
     }
-    address_space_init(&ct3d->hostmem_as, mr, name);
-    g_free(name);
 
-    ct3d->cxl_dstate.pmem_size = ct3d->hostmem->size;
+    if (ct3d->hostpmem) {
+        MemoryRegion *pmr;
+        char *p_name;
 
-    if (!ct3d->lsa) {
-        error_setg(errp, "lsa property must be set");
-        return false;
+        pmr = host_memory_backend_get_memory(ct3d->hostpmem);
+        if (!pmr) {
+            error_setg(errp, "persistent memdev must have backing device");
+            return false;
+        }
+        memory_region_set_nonvolatile(pmr, true);
+        memory_region_set_enabled(pmr, true);
+        host_memory_backend_set_mapped(ct3d->hostpmem, true);
+        if (ds->id) {
+            p_name = g_strdup_printf("cxl-type3-dpa-pmem-space:%s", ds->id);
+        } else {
+            p_name = g_strdup("cxl-type3-dpa-pmem-space");
+        }
+        address_space_init(&ct3d->hostpmem_as, pmr, p_name);
+        ct3d->cxl_dstate.pmem_size = pmr->size;
+        ct3d->cxl_dstate.mem_size += pmr->size;
+        g_free(p_name);
     }
 
     return true;
@@ -607,7 +709,12 @@ err_release_cdat:
     cxl_doe_cdat_release(cxl_cstate);
     g_free(regs->special_ops);
 err_address_space_free:
-    address_space_destroy(&ct3d->hostmem_as);
+    if (ct3d->hostpmem) {
+        address_space_destroy(&ct3d->hostpmem_as);
+    }
+    if (ct3d->hostvmem) {
+        address_space_destroy(&ct3d->hostvmem_as);
+    }
     return;
 }
 
@@ -620,7 +727,12 @@ static void ct3_exit(PCIDevice *pci_dev)
     pcie_aer_exit(pci_dev);
     cxl_doe_cdat_release(cxl_cstate);
     g_free(regs->special_ops);
-    address_space_destroy(&ct3d->hostmem_as);
+    if (ct3d->hostpmem) {
+        address_space_destroy(&ct3d->hostpmem_as);
+    }
+    if (ct3d->hostvmem) {
+        address_space_destroy(&ct3d->hostvmem_as);
+    }
 }
 
 /* TODO: Support multiple HDM decoders and DPA skip */
@@ -655,51 +767,77 @@ static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t *dpa)
     return true;
 }
 
-MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
-                           unsigned size, MemTxAttrs attrs)
+static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d,
+                                       hwaddr host_addr,
+                                       unsigned int size,
+                                       AddressSpace **as,
+                                       uint64_t *dpa_offset)
 {
-    CXLType3Dev *ct3d = CXL_TYPE3(d);
-    uint64_t dpa_offset;
-    MemoryRegion *mr;
+    MemoryRegion *vmr = NULL, *pmr = NULL;
 
-    /* TODO support volatile region */
-    mr = host_memory_backend_get_memory(ct3d->hostmem);
-    if (!mr) {
-        return MEMTX_ERROR;
+    if (ct3d->hostvmem) {
+        vmr = host_memory_backend_get_memory(ct3d->hostvmem);
+    }
+    if (ct3d->hostpmem) {
+        pmr = host_memory_backend_get_memory(ct3d->hostpmem);
     }
 
-    if (!cxl_type3_dpa(ct3d, host_addr, &dpa_offset)) {
-        return MEMTX_ERROR;
+    if (!vmr && !pmr) {
+        return -ENODEV;
+    }
+
+    if (!cxl_type3_dpa(ct3d, host_addr, dpa_offset)) {
+        return -EINVAL;
+    }
+
+    if (*dpa_offset > int128_get64(ct3d->cxl_dstate.mem_size)) {
+        return -EINVAL;
+    }
+
+    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;
     }
 
-    if (dpa_offset > int128_get64(mr->size)) {
+    return 0;
+}
+
+MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
+                           unsigned size, MemTxAttrs attrs)
+{
+    uint64_t dpa_offset = 0;
+    AddressSpace *as = NULL;
+    int res;
+
+    res = cxl_type3_hpa_to_as_and_dpa(CXL_TYPE3(d), host_addr, size,
+                                      &as, &dpa_offset);
+    if (res) {
         return MEMTX_ERROR;
     }
 
-    return address_space_read(&ct3d->hostmem_as, dpa_offset, attrs, data, size);
+    return address_space_read(as, dpa_offset, attrs, data, size);
 }
 
 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;
-
-    mr = host_memory_backend_get_memory(ct3d->hostmem);
-    if (!mr) {
-        return MEMTX_OK;
-    }
+    uint64_t dpa_offset = 0;
+    AddressSpace *as = NULL;
+    int res;
 
-    if (!cxl_type3_dpa(ct3d, host_addr, &dpa_offset)) {
-        return MEMTX_OK;
+    res = cxl_type3_hpa_to_as_and_dpa(CXL_TYPE3(d), host_addr, size,
+                                      &as, &dpa_offset);
+    if (res) {
+        return MEMTX_ERROR;
     }
 
-    if (dpa_offset > int128_get64(mr->size)) {
-        return MEMTX_OK;
-    }
-    return address_space_write(&ct3d->hostmem_as, dpa_offset, attrs,
-                               &data, size);
+    return address_space_write(as, dpa_offset, attrs, &data, size);
 }
 
 static void ct3d_reset(DeviceState *dev)
@@ -714,7 +852,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),
@@ -726,6 +868,10 @@ static uint64_t get_lsa_size(CXLType3Dev *ct3d)
 {
     MemoryRegion *mr;
 
+    if (!ct3d->lsa) {
+        return 0;
+    }
+
     mr = host_memory_backend_get_memory(ct3d->lsa);
     return memory_region_size(mr);
 }
@@ -743,6 +889,10 @@ static uint64_t get_lsa(CXLType3Dev *ct3d, void *buf, uint64_t size,
     MemoryRegion *mr;
     void *lsa;
 
+    if (!ct3d->lsa) {
+        return 0;
+    }
+
     mr = host_memory_backend_get_memory(ct3d->lsa);
     validate_lsa_access(mr, size, offset);
 
@@ -758,6 +908,10 @@ static void set_lsa(CXLType3Dev *ct3d, const void *buf, uint64_t size,
     MemoryRegion *mr;
     void *lsa;
 
+    if (!ct3d->lsa) {
+        return;
+    }
+
     mr = host_memory_backend_get_memory(ct3d->lsa);
     validate_lsa_access(mr, size, offset);
 
@@ -929,7 +1083,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);
 
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index d589f78202..edb9791bab 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -119,8 +119,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 */
@@ -245,12 +247,15 @@ struct CXLType3Dev {
     PCIDevice parent_obj;
 
     /* Properties */
-    HostMemoryBackend *hostmem;
+    HostMemoryBackend *hostmem; /* deprecated */
+    HostMemoryBackend *hostvmem;
+    HostMemoryBackend *hostpmem;
     HostMemoryBackend *lsa;
     uint64_t sn;
 
     /* State */
-    AddressSpace hostmem_as;
+    AddressSpace hostvmem_as;
+    AddressSpace hostpmem_as;
     CXLComponentState cxl_cstate;
     CXLDeviceState cxl_dstate;
 
diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 8608408213..b005c03a92 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -1789,13 +1789,13 @@ static void test_acpi_q35_cxl(void)
                              " -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1"
                              " -device pxb-cxl,bus_nr=222,bus=pcie.0,id=cxl.2"
                              " -device cxl-rp,port=0,bus=cxl.1,id=rp1,chassis=0,slot=2"
-                             " -device cxl-type3,bus=rp1,memdev=cxl-mem1,lsa=lsa1"
+                             " -device cxl-type3,bus=rp1,persistent-memdev=cxl-mem1,lsa=lsa1"
                              " -device cxl-rp,port=1,bus=cxl.1,id=rp2,chassis=0,slot=3"
-                             " -device cxl-type3,bus=rp2,memdev=cxl-mem2,lsa=lsa2"
+                             " -device cxl-type3,bus=rp2,persistent-memdev=cxl-mem2,lsa=lsa2"
                              " -device cxl-rp,port=0,bus=cxl.2,id=rp3,chassis=0,slot=5"
-                             " -device cxl-type3,bus=rp3,memdev=cxl-mem3,lsa=lsa3"
+                             " -device cxl-type3,bus=rp3,persistent-memdev=cxl-mem3,lsa=lsa3"
                              " -device cxl-rp,port=1,bus=cxl.2,id=rp4,chassis=0,slot=6"
-                             " -device cxl-type3,bus=rp4,memdev=cxl-mem4,lsa=lsa4"
+                             " -device cxl-type3,bus=rp4,persistent-memdev=cxl-mem4,lsa=lsa4"
                              " -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.size=4G,cxl-fmw.0.interleave-granularity=8k,"
                              "cxl-fmw.1.targets.0=cxl.1,cxl-fmw.1.targets.1=cxl.2,cxl-fmw.1.size=4G,cxl-fmw.1.interleave-granularity=8k",
                              tmp_path, tmp_path, tmp_path, tmp_path,
diff --git a/tests/qtest/cxl-test.c b/tests/qtest/cxl-test.c
index eda2bbbbe6..edcad4a0ce 100644
--- a/tests/qtest/cxl-test.c
+++ b/tests/qtest/cxl-test.c
@@ -34,32 +34,46 @@
     "-device cxl-rp,id=rp2,bus=cxl.1,chassis=0,slot=2 " \
     "-device cxl-rp,id=rp3,bus=cxl.1,chassis=0,slot=3 "
 
-#define QEMU_T3D \
+#define QEMU_T3D_DEPRECATED \
     "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
     "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
     "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 "
 
+#define QEMU_T3D_PMEM \
+    "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
+    "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
+    "-device cxl-type3,bus=rp0,persistent-memdev=cxl-mem0,lsa=lsa0,id=pmem0 "
+
+#define QEMU_T3D_VMEM \
+    "-object memory-backend-ram,id=cxl-mem0,size=256M " \
+    "-device cxl-type3,bus=rp0,volatile-memdev=cxl-mem0,id=mem0 "
+
+#define QEMU_T3D_VMEM_LSA \
+    "-object memory-backend-ram,id=cxl-mem0,size=256M " \
+    "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
+    "-device cxl-type3,bus=rp0,volatile-memdev=cxl-mem0,lsa=lsa0,id=mem0 "
+
 #define QEMU_2T3D \
     "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
     "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
-    "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 " \
+    "-device cxl-type3,bus=rp0,persistent-memdev=cxl-mem0,lsa=lsa0,id=pmem0 " \
     "-object memory-backend-file,id=cxl-mem1,mem-path=%s,size=256M " \
     "-object memory-backend-file,id=lsa1,mem-path=%s,size=256M " \
-    "-device cxl-type3,bus=rp1,memdev=cxl-mem1,lsa=lsa1,id=cxl-pmem1 "
+    "-device cxl-type3,bus=rp1,persistent-memdev=cxl-mem1,lsa=lsa1,id=pmem1 "
 
 #define QEMU_4T3D \
     "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
     "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
-    "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 " \
+    "-device cxl-type3,bus=rp0,persistent-memdev=cxl-mem0,lsa=lsa0,id=pmem0 " \
     "-object memory-backend-file,id=cxl-mem1,mem-path=%s,size=256M " \
     "-object memory-backend-file,id=lsa1,mem-path=%s,size=256M " \
-    "-device cxl-type3,bus=rp1,memdev=cxl-mem1,lsa=lsa1,id=cxl-pmem1 " \
+    "-device cxl-type3,bus=rp1,persistent-memdev=cxl-mem1,lsa=lsa1,id=pmem1 " \
     "-object memory-backend-file,id=cxl-mem2,mem-path=%s,size=256M " \
     "-object memory-backend-file,id=lsa2,mem-path=%s,size=256M " \
-    "-device cxl-type3,bus=rp2,memdev=cxl-mem2,lsa=lsa2,id=cxl-pmem2 " \
+    "-device cxl-type3,bus=rp2,persistent-memdev=cxl-mem2,lsa=lsa2,id=pmem2 " \
     "-object memory-backend-file,id=cxl-mem3,mem-path=%s,size=256M " \
     "-object memory-backend-file,id=lsa3,mem-path=%s,size=256M " \
-    "-device cxl-type3,bus=rp3,memdev=cxl-mem3,lsa=lsa3,id=cxl-pmem3 "
+    "-device cxl-type3,bus=rp3,persistent-memdev=cxl-mem3,lsa=lsa3,id=pmem3 "
 
 static void cxl_basic_hb(void)
 {
@@ -98,14 +112,53 @@ static void cxl_2root_port(void)
 }
 
 #ifdef CONFIG_POSIX
-static void cxl_t3d(void)
+static void cxl_t3d_deprecated(void)
+{
+    g_autoptr(GString) cmdline = g_string_new(NULL);
+    g_autofree const char *tmpfs = NULL;
+
+    tmpfs = g_dir_make_tmp("cxl-test-XXXXXX", NULL);
+
+    g_string_printf(cmdline, QEMU_PXB_CMD QEMU_RP QEMU_T3D_DEPRECATED,
+                    tmpfs, tmpfs);
+
+    qtest_start(cmdline->str);
+    qtest_end();
+}
+
+static void cxl_t3d_persistent(void)
+{
+    g_autoptr(GString) cmdline = g_string_new(NULL);
+    g_autofree const char *tmpfs = NULL;
+
+    tmpfs = g_dir_make_tmp("cxl-test-XXXXXX", NULL);
+
+    g_string_printf(cmdline, QEMU_PXB_CMD QEMU_RP QEMU_T3D_PMEM,
+                    tmpfs, tmpfs);
+
+    qtest_start(cmdline->str);
+    qtest_end();
+}
+
+static void cxl_t3d_volatile(void)
+{
+    g_autoptr(GString) cmdline = g_string_new(NULL);
+
+    g_string_printf(cmdline, QEMU_PXB_CMD QEMU_RP QEMU_T3D_VMEM);
+
+    qtest_start(cmdline->str);
+    qtest_end();
+}
+
+static void cxl_t3d_volatile_lsa(void)
 {
     g_autoptr(GString) cmdline = g_string_new(NULL);
     g_autofree const char *tmpfs = NULL;
 
     tmpfs = g_dir_make_tmp("cxl-test-XXXXXX", NULL);
 
-    g_string_printf(cmdline, QEMU_PXB_CMD QEMU_RP QEMU_T3D, tmpfs, tmpfs);
+    g_string_printf(cmdline, QEMU_PXB_CMD QEMU_RP QEMU_T3D_VMEM_LSA,
+                    tmpfs);
 
     qtest_start(cmdline->str);
     qtest_end();
@@ -155,7 +208,10 @@ int main(int argc, char **argv)
     qtest_add_func("/pci/cxl/rp", cxl_root_port);
     qtest_add_func("/pci/cxl/rp_x2", cxl_2root_port);
 #ifdef CONFIG_POSIX
-    qtest_add_func("/pci/cxl/type3_device", cxl_t3d);
+    qtest_add_func("/pci/cxl/type3_device", cxl_t3d_deprecated);
+    qtest_add_func("/pci/cxl/type3_device_pmem", cxl_t3d_persistent);
+    qtest_add_func("/pci/cxl/type3_device_vmem", cxl_t3d_volatile);
+    qtest_add_func("/pci/cxl/type3_device_vmem_lsa", cxl_t3d_volatile_lsa);
     qtest_add_func("/pci/cxl/rp_x2_type3_x2", cxl_1pxb_2rp_2t3d);
     qtest_add_func("/pci/cxl/pxb_x2_root_port_x4_type3_x4", cxl_2pxb_4rp_4t3d);
 #endif
-- 
2.37.2



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent)
  2023-01-31 16:38   ` Jonathan Cameron via
  (?)
  (?)
@ 2023-02-14 18:15   ` Davidlohr Bueso
  -1 siblings, 0 replies; 18+ messages in thread
From: Davidlohr Bueso @ 2023-02-14 18:15 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: qemu-devel, Michael Tsirkin, Ben Widawsky, linux-cxl, linuxarm,
	Ira Weiny, Gregory Price, Philippe Mathieu-Daud�,
	Mike Maslenkin

On Tue, 31 Jan 2023, Jonathan Cameron 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>

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent)
       [not found]   ` <CGME20230216184225uscas1p2bb2f60fe3d6f0d1f8d9ffe2ff377e190@uscas1p2.samsung.com>
@ 2023-02-16 18:42     ` Fan Ni
  0 siblings, 0 replies; 18+ messages in thread
From: Fan Ni @ 2023-02-16 18:42 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: qemu-devel, Michael Tsirkin, Ben Widawsky, linux-cxl, linuxarm,
	Ira Weiny, Gregory Price, Philippe Mathieu-Daudé,
	Mike Maslenkin, Adam Manzanares, dave

On Tue, Jan 31, 2023 at 04:38:47PM +0000, Jonathan Cameron 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>
> 

Reviewed-by: Fan Ni <fan.ni@samsung.com>
Tested-by: Fan Ni <fan.ni@samsung.com>

Tested a single HB, single RP, single volatile memdev setup (with
kernel volatile patch from Dan), it passed the following ops,
1. cxl list
2. cxl create-region
3. daxctl online-memory dax0.0
4. simple apps to use cxl memory with "numactl --membind=1"

> ---
> Chances since RFC V4:
> - Fixed up issue reported by Gregory of volatile regions being reported
>   as non volatile in CDAT.  Fix as suggested by Gregory.
> - Report volatile memory as EFI_MEMORY_SP.
> - Fixed some naming issues in tests including sticking to existing
>   naming for memory device of cxl-memX throughout.
> - Update the bios-tables-test.c test to use the new non deprecated
>   parameter name. Note this has no impact on generated tables.
> - Flipped logic of get_lsa_size() callback to slightly simplify change.
> - cxl_setup_memory() reduce scope of local variables and name them
>   to make it clear which are volatile and which are persistent related.
> 
>  docs/system/devices/cxl.rst    |  49 ++++--
>  hw/cxl/cxl-mailbox-utils.c     |  26 +--
>  hw/mem/cxl_type3.c             | 300 +++++++++++++++++++++++++--------
>  include/hw/cxl/cxl_device.h    |  11 +-
>  tests/qtest/bios-tables-test.c |   8 +-
>  tests/qtest/cxl-test.c         |  76 +++++++--
>  6 files changed, 359 insertions(+), 111 deletions(-)
> 
> diff --git a/docs/system/devices/cxl.rst b/docs/system/devices/cxl.rst
> index f25783a4ec..89a41cff73 100644
> --- a/docs/system/devices/cxl.rst
> +++ b/docs/system/devices/cxl.rst
> @@ -300,7 +300,7 @@ Example topology involving a switch::
>  
>  Example command lines
>  ---------------------
> -A very simple setup with just one directly attached CXL Type 3 device::
> +A very simple setup with just one directly attached CXL Type 3 Persistent Memory device::
>  
>    qemu-system-aarch64 -M virt,gic-version=3,cxl=on -m 4g,maxmem=8G,slots=8 -cpu max \
>    ...
> @@ -308,7 +308,28 @@ A very simple setup with just one directly attached CXL Type 3 device::
>    -object memory-backend-file,id=cxl-lsa1,share=on,mem-path=/tmp/lsa.raw,size=256M \
>    -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
>    -device cxl-rp,port=0,bus=cxl.1,id=root_port13,chassis=0,slot=2 \
> -  -device cxl-type3,bus=root_port13,memdev=cxl-mem1,lsa=cxl-lsa1,id=cxl-pmem0 \
> +  -device cxl-type3,bus=root_port13,persistent-memdev=cxl-mem1,lsa=cxl-lsa1,id=cxl-pmem0 \
> +  -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.size=4G
> +
> +A very simple setup with just one directly attached CXL Type 3 Volatile Memory device::
> +
> +  qemu-system-aarch64 -M virt,gic-version=3,cxl=on -m 4g,maxmem=8G,slots=8 -cpu max \
> +  ...
> +  -object memory-backend-ram,id=vmem0,share=on,size=256M \
> +  -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
> +  -device cxl-rp,port=0,bus=cxl.1,id=root_port13,chassis=0,slot=2 \
> +  -device cxl-type3,bus=root_port13,volatile-memdev=vmem0,id=cxl-vmem0 \
> +  -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.size=4G
> +
> +The same volatile setup may optionally include an LSA region::
> +
> +  qemu-system-aarch64 -M virt,gic-version=3,cxl=on -m 4g,maxmem=8G,slots=8 -cpu max \
> +  ...
> +  -object memory-backend-ram,id=vmem0,share=on,size=256M \
> +  -object memory-backend-file,id=cxl-lsa0,share=on,mem-path=/tmp/lsa.raw,size=256M \
> +  -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
> +  -device cxl-rp,port=0,bus=cxl.1,id=root_port13,chassis=0,slot=2 \
> +  -device cxl-type3,bus=root_port13,volatile-memdev=vmem0,lsa=cxl-lsa0,id=cxl-vmem0 \
>    -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.size=4G
>  
>  A setup suitable for 4 way interleave. Only one fixed window provided, to enable 2 way
> @@ -328,13 +349,13 @@ the CXL Type3 device directly attached (no switches).::
>    -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
>    -device pxb-cxl,bus_nr=222,bus=pcie.0,id=cxl.2 \
>    -device cxl-rp,port=0,bus=cxl.1,id=root_port13,chassis=0,slot=2 \
> -  -device cxl-type3,bus=root_port13,memdev=cxl-mem1,lsa=cxl-lsa1,id=cxl-pmem0 \
> +  -device cxl-type3,bus=root_port13,persistent-memdev=cxl-mem1,lsa=cxl-lsa1,id=cxl-pmem0 \
>    -device cxl-rp,port=1,bus=cxl.1,id=root_port14,chassis=0,slot=3 \
> -  -device cxl-type3,bus=root_port14,memdev=cxl-mem2,lsa=cxl-lsa2,id=cxl-pmem1 \
> +  -device cxl-type3,bus=root_port14,persistent-memdev=cxl-mem2,lsa=cxl-lsa2,id=cxl-pmem1 \
>    -device cxl-rp,port=0,bus=cxl.2,id=root_port15,chassis=0,slot=5 \
> -  -device cxl-type3,bus=root_port15,memdev=cxl-mem3,lsa=cxl-lsa3,id=cxl-pmem2 \
> +  -device cxl-type3,bus=root_port15,persistent-memdev=cxl-mem3,lsa=cxl-lsa3,id=cxl-pmem2 \
>    -device cxl-rp,port=1,bus=cxl.2,id=root_port16,chassis=0,slot=6 \
> -  -device cxl-type3,bus=root_port16,memdev=cxl-mem4,lsa=cxl-lsa4,id=cxl-pmem3 \
> +  -device cxl-type3,bus=root_port16,persistent-memdev=cxl-mem4,lsa=cxl-lsa4,id=cxl-pmem3 \
>    -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.targets.1=cxl.2,cxl-fmw.0.size=4G,cxl-fmw.0.interleave-granularity=8k
>  
>  An example of 4 devices below a switch suitable for 1, 2 or 4 way interleave::
> @@ -354,15 +375,23 @@ An example of 4 devices below a switch suitable for 1, 2 or 4 way interleave::
>    -device cxl-rp,port=1,bus=cxl.1,id=root_port1,chassis=0,slot=1 \
>    -device cxl-upstream,bus=root_port0,id=us0 \
>    -device cxl-downstream,port=0,bus=us0,id=swport0,chassis=0,slot=4 \
> -  -device cxl-type3,bus=swport0,memdev=cxl-mem0,lsa=cxl-lsa0,id=cxl-pmem0,size=256M \
> +  -device cxl-type3,bus=swport0,persistent-memdev=cxl-mem0,lsa=cxl-lsa0,id=cxl-pmem0,size=256M \
>    -device cxl-downstream,port=1,bus=us0,id=swport1,chassis=0,slot=5 \
> -  -device cxl-type3,bus=swport1,memdev=cxl-mem1,lsa=cxl-lsa1,id=cxl-pmem1,size=256M \
> +  -device cxl-type3,bus=swport1,persistent-memdev=cxl-mem1,lsa=cxl-lsa1,id=cxl-pmem1,size=256M \
>    -device cxl-downstream,port=2,bus=us0,id=swport2,chassis=0,slot=6 \
> -  -device cxl-type3,bus=swport2,memdev=cxl-mem2,lsa=cxl-lsa2,id=cxl-pmem2,size=256M \
> +  -device cxl-type3,bus=swport2,persistent-memdev=cxl-mem2,lsa=cxl-lsa2,id=cxl-pmem2,size=256M \
>    -device cxl-downstream,port=3,bus=us0,id=swport3,chassis=0,slot=7 \
> -  -device cxl-type3,bus=swport3,memdev=cxl-mem3,lsa=cxl-lsa3,id=cxl-pmem3,size=256M \
> +  -device cxl-type3,bus=swport3,persistent-memdev=cxl-mem3,lsa=cxl-lsa3,id=cxl-pmem3,size=256M \
>    -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.size=4G,cxl-fmw.0.interleave-granularity=4k
>  
> +Deprecations
> +------------
> +
> +The Type 3 device [memdev] attribute has been deprecated in favor of the
> +[persistent-memdev] attributes. [memdev] will default to a persistent memory
> +device for backward compatibility and is incapable of being used in combination
> +with [persistent-memdev].
> +
>  Kernel Configuration Options
>  ----------------------------
>  
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 206e04a4b8..cc9c8b7380 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -141,7 +141,8 @@ 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 < CXL_CAPACITY_MULTIPLIER) {
> +    if ((cxl_dstate->vmem_size < CXL_CAPACITY_MULTIPLIER) ||
> +        (cxl_dstate->pmem_size < CXL_CAPACITY_MULTIPLIER)) {
>          return CXL_MBOX_INTERNAL_ERROR;
>      }
>  
> @@ -288,20 +289,20 @@ 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, CXL_CAPACITY_MULTIPLIER)) {
> +    if ((!QEMU_IS_ALIGNED(cxl_dstate->vmem_size, CXL_CAPACITY_MULTIPLIER)) ||
> +        (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, CXL_CAPACITY_MULTIPLIER))) {
>          return CXL_MBOX_INTERNAL_ERROR;
>      }
>  
>      id = (void *)cmd->payload;
>      memset(id, 0, sizeof(*id));
>  
> -    /* PMEM only */
>      snprintf(id->fw_revision, 0x10, "BWFW VERSION %02d", 0);
>  
> -    id->total_capacity = size / CXL_CAPACITY_MULTIPLIER;
> -    id->persistent_capacity = size / CXL_CAPACITY_MULTIPLIER;
> +    id->total_capacity = cxl_dstate->mem_size / CXL_CAPACITY_MULTIPLIER;
> +    id->persistent_capacity = cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER;
> +    id->volatile_capacity = cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER;
>      id->lsa_size = cvc->get_lsa_size(ct3d);
>  
>      *len = sizeof(*id);
> @@ -319,16 +320,19 @@ 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, CXL_CAPACITY_MULTIPLIER)) {
> +    if ((!QEMU_IS_ALIGNED(cxl_dstate->vmem_size, CXL_CAPACITY_MULTIPLIER)) ||
> +        (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, CXL_CAPACITY_MULTIPLIER))) {
>          return CXL_MBOX_INTERNAL_ERROR;
>      }
>  
> -    /* PMEM only */
> -    part_info->active_vmem = 0;
> +    part_info->active_vmem = cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER;
> +    /*
> +     * When both next_vmem and next_pmem are 0, there is no pending change to
> +     * partitioning.
> +     */
>      part_info->next_vmem = 0;
> -    part_info->active_pmem = size / CXL_CAPACITY_MULTIPLIER;
> +    part_info->active_pmem = cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER;
>      part_info->next_pmem = 0;
>  
>      *len = sizeof(*part_info);
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index e32bbac966..beb931d59a 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -31,7 +31,8 @@ enum {
>  };
>  
>  static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
> -                                         int dsmad_handle, MemoryRegion *mr)
> +                                         int dsmad_handle, MemoryRegion *mr,
> +                                         bool is_pmem, uint64_t dpa_base)
>  {
>      g_autofree CDATDsmas *dsmas = NULL;
>      g_autofree CDATDslbis *dslbis0 = NULL;
> @@ -50,8 +51,8 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
>              .length = sizeof(*dsmas),
>          },
>          .DSMADhandle = dsmad_handle,
> -        .flags = CDAT_DSMAS_FLAG_NV,
> -        .DPA_base = 0,
> +        .flags = is_pmem ? CDAT_DSMAS_FLAG_NV : 0,
> +        .DPA_base = dpa_base,
>          .DPA_length = int128_get64(mr->size),
>      };
>  
> @@ -130,8 +131,11 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
>              .length = sizeof(*dsemts),
>          },
>          .DSMAS_handle = dsmad_handle,
> -        /* Reserved - the non volatile from DSMAS matters */
> -        .EFI_memory_type_attr = 2,
> +        /*
> +         * NV: Reserved - the non volatile from DSMAS matters
> +         * V: EFI_MEMORY_SP
> +         */
> +        .EFI_memory_type_attr = is_pmem ? 2 : 1,
>          .DPA_offset = 0,
>          .DPA_length = int128_get64(mr->size),
>      };
> @@ -150,33 +154,66 @@ static int ct3_build_cdat_entries_for_mr(CDATSubHeader **cdat_table,
>  static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
>  {
>      g_autofree CDATSubHeader **table = NULL;
> -    MemoryRegion *nonvolatile_mr;
>      CXLType3Dev *ct3d = priv;
> +    MemoryRegion *volatile_mr = NULL, *nonvolatile_mr = NULL;
>      int dsmad_handle = 0;
> -    int rc;
> +    int cur_ent = 0;
> +    int len = 0;
> +    int rc, i;
>  
> -    if (!ct3d->hostmem) {
> +    if (!ct3d->hostpmem && !ct3d->hostvmem) {
>          return 0;
>      }
>  
> -    nonvolatile_mr = host_memory_backend_get_memory(ct3d->hostmem);
> -    if (!nonvolatile_mr) {
> -        return -EINVAL;
> +    if (ct3d->hostvmem) {
> +        volatile_mr = host_memory_backend_get_memory(ct3d->hostvmem);
> +        if (!volatile_mr) {
> +            return -EINVAL;
> +        }
> +        len += CT3_CDAT_NUM_ENTRIES;
> +    }
> +
> +    if (ct3d->hostpmem) {
> +        nonvolatile_mr = host_memory_backend_get_memory(ct3d->hostpmem);
> +        if (!nonvolatile_mr) {
> +            return -EINVAL;
> +        }
> +        len += CT3_CDAT_NUM_ENTRIES;
>      }
>  
> -    table = g_malloc0(CT3_CDAT_NUM_ENTRIES * sizeof(*table));
> +    table = g_malloc0(len * sizeof(*table));
>      if (!table) {
>          return -ENOMEM;
>      }
>  
> -    rc = ct3_build_cdat_entries_for_mr(table, dsmad_handle++, nonvolatile_mr);
> -    if (rc < 0) {
> -        return rc;
> +    /* Now fill them in */
> +    if (volatile_mr) {
> +        rc = ct3_build_cdat_entries_for_mr(table, dsmad_handle++, volatile_mr,
> +                                           false, 0);
> +        if (rc < 0) {
> +            return rc;
> +        }
> +        cur_ent = CT3_CDAT_NUM_ENTRIES;
>      }
>  
> +    if (nonvolatile_mr) {
> +        rc = ct3_build_cdat_entries_for_mr(&(table[cur_ent]), dsmad_handle++,
> +                nonvolatile_mr, true, (volatile_mr ? volatile_mr->size : 0));
> +        if (rc < 0) {
> +            goto error_cleanup;
> +        }
> +        cur_ent += CT3_CDAT_NUM_ENTRIES;
> +    }
> +    assert(len == cur_ent);
> +
>      *cdat_table = g_steal_pointer(&table);
>  
> -    return CT3_CDAT_NUM_ENTRIES;
> +    return len;
> +error_cleanup:
> +    for (i = 0; i < cur_ent; i++) {
> +        g_free(table[i]);
> +    }
> +    return rc;
>  }
>  
>  static void ct3_free_cdat_table(CDATSubHeader **cdat_table, int num, void *priv)
> @@ -264,16 +301,48 @@ static void build_dvsecs(CXLType3Dev *ct3d)
>  {
>      CXLComponentState *cxl_cstate = &ct3d->cxl_cstate;
>      uint8_t *dvsec;
> +    uint32_t range1_size_hi, range1_size_lo,
> +             range1_base_hi, range1_base_lo,
> +             range2_size_hi = 0, range2_size_lo = 0,
> +             range2_base_hi = 0, range2_base_lo = 0;
> +
> +    /*
> +     * Volatile memory is mapped as (0x0)
> +     * Persistent memory is mapped at (volatile->size)
> +     */
> +    if (ct3d->hostvmem) {
> +        range1_size_hi = ct3d->hostvmem->size >> 32;
> +        range1_size_lo = (2 << 5) | (2 << 2) | 0x3 |
> +                         (ct3d->hostvmem->size & 0xF0000000);
> +        range1_base_hi = 0;
> +        range1_base_lo = 0;
> +        if (ct3d->hostpmem) {
> +            range2_size_hi = ct3d->hostpmem->size >> 32;
> +            range2_size_lo = (2 << 5) | (2 << 2) | 0x3 |
> +                             (ct3d->hostpmem->size & 0xF0000000);
> +            range2_base_hi = ct3d->hostvmem->size >> 32;
> +            range2_base_lo = ct3d->hostvmem->size & 0xF0000000;
> +        }
> +    } else {
> +        range1_size_hi = ct3d->hostpmem->size >> 32;
> +        range1_size_lo = (2 << 5) | (2 << 2) | 0x3 |
> +                         (ct3d->hostpmem->size & 0xF0000000);
> +        range1_base_hi = 0;
> +        range1_base_lo = 0;
> +    }
>  
>      dvsec = (uint8_t *)&(CXLDVSECDevice){
>          .cap = 0x1e,
>          .ctrl = 0x2,
>          .status2 = 0x2,
> -        .range1_size_hi = ct3d->hostmem->size >> 32,
> -        .range1_size_lo = (2 << 5) | (2 << 2) | 0x3 |
> -        (ct3d->hostmem->size & 0xF0000000),
> -        .range1_base_hi = 0,
> -        .range1_base_lo = 0,
> +        .range1_size_hi = range1_size_hi,
> +        .range1_size_lo = range1_size_lo,
> +        .range1_base_hi = range1_base_hi,
> +        .range1_base_lo = range1_base_lo,
> +        .range2_size_hi = range2_size_hi,
> +        .range2_size_lo = range2_size_lo,
> +        .range2_base_hi = range2_base_hi,
> +        .range2_base_lo = range2_base_lo,
>      };
>      cxl_component_create_dvsec(cxl_cstate, CXL2_TYPE3_DEVICE,
>                                 PCIE_CXL_DEVICE_DVSEC_LENGTH,
> @@ -492,36 +561,69 @@ 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;
>  
> -    if (!ct3d->hostmem) {
> -        error_setg(errp, "memdev property must be set");
> +    if (!ct3d->hostmem && !ct3d->hostvmem && !ct3d->hostpmem) {
> +        error_setg(errp, "at least one memdev property must be set");
>          return false;
> +    } else if (ct3d->hostmem && ct3d->hostpmem) {
> +        error_setg(errp, "[memdev] cannot be used with new "
> +                         "[persistent-memdev] property");
> +        return false;
> +    } else if (ct3d->hostmem) {
> +        /* Use of hostmem property implies pmem */
> +        ct3d->hostpmem = ct3d->hostmem;
> +        ct3d->hostmem = NULL;
>      }
>  
> -    mr = host_memory_backend_get_memory(ct3d->hostmem);
> -    if (!mr) {
> -        error_setg(errp, "memdev property must be set");
> +    if (ct3d->hostpmem && !ct3d->lsa) {
> +        error_setg(errp, "lsa property must be set for persistent devices");
>          return false;
>      }
> -    memory_region_set_nonvolatile(mr, true);
> -    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");
> +    if (ct3d->hostvmem) {
> +        MemoryRegion *vmr;
> +        char *v_name;
> +
> +        vmr = host_memory_backend_get_memory(ct3d->hostvmem);
> +        if (!vmr) {
> +            error_setg(errp, "volatile memdev must have backing device");
> +            return false;
> +        }
> +        memory_region_set_nonvolatile(vmr, false);
> +        memory_region_set_enabled(vmr, true);
> +        host_memory_backend_set_mapped(ct3d->hostvmem, true);
> +        if (ds->id) {
> +            v_name = g_strdup_printf("cxl-type3-dpa-vmem-space:%s", ds->id);
> +        } else {
> +            v_name = g_strdup("cxl-type3-dpa-vmem-space");
> +        }
> +        address_space_init(&ct3d->hostvmem_as, vmr, v_name);
> +        ct3d->cxl_dstate.vmem_size = vmr->size;
> +        ct3d->cxl_dstate.mem_size += vmr->size;
> +        g_free(v_name);
>      }
> -    address_space_init(&ct3d->hostmem_as, mr, name);
> -    g_free(name);
>  
> -    ct3d->cxl_dstate.pmem_size = ct3d->hostmem->size;
> +    if (ct3d->hostpmem) {
> +        MemoryRegion *pmr;
> +        char *p_name;
>  
> -    if (!ct3d->lsa) {
> -        error_setg(errp, "lsa property must be set");
> -        return false;
> +        pmr = host_memory_backend_get_memory(ct3d->hostpmem);
> +        if (!pmr) {
> +            error_setg(errp, "persistent memdev must have backing device");
> +            return false;
> +        }
> +        memory_region_set_nonvolatile(pmr, true);
> +        memory_region_set_enabled(pmr, true);
> +        host_memory_backend_set_mapped(ct3d->hostpmem, true);
> +        if (ds->id) {
> +            p_name = g_strdup_printf("cxl-type3-dpa-pmem-space:%s", ds->id);
> +        } else {
> +            p_name = g_strdup("cxl-type3-dpa-pmem-space");
> +        }
> +        address_space_init(&ct3d->hostpmem_as, pmr, p_name);
> +        ct3d->cxl_dstate.pmem_size = pmr->size;
> +        ct3d->cxl_dstate.mem_size += pmr->size;
> +        g_free(p_name);
>      }
>  
>      return true;
> @@ -607,7 +709,12 @@ err_release_cdat:
>      cxl_doe_cdat_release(cxl_cstate);
>      g_free(regs->special_ops);
>  err_address_space_free:
> -    address_space_destroy(&ct3d->hostmem_as);
> +    if (ct3d->hostpmem) {
> +        address_space_destroy(&ct3d->hostpmem_as);
> +    }
> +    if (ct3d->hostvmem) {
> +        address_space_destroy(&ct3d->hostvmem_as);
> +    }
>      return;
>  }
>  
> @@ -620,7 +727,12 @@ static void ct3_exit(PCIDevice *pci_dev)
>      pcie_aer_exit(pci_dev);
>      cxl_doe_cdat_release(cxl_cstate);
>      g_free(regs->special_ops);
> -    address_space_destroy(&ct3d->hostmem_as);
> +    if (ct3d->hostpmem) {
> +        address_space_destroy(&ct3d->hostpmem_as);
> +    }
> +    if (ct3d->hostvmem) {
> +        address_space_destroy(&ct3d->hostvmem_as);
> +    }
>  }
>  
>  /* TODO: Support multiple HDM decoders and DPA skip */
> @@ -655,51 +767,77 @@ static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t *dpa)
>      return true;
>  }
>  
> -MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
> -                           unsigned size, MemTxAttrs attrs)
> +static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d,
> +                                       hwaddr host_addr,
> +                                       unsigned int size,
> +                                       AddressSpace **as,
> +                                       uint64_t *dpa_offset)
>  {
> -    CXLType3Dev *ct3d = CXL_TYPE3(d);
> -    uint64_t dpa_offset;
> -    MemoryRegion *mr;
> +    MemoryRegion *vmr = NULL, *pmr = NULL;
>  
> -    /* TODO support volatile region */
> -    mr = host_memory_backend_get_memory(ct3d->hostmem);
> -    if (!mr) {
> -        return MEMTX_ERROR;
> +    if (ct3d->hostvmem) {
> +        vmr = host_memory_backend_get_memory(ct3d->hostvmem);
> +    }
> +    if (ct3d->hostpmem) {
> +        pmr = host_memory_backend_get_memory(ct3d->hostpmem);
>      }
>  
> -    if (!cxl_type3_dpa(ct3d, host_addr, &dpa_offset)) {
> -        return MEMTX_ERROR;
> +    if (!vmr && !pmr) {
> +        return -ENODEV;
> +    }
> +
> +    if (!cxl_type3_dpa(ct3d, host_addr, dpa_offset)) {
> +        return -EINVAL;
> +    }
> +
> +    if (*dpa_offset > int128_get64(ct3d->cxl_dstate.mem_size)) {
> +        return -EINVAL;
> +    }
> +
> +    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;
>      }
>  
> -    if (dpa_offset > int128_get64(mr->size)) {
> +    return 0;
> +}
> +
> +MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
> +                           unsigned size, MemTxAttrs attrs)
> +{
> +    uint64_t dpa_offset = 0;
> +    AddressSpace *as = NULL;
> +    int res;
> +
> +    res = cxl_type3_hpa_to_as_and_dpa(CXL_TYPE3(d), host_addr, size,
> +                                      &as, &dpa_offset);
> +    if (res) {
>          return MEMTX_ERROR;
>      }
>  
> -    return address_space_read(&ct3d->hostmem_as, dpa_offset, attrs, data, size);
> +    return address_space_read(as, dpa_offset, attrs, data, size);
>  }
>  
>  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;
> -
> -    mr = host_memory_backend_get_memory(ct3d->hostmem);
> -    if (!mr) {
> -        return MEMTX_OK;
> -    }
> +    uint64_t dpa_offset = 0;
> +    AddressSpace *as = NULL;
> +    int res;
>  
> -    if (!cxl_type3_dpa(ct3d, host_addr, &dpa_offset)) {
> -        return MEMTX_OK;
> +    res = cxl_type3_hpa_to_as_and_dpa(CXL_TYPE3(d), host_addr, size,
> +                                      &as, &dpa_offset);
> +    if (res) {
> +        return MEMTX_ERROR;
>      }
>  
> -    if (dpa_offset > int128_get64(mr->size)) {
> -        return MEMTX_OK;
> -    }
> -    return address_space_write(&ct3d->hostmem_as, dpa_offset, attrs,
> -                               &data, size);
> +    return address_space_write(as, dpa_offset, attrs, &data, size);
>  }
>  
>  static void ct3d_reset(DeviceState *dev)
> @@ -714,7 +852,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),
> @@ -726,6 +868,10 @@ static uint64_t get_lsa_size(CXLType3Dev *ct3d)
>  {
>      MemoryRegion *mr;
>  
> +    if (!ct3d->lsa) {
> +        return 0;
> +    }
> +
>      mr = host_memory_backend_get_memory(ct3d->lsa);
>      return memory_region_size(mr);
>  }
> @@ -743,6 +889,10 @@ static uint64_t get_lsa(CXLType3Dev *ct3d, void *buf, uint64_t size,
>      MemoryRegion *mr;
>      void *lsa;
>  
> +    if (!ct3d->lsa) {
> +        return 0;
> +    }
> +
>      mr = host_memory_backend_get_memory(ct3d->lsa);
>      validate_lsa_access(mr, size, offset);
>  
> @@ -758,6 +908,10 @@ static void set_lsa(CXLType3Dev *ct3d, const void *buf, uint64_t size,
>      MemoryRegion *mr;
>      void *lsa;
>  
> +    if (!ct3d->lsa) {
> +        return;
> +    }
> +
>      mr = host_memory_backend_get_memory(ct3d->lsa);
>      validate_lsa_access(mr, size, offset);
>  
> @@ -929,7 +1083,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);
>  
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index d589f78202..edb9791bab 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -119,8 +119,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 */
> @@ -245,12 +247,15 @@ struct CXLType3Dev {
>      PCIDevice parent_obj;
>  
>      /* Properties */
> -    HostMemoryBackend *hostmem;
> +    HostMemoryBackend *hostmem; /* deprecated */
> +    HostMemoryBackend *hostvmem;
> +    HostMemoryBackend *hostpmem;
>      HostMemoryBackend *lsa;
>      uint64_t sn;
>  
>      /* State */
> -    AddressSpace hostmem_as;
> +    AddressSpace hostvmem_as;
> +    AddressSpace hostpmem_as;
>      CXLComponentState cxl_cstate;
>      CXLDeviceState cxl_dstate;
>  
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index 8608408213..b005c03a92 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -1789,13 +1789,13 @@ static void test_acpi_q35_cxl(void)
>                               " -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1"
>                               " -device pxb-cxl,bus_nr=222,bus=pcie.0,id=cxl.2"
>                               " -device cxl-rp,port=0,bus=cxl.1,id=rp1,chassis=0,slot=2"
> -                             " -device cxl-type3,bus=rp1,memdev=cxl-mem1,lsa=lsa1"
> +                             " -device cxl-type3,bus=rp1,persistent-memdev=cxl-mem1,lsa=lsa1"
>                               " -device cxl-rp,port=1,bus=cxl.1,id=rp2,chassis=0,slot=3"
> -                             " -device cxl-type3,bus=rp2,memdev=cxl-mem2,lsa=lsa2"
> +                             " -device cxl-type3,bus=rp2,persistent-memdev=cxl-mem2,lsa=lsa2"
>                               " -device cxl-rp,port=0,bus=cxl.2,id=rp3,chassis=0,slot=5"
> -                             " -device cxl-type3,bus=rp3,memdev=cxl-mem3,lsa=lsa3"
> +                             " -device cxl-type3,bus=rp3,persistent-memdev=cxl-mem3,lsa=lsa3"
>                               " -device cxl-rp,port=1,bus=cxl.2,id=rp4,chassis=0,slot=6"
> -                             " -device cxl-type3,bus=rp4,memdev=cxl-mem4,lsa=lsa4"
> +                             " -device cxl-type3,bus=rp4,persistent-memdev=cxl-mem4,lsa=lsa4"
>                               " -M cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.size=4G,cxl-fmw.0.interleave-granularity=8k,"
>                               "cxl-fmw.1.targets.0=cxl.1,cxl-fmw.1.targets.1=cxl.2,cxl-fmw.1.size=4G,cxl-fmw.1.interleave-granularity=8k",
>                               tmp_path, tmp_path, tmp_path, tmp_path,
> diff --git a/tests/qtest/cxl-test.c b/tests/qtest/cxl-test.c
> index eda2bbbbe6..edcad4a0ce 100644
> --- a/tests/qtest/cxl-test.c
> +++ b/tests/qtest/cxl-test.c
> @@ -34,32 +34,46 @@
>      "-device cxl-rp,id=rp2,bus=cxl.1,chassis=0,slot=2 " \
>      "-device cxl-rp,id=rp3,bus=cxl.1,chassis=0,slot=3 "
>  
> -#define QEMU_T3D \
> +#define QEMU_T3D_DEPRECATED \
>      "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
>      "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
>      "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 "
>  
> +#define QEMU_T3D_PMEM \
> +    "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
> +    "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
> +    "-device cxl-type3,bus=rp0,persistent-memdev=cxl-mem0,lsa=lsa0,id=pmem0 "
> +
> +#define QEMU_T3D_VMEM \
> +    "-object memory-backend-ram,id=cxl-mem0,size=256M " \
> +    "-device cxl-type3,bus=rp0,volatile-memdev=cxl-mem0,id=mem0 "
> +
> +#define QEMU_T3D_VMEM_LSA \
> +    "-object memory-backend-ram,id=cxl-mem0,size=256M " \
> +    "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
> +    "-device cxl-type3,bus=rp0,volatile-memdev=cxl-mem0,lsa=lsa0,id=mem0 "
> +
>  #define QEMU_2T3D \
>      "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
>      "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
> -    "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 " \
> +    "-device cxl-type3,bus=rp0,persistent-memdev=cxl-mem0,lsa=lsa0,id=pmem0 " \
>      "-object memory-backend-file,id=cxl-mem1,mem-path=%s,size=256M " \
>      "-object memory-backend-file,id=lsa1,mem-path=%s,size=256M " \
> -    "-device cxl-type3,bus=rp1,memdev=cxl-mem1,lsa=lsa1,id=cxl-pmem1 "
> +    "-device cxl-type3,bus=rp1,persistent-memdev=cxl-mem1,lsa=lsa1,id=pmem1 "
>  
>  #define QEMU_4T3D \
>      "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
>      "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
> -    "-device cxl-type3,bus=rp0,memdev=cxl-mem0,lsa=lsa0,id=cxl-pmem0 " \
> +    "-device cxl-type3,bus=rp0,persistent-memdev=cxl-mem0,lsa=lsa0,id=pmem0 " \
>      "-object memory-backend-file,id=cxl-mem1,mem-path=%s,size=256M " \
>      "-object memory-backend-file,id=lsa1,mem-path=%s,size=256M " \
> -    "-device cxl-type3,bus=rp1,memdev=cxl-mem1,lsa=lsa1,id=cxl-pmem1 " \
> +    "-device cxl-type3,bus=rp1,persistent-memdev=cxl-mem1,lsa=lsa1,id=pmem1 " \
>      "-object memory-backend-file,id=cxl-mem2,mem-path=%s,size=256M " \
>      "-object memory-backend-file,id=lsa2,mem-path=%s,size=256M " \
> -    "-device cxl-type3,bus=rp2,memdev=cxl-mem2,lsa=lsa2,id=cxl-pmem2 " \
> +    "-device cxl-type3,bus=rp2,persistent-memdev=cxl-mem2,lsa=lsa2,id=pmem2 " \
>      "-object memory-backend-file,id=cxl-mem3,mem-path=%s,size=256M " \
>      "-object memory-backend-file,id=lsa3,mem-path=%s,size=256M " \
> -    "-device cxl-type3,bus=rp3,memdev=cxl-mem3,lsa=lsa3,id=cxl-pmem3 "
> +    "-device cxl-type3,bus=rp3,persistent-memdev=cxl-mem3,lsa=lsa3,id=pmem3 "
>  
>  static void cxl_basic_hb(void)
>  {
> @@ -98,14 +112,53 @@ static void cxl_2root_port(void)
>  }
>  
>  #ifdef CONFIG_POSIX
> -static void cxl_t3d(void)
> +static void cxl_t3d_deprecated(void)
> +{
> +    g_autoptr(GString) cmdline = g_string_new(NULL);
> +    g_autofree const char *tmpfs = NULL;
> +
> +    tmpfs = g_dir_make_tmp("cxl-test-XXXXXX", NULL);
> +
> +    g_string_printf(cmdline, QEMU_PXB_CMD QEMU_RP QEMU_T3D_DEPRECATED,
> +                    tmpfs, tmpfs);
> +
> +    qtest_start(cmdline->str);
> +    qtest_end();
> +}
> +
> +static void cxl_t3d_persistent(void)
> +{
> +    g_autoptr(GString) cmdline = g_string_new(NULL);
> +    g_autofree const char *tmpfs = NULL;
> +
> +    tmpfs = g_dir_make_tmp("cxl-test-XXXXXX", NULL);
> +
> +    g_string_printf(cmdline, QEMU_PXB_CMD QEMU_RP QEMU_T3D_PMEM,
> +                    tmpfs, tmpfs);
> +
> +    qtest_start(cmdline->str);
> +    qtest_end();
> +}
> +
> +static void cxl_t3d_volatile(void)
> +{
> +    g_autoptr(GString) cmdline = g_string_new(NULL);
> +
> +    g_string_printf(cmdline, QEMU_PXB_CMD QEMU_RP QEMU_T3D_VMEM);
> +
> +    qtest_start(cmdline->str);
> +    qtest_end();
> +}
> +
> +static void cxl_t3d_volatile_lsa(void)
>  {
>      g_autoptr(GString) cmdline = g_string_new(NULL);
>      g_autofree const char *tmpfs = NULL;
>  
>      tmpfs = g_dir_make_tmp("cxl-test-XXXXXX", NULL);
>  
> -    g_string_printf(cmdline, QEMU_PXB_CMD QEMU_RP QEMU_T3D, tmpfs, tmpfs);
> +    g_string_printf(cmdline, QEMU_PXB_CMD QEMU_RP QEMU_T3D_VMEM_LSA,
> +                    tmpfs);
>  
>      qtest_start(cmdline->str);
>      qtest_end();
> @@ -155,7 +208,10 @@ int main(int argc, char **argv)
>      qtest_add_func("/pci/cxl/rp", cxl_root_port);
>      qtest_add_func("/pci/cxl/rp_x2", cxl_2root_port);
>  #ifdef CONFIG_POSIX
> -    qtest_add_func("/pci/cxl/type3_device", cxl_t3d);
> +    qtest_add_func("/pci/cxl/type3_device", cxl_t3d_deprecated);
> +    qtest_add_func("/pci/cxl/type3_device_pmem", cxl_t3d_persistent);
> +    qtest_add_func("/pci/cxl/type3_device_vmem", cxl_t3d_volatile);
> +    qtest_add_func("/pci/cxl/type3_device_vmem_lsa", cxl_t3d_volatile_lsa);
>      qtest_add_func("/pci/cxl/rp_x2_type3_x2", cxl_1pxb_2rp_2t3d);
>      qtest_add_func("/pci/cxl/pxb_x2_root_port_x4_type3_x4", cxl_2pxb_4rp_4t3d);
>  #endif
> -- 
> 2.37.2
> 
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent)
  2023-02-17 16:16     ` Jonathan Cameron
  (?)
@ 2023-02-17 11:08     ` Gregory Price
  2023-02-20 11:46         ` Jonathan Cameron via
  -1 siblings, 1 reply; 18+ messages in thread
From: Gregory Price @ 2023-02-17 11:08 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Jonathan Cameron via, Michael Tsirkin, Ben Widawsky, linux-cxl,
	linuxarm, Ira Weiny, Gregory Price, Philippe Mathieu-Daudé,
	Mike Maslenkin

On Fri, Feb 17, 2023 at 04:16:17PM +0000, Jonathan Cameron via wrote:
> On Tue, 31 Jan 2023 16:38:47 +0000
> Jonathan Cameron via <qemu-devel@nongnu.org> 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>
> > 
> Hi Gregory,
> 
> I've added support for multiple HDM decoders and hence can now
> test both volatile and non volatile on same device.
> It very nearly all works. With one exception which is I couldn't
> poke the first byte of the non volatile region.
> 
> I think we have an off by one in a single check.
> 
> Interestingly it makes no difference when creating an FS on top
> (which was my standard test) so I only noticed when poking memory
> addresses directly to sanity check the HDM decoder setup.
> 
> I'll roll a v2 if no one shouts out that I'm wrong.
> 
> Note that adding multiple HDM decoders massively increases
> the number of test cases over what we had before to poke all the
> corners so I may well be missing stuff.  Hopefully can send an RFC
> of that support out next week.
> 
> Jonathan
> 

Very cool! Thanks for pushing this over the finishing line.

All my testing so far has been really smooth since getting the TCG issue
worked out.

> > -MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
> > -                           unsigned size, MemTxAttrs attrs)
[...]
> > +    if (vmr) {
> > +        if (*dpa_offset <= int128_get64(vmr->size)) {
> 
> Off by one I think.  < 
> 

Yes that makes sense, should be <.  Derp derp.

Though I think this may alludes to more off-by-one issues?  This says

if (dpa_offset < vmr->size)

but dpa_offset should be (hpa - memory_region_base),

The HPA is used by memory access routing for the whole system to determine
what device it should access.

If that corner case is being hit, doesn't it imply the higher level code
is also susceptible to this, and is routing accesses to the wrong device?

~Gregory

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent)
  2023-01-31 16:38   ` Jonathan Cameron via
@ 2023-02-17 16:16     ` Jonathan Cameron
  -1 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron via @ 2023-02-17 16:16 UTC (permalink / raw)
  To: Jonathan Cameron via
  Cc: Jonathan Cameron, Michael Tsirkin, Ben Widawsky, linux-cxl,
	linuxarm, Ira Weiny, Gregory Price, Philippe Mathieu-Daudé,
	Mike Maslenkin

On Tue, 31 Jan 2023 16:38:47 +0000
Jonathan Cameron via <qemu-devel@nongnu.org> 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>
> 
Hi Gregory,

I've added support for multiple HDM decoders and hence can now
test both volatile and non volatile on same device.
It very nearly all works. With one exception which is I couldn't
poke the first byte of the non volatile region.

I think we have an off by one in a single check.

Interestingly it makes no difference when creating an FS on top
(which was my standard test) so I only noticed when poking memory
addresses directly to sanity check the HDM decoder setup.

I'll roll a v2 if no one shouts out that I'm wrong.

Note that adding multiple HDM decoders massively increases
the number of test cases over what we had before to poke all the
corners so I may well be missing stuff.  Hopefully can send an RFC
of that support out next week.

Jonathan

> -MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
> -                           unsigned size, MemTxAttrs attrs)
> +static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d,
> +                                       hwaddr host_addr,
> +                                       unsigned int size,
> +                                       AddressSpace **as,
> +                                       uint64_t *dpa_offset)
>  {
> -    CXLType3Dev *ct3d = CXL_TYPE3(d);
> -    uint64_t dpa_offset;
> -    MemoryRegion *mr;
> +    MemoryRegion *vmr = NULL, *pmr = NULL;
>  
> -    /* TODO support volatile region */
> -    mr = host_memory_backend_get_memory(ct3d->hostmem);
> -    if (!mr) {
> -        return MEMTX_ERROR;
> +    if (ct3d->hostvmem) {
> +        vmr = host_memory_backend_get_memory(ct3d->hostvmem);
> +    }
> +    if (ct3d->hostpmem) {
> +        pmr = host_memory_backend_get_memory(ct3d->hostpmem);
>      }
>  
> -    if (!cxl_type3_dpa(ct3d, host_addr, &dpa_offset)) {
> -        return MEMTX_ERROR;
> +    if (!vmr && !pmr) {
> +        return -ENODEV;
> +    }
> +
> +    if (!cxl_type3_dpa(ct3d, host_addr, dpa_offset)) {
> +        return -EINVAL;
> +    }
> +
> +    if (*dpa_offset > int128_get64(ct3d->cxl_dstate.mem_size)) {
> +        return -EINVAL;
> +    }
> +
> +    if (vmr) {
> +        if (*dpa_offset <= int128_get64(vmr->size)) {

Off by one I think.  < 

> +            *as = &ct3d->hostvmem_as;
> +        } else {
> +            *as = &ct3d->hostpmem_as;
> +            *dpa_offset -= vmr->size;
> +        }
> +    } else {
> +        *as = &ct3d->hostpmem_as;
>      }
>  
> -    if (dpa_offset > int128_get64(mr->size)) {
> +    return 0;
> +}


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent)
@ 2023-02-17 16:16     ` Jonathan Cameron
  0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2023-02-17 16:16 UTC (permalink / raw)
  To: Jonathan Cameron via
  Cc: Jonathan Cameron, Michael Tsirkin, Ben Widawsky, linux-cxl,
	linuxarm, Ira Weiny, Gregory Price, Philippe Mathieu-Daudé,
	Mike Maslenkin

On Tue, 31 Jan 2023 16:38:47 +0000
Jonathan Cameron via <qemu-devel@nongnu.org> 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>
> 
Hi Gregory,

I've added support for multiple HDM decoders and hence can now
test both volatile and non volatile on same device.
It very nearly all works. With one exception which is I couldn't
poke the first byte of the non volatile region.

I think we have an off by one in a single check.

Interestingly it makes no difference when creating an FS on top
(which was my standard test) so I only noticed when poking memory
addresses directly to sanity check the HDM decoder setup.

I'll roll a v2 if no one shouts out that I'm wrong.

Note that adding multiple HDM decoders massively increases
the number of test cases over what we had before to poke all the
corners so I may well be missing stuff.  Hopefully can send an RFC
of that support out next week.

Jonathan

> -MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
> -                           unsigned size, MemTxAttrs attrs)
> +static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d,
> +                                       hwaddr host_addr,
> +                                       unsigned int size,
> +                                       AddressSpace **as,
> +                                       uint64_t *dpa_offset)
>  {
> -    CXLType3Dev *ct3d = CXL_TYPE3(d);
> -    uint64_t dpa_offset;
> -    MemoryRegion *mr;
> +    MemoryRegion *vmr = NULL, *pmr = NULL;
>  
> -    /* TODO support volatile region */
> -    mr = host_memory_backend_get_memory(ct3d->hostmem);
> -    if (!mr) {
> -        return MEMTX_ERROR;
> +    if (ct3d->hostvmem) {
> +        vmr = host_memory_backend_get_memory(ct3d->hostvmem);
> +    }
> +    if (ct3d->hostpmem) {
> +        pmr = host_memory_backend_get_memory(ct3d->hostpmem);
>      }
>  
> -    if (!cxl_type3_dpa(ct3d, host_addr, &dpa_offset)) {
> -        return MEMTX_ERROR;
> +    if (!vmr && !pmr) {
> +        return -ENODEV;
> +    }
> +
> +    if (!cxl_type3_dpa(ct3d, host_addr, dpa_offset)) {
> +        return -EINVAL;
> +    }
> +
> +    if (*dpa_offset > int128_get64(ct3d->cxl_dstate.mem_size)) {
> +        return -EINVAL;
> +    }
> +
> +    if (vmr) {
> +        if (*dpa_offset <= int128_get64(vmr->size)) {

Off by one I think.  < 

> +            *as = &ct3d->hostvmem_as;
> +        } else {
> +            *as = &ct3d->hostpmem_as;
> +            *dpa_offset -= vmr->size;
> +        }
> +    } else {
> +        *as = &ct3d->hostpmem_as;
>      }
>  
> -    if (dpa_offset > int128_get64(mr->size)) {
> +    return 0;
> +}

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent)
  2023-02-17 11:08     ` Gregory Price
@ 2023-02-20 11:46         ` Jonathan Cameron via
  0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2023-02-20 11:46 UTC (permalink / raw)
  To: Gregory Price
  Cc: Jonathan Cameron via, Michael Tsirkin, Ben Widawsky, linux-cxl,
	linuxarm, Ira Weiny, Gregory Price, Philippe Mathieu-Daudé,
	Mike Maslenkin

On Fri, 17 Feb 2023 06:08:57 -0500
Gregory Price <gregory.price@memverge.com> wrote:

> On Fri, Feb 17, 2023 at 04:16:17PM +0000, Jonathan Cameron via wrote:
> > On Tue, 31 Jan 2023 16:38:47 +0000
> > Jonathan Cameron via <qemu-devel@nongnu.org> 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>
> > >   
> > Hi Gregory,
> > 
> > I've added support for multiple HDM decoders and hence can now
> > test both volatile and non volatile on same device.
> > It very nearly all works. With one exception which is I couldn't
> > poke the first byte of the non volatile region.
> > 
> > I think we have an off by one in a single check.
> > 
> > Interestingly it makes no difference when creating an FS on top
> > (which was my standard test) so I only noticed when poking memory
> > addresses directly to sanity check the HDM decoder setup.
> > 
> > I'll roll a v2 if no one shouts out that I'm wrong.
> > 
> > Note that adding multiple HDM decoders massively increases
> > the number of test cases over what we had before to poke all the
> > corners so I may well be missing stuff.  Hopefully can send an RFC
> > of that support out next week.
> > 
> > Jonathan
> >   
> 
> Very cool! Thanks for pushing this over the finishing line.
> 
> All my testing so far has been really smooth since getting the TCG issue
> worked out.
> 
> > > -MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
> > > -                           unsigned size, MemTxAttrs attrs)  
> [...]
> > > +    if (vmr) {
> > > +        if (*dpa_offset <= int128_get64(vmr->size)) {  
> > 
> > Off by one I think.  < 
> >   
> 
> Yes that makes sense, should be <.  Derp derp.
> 
> Though I think this may alludes to more off-by-one issues?  This says
> 
> if (dpa_offset < vmr->size)
> 
> but dpa_offset should be (hpa - memory_region_base),
> 
> The HPA is used by memory access routing for the whole system to determine
> what device it should access.
> 
> If that corner case is being hit, doesn't it imply the higher level code
> is also susceptible to this, and is routing accesses to the wrong device?

I don't think so though I may be missing something. 

Say vmr->size = 8

hpa  dpa_offset
0       0
1       1
2       2
3       3
4       4
5       5
6       6
7       7
8       0

etc

Also the writes are turning up where I expect them to.

Also just noticed that the code is setting Memory_base in the CXL dvsec.
Given we are emulating a device as if it has been freshly powered up
those should not be set - it's the OS or firmware's job to set them up.
Harmless though, so can be a cleanup to follow the main series.

We don't currently handle dvsec range based routing anyway and
I'm not sure we ever will as it is a pain to test without some firmware
or OS code to program them for us.

Note that if you update your kernel to cxl/next it will currently fail
as the Range register emulation is (I think) rather over enthusiastic
and currently decides to emulate the HDM decoders for the QEMU emulated
type 3 devices.

https://lore.kernel.org/linux-cxl/167640366272.935665.1056268838301725481.stgit@dwillia2-xfh.jf.intel.com/T/#m6c025d5c9b27d8360a64079593f6c5adaa408772

Jonathan


> 
> ~Gregory


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent)
@ 2023-02-20 11:46         ` Jonathan Cameron via
  0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron via @ 2023-02-20 11:46 UTC (permalink / raw)
  To: Gregory Price
  Cc: Jonathan Cameron via, Michael Tsirkin, Ben Widawsky, linux-cxl,
	linuxarm, Ira Weiny, Gregory Price, Philippe Mathieu-Daudé,
	Mike Maslenkin

On Fri, 17 Feb 2023 06:08:57 -0500
Gregory Price <gregory.price@memverge.com> wrote:

> On Fri, Feb 17, 2023 at 04:16:17PM +0000, Jonathan Cameron via wrote:
> > On Tue, 31 Jan 2023 16:38:47 +0000
> > Jonathan Cameron via <qemu-devel@nongnu.org> 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>
> > >   
> > Hi Gregory,
> > 
> > I've added support for multiple HDM decoders and hence can now
> > test both volatile and non volatile on same device.
> > It very nearly all works. With one exception which is I couldn't
> > poke the first byte of the non volatile region.
> > 
> > I think we have an off by one in a single check.
> > 
> > Interestingly it makes no difference when creating an FS on top
> > (which was my standard test) so I only noticed when poking memory
> > addresses directly to sanity check the HDM decoder setup.
> > 
> > I'll roll a v2 if no one shouts out that I'm wrong.
> > 
> > Note that adding multiple HDM decoders massively increases
> > the number of test cases over what we had before to poke all the
> > corners so I may well be missing stuff.  Hopefully can send an RFC
> > of that support out next week.
> > 
> > Jonathan
> >   
> 
> Very cool! Thanks for pushing this over the finishing line.
> 
> All my testing so far has been really smooth since getting the TCG issue
> worked out.
> 
> > > -MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
> > > -                           unsigned size, MemTxAttrs attrs)  
> [...]
> > > +    if (vmr) {
> > > +        if (*dpa_offset <= int128_get64(vmr->size)) {  
> > 
> > Off by one I think.  < 
> >   
> 
> Yes that makes sense, should be <.  Derp derp.
> 
> Though I think this may alludes to more off-by-one issues?  This says
> 
> if (dpa_offset < vmr->size)
> 
> but dpa_offset should be (hpa - memory_region_base),
> 
> The HPA is used by memory access routing for the whole system to determine
> what device it should access.
> 
> If that corner case is being hit, doesn't it imply the higher level code
> is also susceptible to this, and is routing accesses to the wrong device?

I don't think so though I may be missing something. 

Say vmr->size = 8

hpa  dpa_offset
0       0
1       1
2       2
3       3
4       4
5       5
6       6
7       7
8       0

etc

Also the writes are turning up where I expect them to.

Also just noticed that the code is setting Memory_base in the CXL dvsec.
Given we are emulating a device as if it has been freshly powered up
those should not be set - it's the OS or firmware's job to set them up.
Harmless though, so can be a cleanup to follow the main series.

We don't currently handle dvsec range based routing anyway and
I'm not sure we ever will as it is a pain to test without some firmware
or OS code to program them for us.

Note that if you update your kernel to cxl/next it will currently fail
as the Range register emulation is (I think) rather over enthusiastic
and currently decides to emulate the HDM decoders for the QEMU emulated
type 3 devices.

https://lore.kernel.org/linux-cxl/167640366272.935665.1056268838301725481.stgit@dwillia2-xfh.jf.intel.com/T/#m6c025d5c9b27d8360a64079593f6c5adaa408772

Jonathan


> 
> ~Gregory



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent)
       [not found]         ` <CGME20230224002948uscas1p220ab1ec9fbe16138e0fd314b7412f833@uscas1p2.samsung.com>
@ 2023-02-24  0:29           ` Fan Ni
  2023-02-24 12:01               ` Jonathan Cameron via
  0 siblings, 1 reply; 18+ messages in thread
From: Fan Ni @ 2023-02-24  0:29 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Gregory Price, Jonathan Cameron via, Michael Tsirkin,
	Ben Widawsky, linux-cxl, linuxarm, Ira Weiny, Gregory Price,
	Philippe Mathieu-Daudé,
	Mike Maslenkin

On Mon, Feb 20, 2023 at 11:46:46AM +0000, Jonathan Cameron wrote:

> On Fri, 17 Feb 2023 06:08:57 -0500
> Gregory Price <gregory.price@memverge.com> wrote:
> 
> > On Fri, Feb 17, 2023 at 04:16:17PM +0000, Jonathan Cameron via wrote:
> > > On Tue, 31 Jan 2023 16:38:47 +0000
> > > Jonathan Cameron via <qemu-devel@nongnu.org> 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>
> > > >   
> > > Hi Gregory,
> > > 
> > > I've added support for multiple HDM decoders and hence can now
> > > test both volatile and non volatile on same device.
> > > It very nearly all works. With one exception which is I couldn't
> > > poke the first byte of the non volatile region.
> > > 
> > > I think we have an off by one in a single check.
> > > 
> > > Interestingly it makes no difference when creating an FS on top
> > > (which was my standard test) so I only noticed when poking memory
> > > addresses directly to sanity check the HDM decoder setup.
> > > 
> > > I'll roll a v2 if no one shouts out that I'm wrong.
> > > 
> > > Note that adding multiple HDM decoders massively increases
> > > the number of test cases over what we had before to poke all the
> > > corners so I may well be missing stuff.  Hopefully can send an RFC
> > > of that support out next week.
> > > 
> > > Jonathan
> > >   
> > 
> > Very cool! Thanks for pushing this over the finishing line.
> > 
> > All my testing so far has been really smooth since getting the TCG issue
> > worked out.
> > 
> > > > -MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
> > > > -                           unsigned size, MemTxAttrs attrs)  
> > [...]
> > > > +    if (vmr) {
> > > > +        if (*dpa_offset <= int128_get64(vmr->size)) {  
> > > 
> > > Off by one I think.  < 
> > >   
> > 
> > Yes that makes sense, should be <.  Derp derp.
> > 
> > Though I think this may alludes to more off-by-one issues?  This says
> > 
> > if (dpa_offset < vmr->size)
> > 
> > but dpa_offset should be (hpa - memory_region_base),
> > 
> > The HPA is used by memory access routing for the whole system to determine
> > what device it should access.
> > 
> > If that corner case is being hit, doesn't it imply the higher level code
> > is also susceptible to this, and is routing accesses to the wrong device?
> 
> I don't think so though I may be missing something. 
> 
> Say vmr->size = 8
> 
> hpa  dpa_offset
> 0       0
> 1       1
> 2       2
> 3       3
> 4       4
> 5       5
> 6       6
> 7       7
> 8       0
> 
> etc

If vmr->size=8, we should expect that at most 8 offsets (e.g., hpa [0,7]) will
access vmem and hpa=8 will access offset=0 at pmem, right?
If with hpa=8, dpa_offset round to 0, does it mean we never get a
dpa_offset larger than 7? If so, when pmem will be accessed?

> 
> Also the writes are turning up where I expect them to.
> 
> Also just noticed that the code is setting Memory_base in the CXL dvsec.
> Given we are emulating a device as if it has been freshly powered up
> those should not be set - it's the OS or firmware's job to set them up.
> Harmless though, so can be a cleanup to follow the main series.
> 
> We don't currently handle dvsec range based routing anyway and
> I'm not sure we ever will as it is a pain to test without some firmware
> or OS code to program them for us.
> 
> Note that if you update your kernel to cxl/next it will currently fail
> as the Range register emulation is (I think) rather over enthusiastic
> and currently decides to emulate the HDM decoders for the QEMU emulated
> type 3 devices.
> 
> https://lore.kernel.org/linux-cxl/167640366272.935665.1056268838301725481.stgit@dwillia2-xfh.jf.intel.com/T/#m6c025d5c9b27d8360a64079593f6c5adaa408772
> 
> Jonathan
> 
> 
> > 
> > ~Gregory
> 
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent)
  2023-02-24  0:29           ` Fan Ni
@ 2023-02-24 12:01               ` Jonathan Cameron via
  0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2023-02-24 12:01 UTC (permalink / raw)
  To: Fan Ni
  Cc: Gregory Price, Jonathan Cameron via, Michael Tsirkin,
	Ben Widawsky, linux-cxl, linuxarm, Ira Weiny, Gregory Price,
	Philippe Mathieu-Daudé,
	Mike Maslenkin

On Fri, 24 Feb 2023 00:29:47 +0000
Fan Ni <fan.ni@samsung.com> wrote:

> On Mon, Feb 20, 2023 at 11:46:46AM +0000, Jonathan Cameron wrote:
> 
> > On Fri, 17 Feb 2023 06:08:57 -0500
> > Gregory Price <gregory.price@memverge.com> wrote:
> >   
> > > On Fri, Feb 17, 2023 at 04:16:17PM +0000, Jonathan Cameron via wrote:  
> > > > On Tue, 31 Jan 2023 16:38:47 +0000
> > > > Jonathan Cameron via <qemu-devel@nongnu.org> 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>
> > > > >     
> > > > Hi Gregory,
> > > > 
> > > > I've added support for multiple HDM decoders and hence can now
> > > > test both volatile and non volatile on same device.
> > > > It very nearly all works. With one exception which is I couldn't
> > > > poke the first byte of the non volatile region.
> > > > 
> > > > I think we have an off by one in a single check.
> > > > 
> > > > Interestingly it makes no difference when creating an FS on top
> > > > (which was my standard test) so I only noticed when poking memory
> > > > addresses directly to sanity check the HDM decoder setup.
> > > > 
> > > > I'll roll a v2 if no one shouts out that I'm wrong.
> > > > 
> > > > Note that adding multiple HDM decoders massively increases
> > > > the number of test cases over what we had before to poke all the
> > > > corners so I may well be missing stuff.  Hopefully can send an RFC
> > > > of that support out next week.
> > > > 
> > > > Jonathan
> > > >     
> > > 
> > > Very cool! Thanks for pushing this over the finishing line.
> > > 
> > > All my testing so far has been really smooth since getting the TCG issue
> > > worked out.
> > >   
> > > > > -MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
> > > > > -                           unsigned size, MemTxAttrs attrs)    
> > > [...]  
> > > > > +    if (vmr) {
> > > > > +        if (*dpa_offset <= int128_get64(vmr->size)) {    
> > > > 
> > > > Off by one I think.  < 
> > > >     
> > > 
> > > Yes that makes sense, should be <.  Derp derp.
> > > 
> > > Though I think this may alludes to more off-by-one issues?  This says
> > > 
> > > if (dpa_offset < vmr->size)
> > > 
> > > but dpa_offset should be (hpa - memory_region_base),
> > > 
> > > The HPA is used by memory access routing for the whole system to determine
> > > what device it should access.
> > > 
> > > If that corner case is being hit, doesn't it imply the higher level code
> > > is also susceptible to this, and is routing accesses to the wrong device?  
> > 
> > I don't think so though I may be missing something. 
> > 
> > Say vmr->size = 8
> > 
> > hpa  dpa_offset
> > 0       0
> > 1       1
> > 2       2
> > 3       3
> > 4       4
> > 5       5
> > 6       6
> > 7       7
> > 8       0
> > 
> > etc  
> 
> If vmr->size=8, we should expect that at most 8 offsets (e.g., hpa [0,7]) will
> access vmem and hpa=8 will access offset=0 at pmem, right?
> If with hpa=8, dpa_offset round to 0, does it mean we never get a
> dpa_offset larger than 7? If so, when pmem will be accessed?

Ah. I didn't mention that when HPA == 8 in this example the dpa_offset is
now used to access pmem until we reach vmr->size + persistent_mr->size at
which point it is off the end and it's routed no where at all.

Table is too simplisitic!

> 
> > 
> > Also the writes are turning up where I expect them to.
> > 
> > Also just noticed that the code is setting Memory_base in the CXL dvsec.
> > Given we are emulating a device as if it has been freshly powered up
> > those should not be set - it's the OS or firmware's job to set them up.
> > Harmless though, so can be a cleanup to follow the main series.
> > 
> > We don't currently handle dvsec range based routing anyway and
> > I'm not sure we ever will as it is a pain to test without some firmware
> > or OS code to program them for us.
> > 
> > Note that if you update your kernel to cxl/next it will currently fail
> > as the Range register emulation is (I think) rather over enthusiastic
> > and currently decides to emulate the HDM decoders for the QEMU emulated
> > type 3 devices.
> > 
> > https://lore.kernel.org/linux-cxl/167640366272.935665.1056268838301725481.stgit@dwillia2-xfh.jf.intel.com/T/#m6c025d5c9b27d8360a64079593f6c5adaa408772
> > 
> > Jonathan
> > 
> >   
> > > 
> > > ~Gregory  
> > 
> >  


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/2] hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent)
@ 2023-02-24 12:01               ` Jonathan Cameron via
  0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron via @ 2023-02-24 12:01 UTC (permalink / raw)
  To: Fan Ni
  Cc: Gregory Price, Jonathan Cameron via, Michael Tsirkin,
	Ben Widawsky, linux-cxl, linuxarm, Ira Weiny, Gregory Price,
	Philippe Mathieu-Daudé,
	Mike Maslenkin

On Fri, 24 Feb 2023 00:29:47 +0000
Fan Ni <fan.ni@samsung.com> wrote:

> On Mon, Feb 20, 2023 at 11:46:46AM +0000, Jonathan Cameron wrote:
> 
> > On Fri, 17 Feb 2023 06:08:57 -0500
> > Gregory Price <gregory.price@memverge.com> wrote:
> >   
> > > On Fri, Feb 17, 2023 at 04:16:17PM +0000, Jonathan Cameron via wrote:  
> > > > On Tue, 31 Jan 2023 16:38:47 +0000
> > > > Jonathan Cameron via <qemu-devel@nongnu.org> 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>
> > > > >     
> > > > Hi Gregory,
> > > > 
> > > > I've added support for multiple HDM decoders and hence can now
> > > > test both volatile and non volatile on same device.
> > > > It very nearly all works. With one exception which is I couldn't
> > > > poke the first byte of the non volatile region.
> > > > 
> > > > I think we have an off by one in a single check.
> > > > 
> > > > Interestingly it makes no difference when creating an FS on top
> > > > (which was my standard test) so I only noticed when poking memory
> > > > addresses directly to sanity check the HDM decoder setup.
> > > > 
> > > > I'll roll a v2 if no one shouts out that I'm wrong.
> > > > 
> > > > Note that adding multiple HDM decoders massively increases
> > > > the number of test cases over what we had before to poke all the
> > > > corners so I may well be missing stuff.  Hopefully can send an RFC
> > > > of that support out next week.
> > > > 
> > > > Jonathan
> > > >     
> > > 
> > > Very cool! Thanks for pushing this over the finishing line.
> > > 
> > > All my testing so far has been really smooth since getting the TCG issue
> > > worked out.
> > >   
> > > > > -MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
> > > > > -                           unsigned size, MemTxAttrs attrs)    
> > > [...]  
> > > > > +    if (vmr) {
> > > > > +        if (*dpa_offset <= int128_get64(vmr->size)) {    
> > > > 
> > > > Off by one I think.  < 
> > > >     
> > > 
> > > Yes that makes sense, should be <.  Derp derp.
> > > 
> > > Though I think this may alludes to more off-by-one issues?  This says
> > > 
> > > if (dpa_offset < vmr->size)
> > > 
> > > but dpa_offset should be (hpa - memory_region_base),
> > > 
> > > The HPA is used by memory access routing for the whole system to determine
> > > what device it should access.
> > > 
> > > If that corner case is being hit, doesn't it imply the higher level code
> > > is also susceptible to this, and is routing accesses to the wrong device?  
> > 
> > I don't think so though I may be missing something. 
> > 
> > Say vmr->size = 8
> > 
> > hpa  dpa_offset
> > 0       0
> > 1       1
> > 2       2
> > 3       3
> > 4       4
> > 5       5
> > 6       6
> > 7       7
> > 8       0
> > 
> > etc  
> 
> If vmr->size=8, we should expect that at most 8 offsets (e.g., hpa [0,7]) will
> access vmem and hpa=8 will access offset=0 at pmem, right?
> If with hpa=8, dpa_offset round to 0, does it mean we never get a
> dpa_offset larger than 7? If so, when pmem will be accessed?

Ah. I didn't mention that when HPA == 8 in this example the dpa_offset is
now used to access pmem until we reach vmr->size + persistent_mr->size at
which point it is off the end and it's routed no where at all.

Table is too simplisitic!

> 
> > 
> > Also the writes are turning up where I expect them to.
> > 
> > Also just noticed that the code is setting Memory_base in the CXL dvsec.
> > Given we are emulating a device as if it has been freshly powered up
> > those should not be set - it's the OS or firmware's job to set them up.
> > Harmless though, so can be a cleanup to follow the main series.
> > 
> > We don't currently handle dvsec range based routing anyway and
> > I'm not sure we ever will as it is a pain to test without some firmware
> > or OS code to program them for us.
> > 
> > Note that if you update your kernel to cxl/next it will currently fail
> > as the Range register emulation is (I think) rather over enthusiastic
> > and currently decides to emulate the HDM decoders for the QEMU emulated
> > type 3 devices.
> > 
> > https://lore.kernel.org/linux-cxl/167640366272.935665.1056268838301725481.stgit@dwillia2-xfh.jf.intel.com/T/#m6c025d5c9b27d8360a64079593f6c5adaa408772
> > 
> > Jonathan
> > 
> >   
> > > 
> > > ~Gregory  
> > 
> >  



^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2023-02-24 12:03 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-31 16:38 [PATCH 0/2] hw/mem: CXL Type-3 Volatile Memory Support Jonathan Cameron
2023-01-31 16:38 ` Jonathan Cameron via
2023-01-31 16:38 ` [PATCH 1/2] tests/qtest/cxl-test: whitespace, line ending cleanup Jonathan Cameron
2023-01-31 16:38   ` Jonathan Cameron via
2023-01-31 16:08   ` Gregory Price
2023-01-31 16:38 ` [PATCH 2/2] hw/cxl: Multi-Region CXL Type-3 Devices (Volatile and Persistent) Jonathan Cameron
2023-01-31 16:38   ` Jonathan Cameron via
2023-01-31 16:25   ` Gregory Price
2023-02-14 18:15   ` Davidlohr Bueso
     [not found]   ` <CGME20230216184225uscas1p2bb2f60fe3d6f0d1f8d9ffe2ff377e190@uscas1p2.samsung.com>
2023-02-16 18:42     ` Fan Ni
2023-02-17 16:16   ` Jonathan Cameron via
2023-02-17 16:16     ` Jonathan Cameron
2023-02-17 11:08     ` Gregory Price
2023-02-20 11:46       ` Jonathan Cameron
2023-02-20 11:46         ` Jonathan Cameron via
     [not found]         ` <CGME20230224002948uscas1p220ab1ec9fbe16138e0fd314b7412f833@uscas1p2.samsung.com>
2023-02-24  0:29           ` Fan Ni
2023-02-24 12:01             ` Jonathan Cameron
2023-02-24 12:01               ` Jonathan Cameron via

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.