[PATCH] hw/mem: support zero memory size CXL device

[PATCH] hw/mem: support zero memory size CXL device

[Hongjian Fan]

According to CXL 3.1 specification, 8.1.3.8 DVSEC CXL Range Registers "A CXL.mem-capable device is permitted to report zero memory size."
This patch will allow a CXL type3 device to be initialized with zero memory size, when there is no memory device property provided ( neither volatile, persistent, nor dynamic region).

Signed-off-by: Hongjian Fan <hongjian.fan@seagate.com>
---
 hw/mem/cxl_type3.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 5cf754b38f..35caa186ca 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -159,7 +159,12 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
     int len = 0;
 
     if (!ct3d->hostpmem && !ct3d->hostvmem && !ct3d->dc.num_regions) {
-        return 0;
+        // zero memory size device. Build one entry with size 0
+        table = g_malloc0(CT3_CDAT_NUM_ENTRIES * sizeof(*table));
+        ct3_build_cdat_entries_for_mr(&(table[0]), dsmad_handle++,
+                                0, false, false, 0);
+        *cdat_table = g_steal_pointer(&table);
+        return CT3_CDAT_NUM_ENTRIES;
     }
 
     if (ct3d->hostvmem) {
@@ -712,8 +717,11 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
 
     if (!ct3d->hostmem && !ct3d->hostvmem && !ct3d->hostpmem
         && !ct3d->dc.num_regions) {
-        error_setg(errp, "at least one memdev property must be set");
-        return false;
+        // no memdev property provided. Default to zero memory size device
+        ct3d->cxl_dstate.pmem_size = 0;
+        ct3d->cxl_dstate.vmem_size = 0;
+        ct3d->cxl_dstate.static_mem_size = 0;
+        return true;
     } else if (ct3d->hostmem && ct3d->hostpmem) {
         error_setg(errp, "[memdev] cannot be used with new "
                          "[persistent-memdev] property");
-- 
2.25.1

Re: [PATCH] hw/mem: support zero memory size CXL device

[Jonathan Cameron]

On Mon,  2 Dec 2024 17:03:11 -0600
Hongjian Fan <hongjian.fan@seagate.com> wrote:

> According to CXL 3.1 specification, 8.1.3.8 DVSEC CXL Range Registers "A CXL.mem-capable device is permitted to report zero memory size."
> This patch will allow a CXL type3 device to be initialized with zero memory size, when there is no memory device property provided ( neither volatile, persistent, nor dynamic region).
Wrap at less than 75 chars.

I'm curious.  Why do you want to emulate such a device?

There are many things the CXL spec allows that we don't emulate.
I'd normally count this as just another one of those.

Jonathan


> 
> Signed-off-by: Hongjian Fan <hongjian.fan@seagate.com>
> ---
>  hw/mem/cxl_type3.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 5cf754b38f..35caa186ca 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -159,7 +159,12 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
>      int len = 0;
>  
>      if (!ct3d->hostpmem && !ct3d->hostvmem && !ct3d->dc.num_regions) {
> -        return 0;
> +        // zero memory size device. Build one entry with size 0

Comment syntax /* */

> +        table = g_malloc0(CT3_CDAT_NUM_ENTRIES * sizeof(*table));
> +        ct3_build_cdat_entries_for_mr(&(table[0]), dsmad_handle++,
> +                                0, false, false, 0);
> +        *cdat_table = g_steal_pointer(&table);
> +        return CT3_CDAT_NUM_ENTRIES;
>      }
>  
>      if (ct3d->hostvmem) {
> @@ -712,8 +717,11 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
>  
>      if (!ct3d->hostmem && !ct3d->hostvmem && !ct3d->hostpmem
>          && !ct3d->dc.num_regions) {
> -        error_setg(errp, "at least one memdev property must be set");
> -        return false;
> +        // no memdev property provided. Default to zero memory size device
> +        ct3d->cxl_dstate.pmem_size = 0;
> +        ct3d->cxl_dstate.vmem_size = 0;
> +        ct3d->cxl_dstate.static_mem_size = 0;
> +        return true;
>      } else if (ct3d->hostmem && ct3d->hostpmem) {
>          error_setg(errp, "[memdev] cannot be used with new "
>                           "[persistent-memdev] property");

Re: [PATCH] hw/mem: support zero memory size CXL device

[Hongjian Fan]

Hi Jonathan,

I'm trying to emulate our memory appliance which is similar to a MH-SLD. The memory device is connected to the host server while the size of the memory could be changed by the out-of-band fabric manager. If there is no memory assigned to the host, the CXL device will be booted as zero memory size.
Recently we got some interest on trying our fabric manager software without having the real hardware. Supporting zero memory size in QEMU will be needed in this scenario.
Some detail about our memory appliance could be found from our OCP presentation: https://drive.google.com/file/d/1i8kBsBfRGjNqnTQqJ9upC-Xm9o56Y2Y5/view?usp=drive_link

Thanks,
Hongjian Fan


Seagate Internal
________________________________________
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Sent: Tuesday, December 3, 2024 11:23 AM
To: Hongjian Fan <hongjian.fan@seagate.com>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; linux-cxl@vger.kernel.org <linux-cxl@vger.kernel.org>; fan.ni@samsung.com <fan.ni@samsung.com>
Subject: Re: [PATCH] hw/mem: support zero memory size CXL device


This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.


On Mon,  2 Dec 2024 17:03:11 -0600
Hongjian Fan <hongjian.fan@seagate.com> wrote:

> According to CXL 3.1 specification, 8.1.3.8 DVSEC CXL Range Registers "A CXL.mem-capable device is permitted to report zero memory size."
> This patch will allow a CXL type3 device to be initialized with zero memory size, when there is no memory device property provided ( neither volatile, persistent, nor dynamic region).
Wrap at less than 75 chars.

I'm curious.  Why do you want to emulate such a device?

There are many things the CXL spec allows that we don't emulate.
I'd normally count this as just another one of those.

Jonathan


>
> Signed-off-by: Hongjian Fan <hongjian.fan@seagate.com>
> ---
>  hw/mem/cxl_type3.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 5cf754b38f..35caa186ca 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -159,7 +159,12 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
>      int len = 0;
>
>      if (!ct3d->hostpmem && !ct3d->hostvmem && !ct3d->dc.num_regions) {
> -        return 0;
> +        // zero memory size device. Build one entry with size 0

Comment syntax /* */

> +        table = g_malloc0(CT3_CDAT_NUM_ENTRIES * sizeof(*table));
> +        ct3_build_cdat_entries_for_mr(&(table[0]), dsmad_handle++,
> +                                0, false, false, 0);
> +        *cdat_table = g_steal_pointer(&table);
> +        return CT3_CDAT_NUM_ENTRIES;
>      }
>
>      if (ct3d->hostvmem) {
> @@ -712,8 +717,11 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
>
>      if (!ct3d->hostmem && !ct3d->hostvmem && !ct3d->hostpmem
>          && !ct3d->dc.num_regions) {
> -        error_setg(errp, "at least one memdev property must be set");
> -        return false;
> +        // no memdev property provided. Default to zero memory size device
> +        ct3d->cxl_dstate.pmem_size = 0;
> +        ct3d->cxl_dstate.vmem_size = 0;
> +        ct3d->cxl_dstate.static_mem_size = 0;
> +        return true;
>      } else if (ct3d->hostmem && ct3d->hostpmem) {
>          error_setg(errp, "[memdev] cannot be used with new "
>                           "[persistent-memdev] property");

Re: [PATCH] hw/mem: support zero memory size CXL device

[Gregory Price]

On Tue, Dec 03, 2024 at 09:15:51PM +0000, Hongjian Fan wrote:
> Hi Jonathan,
> 
> I'm trying to emulate our memory appliance which is similar to a MH-SLD. The memory device is connected to the host server while the size of the memory could be changed by the out-of-band fabric manager. If there is no memory assigned to the host, the CXL device will be booted as zero memory size.

This should not be how this is done.

The ACPI tables should report the maximum possible size, and the DCD
infrastructure should enable physical regions that have been added to the host.

Changing ACPI tables to report 0 memory size will basically result
in the host memory map not reserving physical memory regions for that
device.

See this emulation example of an MHSLD - which can be used for DCD.

https://lore.kernel.org/linux-cxl/20241018161252.8896-1-gourry@gourry.net/

> Recently we got some interest on trying our fabric manager software without having the real hardware. Supporting zero memory size in QEMU will be needed in this scenario.
> Some detail about our memory appliance could be found from our OCP presentation: https://drive.google.com/file/d/1i8kBsBfRGjNqnTQqJ9upC-Xm9o56Y2Y5/view?usp=drive_link
> 
> Thanks,
> Hongjian Fan
> 
> 
> Seagate Internal

Re: [PATCH] hw/mem: support zero memory size CXL device

[Jonathan Cameron]

On Tue, 10 Dec 2024 14:13:29 -0500
Gregory Price <gourry@gourry.net> wrote:

> On Tue, Dec 03, 2024 at 09:15:51PM +0000, Hongjian Fan wrote:
> > Hi Jonathan,
> > 
> > I'm trying to emulate our memory appliance which is similar to a MH-SLD. The memory device is connected to the host server while the size of the memory could be changed by the out-of-band fabric manager. If there is no memory assigned to the host, the CXL device will be booted as zero memory size.  
> 
> This should not be how this is done.
Agreed, but...

It sounds like a pre DCD boot time only pooling solution?

What is the path to adding memory?

> 
> The ACPI tables should report the maximum possible size, and the DCD
> infrastructure should enable physical regions that have been added to the host.
> 
> Changing ACPI tables to report 0 memory size will basically result
> in the host memory map not reserving physical memory regions for that
> device.
This isn't the ACPI bit, it's just the device reporting.  Can have a huge
CFMWS and tiny devices.

> 
> See this emulation example of an MHSLD - which can be used for DCD.
> 
> https://lore.kernel.org/linux-cxl/20241018161252.8896-1-gourry@gourry.net/
> 
> > Recently we got some interest on trying our fabric manager software without having the real hardware. Supporting zero memory size in QEMU will be needed in this scenario.
> > Some detail about our memory appliance could be found from our OCP presentation: https://drive.google.com/file/d/1i8kBsBfRGjNqnTQqJ9upC-Xm9o56Y2Y5/view?usp=drive_link

That doesn't really give me any indication of how the addition of memory happens.

Jonathan

> > 
> > Thanks,
> > Hongjian Fan
> > 
> > 
> > Seagate Internal  

Re: [PATCH] hw/mem: support zero memory size CXL device

[Gregory Price]

On Tue, Dec 24, 2024 at 03:13:15PM +0000, Jonathan Cameron wrote:
> On Tue, 10 Dec 2024 14:13:29 -0500
> Gregory Price <gourry@gourry.net> wrote:
> 
> > On Tue, Dec 03, 2024 at 09:15:51PM +0000, Hongjian Fan wrote:
> > > Hi Jonathan,
> > > 
> > > I'm trying to emulate our memory appliance which is similar to a MH-SLD. The memory device is connected to the host server while the size of the memory could be changed by the out-of-band fabric manager. If there is no memory assigned to the host, the CXL device will be booted as zero memory size.  
> > 
> > This should not be how this is done.
> Agreed, but...
> 
> It sounds like a pre DCD boot time only pooling solution?
> 
> What is the path to adding memory?
>
> > 
> > The ACPI tables should report the maximum possible size, and the DCD
> > infrastructure should enable physical regions that have been added to the host.
> > 
> This isn't the ACPI bit, it's just the device reporting.  Can have a huge
> CFMWS and tiny devices.
> 

If the device is booted as a 0-memory device, how is the CFMWS populated
with the capacity needed to back it? It's all coming from the same
tables.

You poked the right spot in their post - we need more information about
how the memory is added.

If the intent is to just increase/decrease the capacity of the device
on the surface, then the only method I can see here is "tear the device
device down and re-initialize it". At least then - I think? - the driver
will go off and create the appropriate resources.  I've played with this
in QEMU previously in the past, so I *think* that works?

If it doesn't do that, then they'll need to go off and basically
re-invent DCD for their device in the driver (or via fwctl? yikes).



Side note, after thinking about it: a 0-memory device at least lets you
poke all the control bits without having to care about the actual memory
piece, so maybe that's useful regardless.

~Gregory

[PATCH v2] hw/mem: support zero memory size CXL device

[Hongjian Fan]

This patch allows a CXL type3 device to be initialized with zero memory
size, when there is no memory device property provided.

Signed-off-by: Hongjian Fan <hongjian.fan@seagate.com>
---
 hw/mem/cxl_type3.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 5cf754b38f..4c9d6b8f17 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -159,7 +159,12 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
     int len = 0;
 
     if (!ct3d->hostpmem && !ct3d->hostvmem && !ct3d->dc.num_regions) {
-        return 0;
+        /* zero memory size device. Build one entry with size 0 */
+        table = g_malloc0(CT3_CDAT_NUM_ENTRIES * sizeof(*table));
+        ct3_build_cdat_entries_for_mr(&(table[0]), dsmad_handle++,
+                                0, false, false, 0);
+        *cdat_table = g_steal_pointer(&table);
+        return CT3_CDAT_NUM_ENTRIES;
     }
 
     if (ct3d->hostvmem) {
@@ -712,8 +717,11 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
 
     if (!ct3d->hostmem && !ct3d->hostvmem && !ct3d->hostpmem
         && !ct3d->dc.num_regions) {
-        error_setg(errp, "at least one memdev property must be set");
-        return false;
+        /* no memdev property provided. Default to zero memory size device */
+        ct3d->cxl_dstate.pmem_size = 0;
+        ct3d->cxl_dstate.vmem_size = 0;
+        ct3d->cxl_dstate.static_mem_size = 0;
+        return true;
     } else if (ct3d->hostmem && ct3d->hostpmem) {
         error_setg(errp, "[memdev] cannot be used with new "
                          "[persistent-memdev] property");
-- 
2.25.1

Re: [PATCH v2] hw/mem: support zero memory size CXL device

[Jonathan Cameron]

On Tue,  3 Dec 2024 20:26:19 -0600
Hongjian Fan <hongjian.fan@seagate.com> wrote:

> This patch allows a CXL type3 device to be initialized with zero memory
> size, when there is no memory device property provided.
> 
> Signed-off-by: Hongjian Fan <hongjian.fan@seagate.com>
Hi Hongjian

Some remaining questions on the v1 thread.
We need to understand a little more on why this is useful to support.
It's adding another thing we need to test so whilst simple it's not
zero maintenance cost.

Thanks,

Jonathan

> ---
>  hw/mem/cxl_type3.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 5cf754b38f..4c9d6b8f17 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -159,7 +159,12 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
>      int len = 0;
>  
>      if (!ct3d->hostpmem && !ct3d->hostvmem && !ct3d->dc.num_regions) {
> -        return 0;
> +        /* zero memory size device. Build one entry with size 0 */
> +        table = g_malloc0(CT3_CDAT_NUM_ENTRIES * sizeof(*table));
> +        ct3_build_cdat_entries_for_mr(&(table[0]), dsmad_handle++,
> +                                0, false, false, 0);
> +        *cdat_table = g_steal_pointer(&table);
> +        return CT3_CDAT_NUM_ENTRIES;
>      }
>  
>      if (ct3d->hostvmem) {
> @@ -712,8 +717,11 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
>  
>      if (!ct3d->hostmem && !ct3d->hostvmem && !ct3d->hostpmem
>          && !ct3d->dc.num_regions) {
> -        error_setg(errp, "at least one memdev property must be set");
> -        return false;
> +        /* no memdev property provided. Default to zero memory size device */
> +        ct3d->cxl_dstate.pmem_size = 0;
> +        ct3d->cxl_dstate.vmem_size = 0;
> +        ct3d->cxl_dstate.static_mem_size = 0;
> +        return true;
>      } else if (ct3d->hostmem && ct3d->hostpmem) {
>          error_setg(errp, "[memdev] cannot be used with new "
>                           "[persistent-memdev] property");