[PATCH 1/2] hw/cxl: set cxl-type3 device type to PCI_CLASS_MEMORY_CXL

[PATCH 1/2] hw/cxl: set cxl-type3 device type to PCI_CLASS_MEMORY_CXL

[Gregory Price]

Current code sets to STORAGE_EXPRESS and then overrides it.

Signed-off-by: Gregory Price <gregory.price@memverge.com>
---
 hw/mem/cxl_type3.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index ada2108fac..1837c1c83a 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -146,7 +146,6 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
     }
 
     pci_config_set_prog_interface(pci_conf, 0x10);
-    pci_config_set_class(pci_conf, PCI_CLASS_MEMORY_CXL);
 
     pcie_endpoint_cap_init(pci_dev, 0x80);
     cxl_cstate->dvsec_offset = 0x100;
@@ -335,7 +334,7 @@ static void ct3_class_init(ObjectClass *oc, void *data)
 
     pc->realize = ct3_realize;
     pc->exit = ct3_exit;
-    pc->class_id = PCI_CLASS_STORAGE_EXPRESS;
+    pc->class_id = PCI_CLASS_MEMORY_CXL;
     pc->vendor_id = PCI_VENDOR_ID_INTEL;
     pc->device_id = 0xd93; /* LVF for now */
     pc->revision = 1;
-- 
2.37.3

[PATCH 2/2] hw/cxl: Allow CXL type-3 devices to be persistent or volatile

[Gregory Price]

This commit enables setting one memory region for a type-3 device, but
that region may now be either a persistent region or volatile region.

Future work may enable setting both regions simultaneously, as this is
a possible configuration on a real type-3 device.  The scaffolding was
put in for this, but is left for further work.

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).

There is presently no interface with which to expose a MemoryRegion's
real backing type (HostMemoryBackendRam/File), otherwise we could have
used File to imply pmem (or inspected HostMemoryBackendFile->is_pmem) to
deterine whether the backing device supported pmem mode.  This should be
considered for future work, as it would make creating an array of
HostMemory devices to represent DIMMs on a Single-Logical-Device
MemoryExpander fairly straightforward.

Signed-off-by: Gregory Price <gregory.price@memverge.com>
---
 hw/cxl/cxl-mailbox-utils.c  | 22 ++++++++++--------
 hw/mem/cxl_type3.c          | 46 +++++++++++++++++++++++++++++++++----
 include/hw/cxl/cxl_device.h |  7 +++++-
 3 files changed, 59 insertions(+), 16 deletions(-)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index bc1bb18844..dfec11a1b5 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -138,7 +138,7 @@ 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 < (256 << 20)) {
+    if (cxl_dstate->mem_size < (256 << 20)) {
         return CXL_MBOX_INTERNAL_ERROR;
     }
 
@@ -281,9 +281,10 @@ 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, 256 << 20)) {
+    if ((!QEMU_IS_ALIGNED(cxl_dstate->mem_size, 256 << 20)) ||
+        (!QEMU_IS_ALIGNED(cxl_dstate->vmem_size, 256 << 20)) ||
+        (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, 256 << 20))) {
         return CXL_MBOX_INTERNAL_ERROR;
     }
 
@@ -293,8 +294,9 @@ static ret_code cmd_identify_memory_device(struct cxl_cmd *cmd,
     /* PMEM only */
     snprintf(id->fw_revision, 0x10, "BWFW VERSION %02d", 0);
 
-    id->total_capacity = size / (256 << 20);
-    id->persistent_capacity = size / (256 << 20);
+    id->total_capacity = cxl_dstate->mem_size / (256 << 20);
+    id->persistent_capacity = cxl_dstate->pmem_size / (256 << 20);
+    id->volatile_capacity = cxl_dstate->vmem_size / (256 << 20);
     id->lsa_size = cvc->get_lsa_size(ct3d);
 
     *len = sizeof(*id);
@@ -312,16 +314,16 @@ 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, 256 << 20)) {
+    if ((!QEMU_IS_ALIGNED(cxl_dstate->mem_size, 256 << 20)) ||
+        (!QEMU_IS_ALIGNED(cxl_dstate->vmem_size, 256 << 20)) ||
+        (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, 256 << 20))) {
         return CXL_MBOX_INTERNAL_ERROR;
     }
 
-    /* PMEM only */
-    part_info->active_vmem = 0;
+    part_info->active_vmem = cxl_dstate->vmem_size / (256 << 20);
     part_info->next_vmem = 0;
-    part_info->active_pmem = size / (256 << 20);
+    part_info->active_pmem = cxl_dstate->pmem_size / (256 << 20);
     part_info->next_pmem = 0;
 
     *len = sizeof(*part_info);
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 1837c1c83a..998461dac1 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -100,18 +100,47 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
     DeviceState *ds = DEVICE(ct3d);
     MemoryRegion *mr;
     char *name;
+    bool is_pmem = false;
 
-    if (!ct3d->hostmem) {
-        error_setg(errp, "memdev property must be set");
+    /*
+     * FIXME: For now we only allow a single host memory region.
+     * Handle the deprecated memdev property usage cases
+     */
+    if (!ct3d->hostmem && !ct3d->host_vmem && !ct3d->host_pmem) {
+        error_setg(errp, "at least one memdev property must be set");
         return false;
+    } else if (ct3d->hostmem && (ct3d->host_vmem || ct3d->host_pmem)) {
+        error_setg(errp, "deprecated [memdev] cannot be used with new "
+                         "persistent and volatile memdev properties");
+        return false;
+    } else if (ct3d->hostmem) {
+        warn_report("memdev is deprecated and defaults to pmem. "
+                    "Use (persistent|volatile)-memdev instead.");
+        is_pmem = true;
+    } else {
+        if (ct3d->host_vmem && ct3d->host_pmem) {
+            error_setg(errp, "Multiple memory devices not supported yet");
+            return false;
+        }
+        is_pmem = !!ct3d->host_pmem;
+        ct3d->hostmem = ct3d->host_pmem ? ct3d->host_pmem : ct3d->host_vmem;
     }
 
+    /*
+     * for now, since there is only one memdev, we can set the type
+     * based on whether this was a ram region or file region
+     */
     mr = host_memory_backend_get_memory(ct3d->hostmem);
     if (!mr) {
         error_setg(errp, "memdev property must be set");
         return false;
     }
-    memory_region_set_nonvolatile(mr, true);
+
+    /*
+     * FIXME: This code should eventually enumerate each memory region and
+     * report vmem and pmem capacity separate, but for now just set to one
+     */
+    memory_region_set_nonvolatile(mr, is_pmem);
     memory_region_set_enabled(mr, true);
     host_memory_backend_set_mapped(ct3d->hostmem, true);
 
@@ -123,7 +152,10 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
     address_space_init(&ct3d->hostmem_as, mr, name);
     g_free(name);
 
-    ct3d->cxl_dstate.pmem_size = ct3d->hostmem->size;
+    /* FIXME: When multiple regions are supported, this needs to aggregate */
+    ct3d->cxl_dstate.mem_size = ct3d->hostmem->size;
+    ct3d->cxl_dstate.vmem_size = is_pmem ? 0 : ct3d->hostmem->size;
+    ct3d->cxl_dstate.pmem_size = is_pmem ? ct3d->hostmem->size : 0;
 
     if (!ct3d->lsa) {
         error_setg(errp, "lsa property must be set");
@@ -272,6 +304,10 @@ static void ct3d_reset(DeviceState *dev)
 static Property ct3_props[] = {
     DEFINE_PROP_LINK("memdev", CXLType3Dev, hostmem, TYPE_MEMORY_BACKEND,
                      HostMemoryBackend *),
+    DEFINE_PROP_LINK("persistent-memdev", CXLType3Dev, host_pmem,
+                     TYPE_MEMORY_BACKEND, HostMemoryBackend *),
+    DEFINE_PROP_LINK("volatile-memdev", CXLType3Dev, host_vmem,
+                     TYPE_MEMORY_BACKEND, HostMemoryBackend *),
     DEFINE_PROP_LINK("lsa", CXLType3Dev, lsa, TYPE_MEMORY_BACKEND,
                      HostMemoryBackend *),
     DEFINE_PROP_END_OF_LIST(),
@@ -340,7 +376,7 @@ static void ct3_class_init(ObjectClass *oc, void *data)
     pc->revision = 1;
 
     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 1e141b6621..fd96a5ea4e 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -117,8 +117,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 */
@@ -235,7 +237,10 @@ struct CXLType3Dev {
     PCIDevice parent_obj;
 
     /* Properties */
+    /* TODO: remove hostmem when multi-dev is implemented */
     HostMemoryBackend *hostmem;
+    HostMemoryBackend *host_vmem;
+    HostMemoryBackend *host_pmem;
     HostMemoryBackend *lsa;
 
     /* State */
-- 
2.37.3

Re: [PATCH 1/2] hw/cxl: set cxl-type3 device type to PCI_CLASS_MEMORY_CXL

[Jonathan Cameron via]

On Thu,  6 Oct 2022 19:37:01 -0400
Gregory Price <gourry.memverge@gmail.com> wrote:

> Current code sets to STORAGE_EXPRESS and then overrides it.
> 
> Signed-off-by: Gregory Price <gregory.price@memverge.com>

I'm carry the same patch after you reported it the other day.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  hw/mem/cxl_type3.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index ada2108fac..1837c1c83a 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -146,7 +146,6 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>      }
>  
>      pci_config_set_prog_interface(pci_conf, 0x10);
> -    pci_config_set_class(pci_conf, PCI_CLASS_MEMORY_CXL);
>  
>      pcie_endpoint_cap_init(pci_dev, 0x80);
>      cxl_cstate->dvsec_offset = 0x100;
> @@ -335,7 +334,7 @@ static void ct3_class_init(ObjectClass *oc, void *data)
>  
>      pc->realize = ct3_realize;
>      pc->exit = ct3_exit;
> -    pc->class_id = PCI_CLASS_STORAGE_EXPRESS;
> +    pc->class_id = PCI_CLASS_MEMORY_CXL;
>      pc->vendor_id = PCI_VENDOR_ID_INTEL;
>      pc->device_id = 0xd93; /* LVF for now */
>      pc->revision = 1;

Re: [PATCH 1/2] hw/cxl: set cxl-type3 device type to PCI_CLASS_MEMORY_CXL

[Davidlohr Bueso]

On Thu, 06 Oct 2022, Gregory Price wrote:

>Current code sets to STORAGE_EXPRESS and then overrides it.

Good catch.

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

Re: [PATCH 1/2] hw/cxl: set cxl-type3 device type to PCI_CLASS_MEMORY_CXL

[Davidlohr Bueso]

On Thu, 06 Oct 2022, Gregory Price wrote:

>Current code sets to STORAGE_EXPRESS and then overrides it.

Good catch.

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

>
>Signed-off-by: Gregory Price <gregory.price@memverge.com>
>---
> hw/mem/cxl_type3.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
>diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
>index ada2108fac..1837c1c83a 100644
>--- a/hw/mem/cxl_type3.c
>+++ b/hw/mem/cxl_type3.c
>@@ -146,7 +146,6 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>     }
>
>     pci_config_set_prog_interface(pci_conf, 0x10);
>-    pci_config_set_class(pci_conf, PCI_CLASS_MEMORY_CXL);
>
>     pcie_endpoint_cap_init(pci_dev, 0x80);
>     cxl_cstate->dvsec_offset = 0x100;
>@@ -335,7 +334,7 @@ static void ct3_class_init(ObjectClass *oc, void *data)
>
>     pc->realize = ct3_realize;
>     pc->exit = ct3_exit;
>-    pc->class_id = PCI_CLASS_STORAGE_EXPRESS;
>+    pc->class_id = PCI_CLASS_MEMORY_CXL;
>     pc->vendor_id = PCI_VENDOR_ID_INTEL;
>     pc->device_id = 0xd93; /* LVF for now */
>     pc->revision = 1;
>-- 
>2.37.3
>

Re: [PATCH 2/2] hw/cxl: Allow CXL type-3 devices to be persistent or volatile

[Jonathan Cameron via]

On Thu,  6 Oct 2022 19:37:02 -0400
Gregory Price <gourry.memverge@gmail.com> wrote:

> This commit enables setting one memory region for a type-3 device, but
> that region may now be either a persistent region or volatile region.
> 
> Future work may enable setting both regions simultaneously, as this is
> a possible configuration on a real type-3 device.  The scaffolding was
> put in for this, but is left for further work.
> 
> 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).
> 
> There is presently no interface with which to expose a MemoryRegion's
> real backing type (HostMemoryBackendRam/File), otherwise we could have
> used File to imply pmem (or inspected HostMemoryBackendFile->is_pmem) to
> deterine whether the backing device supported pmem mode.  This should be
> considered for future work, as it would make creating an array of
> HostMemory devices to represent DIMMs on a Single-Logical-Device
> MemoryExpander fairly straightforward.
> 
> Signed-off-by: Gregory Price <gregory.price@memverge.com>
> ---
Looks good to me, though I haven't tested it yet.

A few trivial comments inline.

>      *len = sizeof(*part_info);
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 1837c1c83a..998461dac1 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -100,18 +100,47 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
>      DeviceState *ds = DEVICE(ct3d);
>      MemoryRegion *mr;
>      char *name;
> +    bool is_pmem = false;
>  
> -    if (!ct3d->hostmem) {
> -        error_setg(errp, "memdev property must be set");
> +    /*
> +     * FIXME: For now we only allow a single host memory region.

TODO maybe?  FIXME tends to lead to reviewers asking why we haven't
fixed it yet!

> +     * Handle the deprecated memdev property usage cases
> +     */
> +    if (!ct3d->hostmem && !ct3d->host_vmem && !ct3d->host_pmem) {
> +        error_setg(errp, "at least one memdev property must be set");
>          return false;
> +    } else if (ct3d->hostmem && (ct3d->host_vmem || ct3d->host_pmem)) {
> +        error_setg(errp, "deprecated [memdev] cannot be used with new "
> +                         "persistent and volatile memdev properties");
> +        return false;
> +    } else if (ct3d->hostmem) {
> +        warn_report("memdev is deprecated and defaults to pmem. "
> +                    "Use (persistent|volatile)-memdev instead.");

I'd suggest we don't warn on this yet. There is limited advantage in
doing so given it's easy to carry on supporting by just treating
it as another name for persistent-memdev


> +        is_pmem = true;
> +    } else {
> +        if (ct3d->host_vmem && ct3d->host_pmem) {
> +            error_setg(errp, "Multiple memory devices not supported yet");
> +            return false;
> +        }
> +        is_pmem = !!ct3d->host_pmem;
> +        ct3d->hostmem = ct3d->host_pmem ? ct3d->host_pmem : ct3d->host_vmem;
>      }
>  

Re: [PATCH 2/2] hw/cxl: Allow CXL type-3 devices to be persistent or volatile

[Davidlohr Bueso]

On Thu, 06 Oct 2022, Gregory Price wrote:

>diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
>index bc1bb18844..dfec11a1b5 100644
>--- a/hw/cxl/cxl-mailbox-utils.c
>+++ b/hw/cxl/cxl-mailbox-utils.c
>@@ -138,7 +138,7 @@ 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 < (256 << 20)) {
>+    if (cxl_dstate->mem_size < (256 << 20)) {

Nit but we probably want to abstract this out (in a pre-patch), just like in the
kernel side. Ie:

#define CXL_CAPACITY_MULTIPLIER   0x10000000 /* SZ_256M */

>         return CXL_MBOX_INTERNAL_ERROR;
>     }
>
>@@ -281,9 +281,10 @@ 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, 256 << 20)) {
>+    if ((!QEMU_IS_ALIGNED(cxl_dstate->mem_size, 256 << 20)) ||

is the full mem_size check here really needed?

>+        (!QEMU_IS_ALIGNED(cxl_dstate->vmem_size, 256 << 20)) ||
>+        (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, 256 << 20))) {
>         return CXL_MBOX_INTERNAL_ERROR;
>     }
>
>@@ -293,8 +294,9 @@ static ret_code cmd_identify_memory_device(struct cxl_cmd *cmd,
>     /* PMEM only */

This comment wants removed.

>     snprintf(id->fw_revision, 0x10, "BWFW VERSION %02d", 0);
>
>-    id->total_capacity = size / (256 << 20);
>-    id->persistent_capacity = size / (256 << 20);
>+    id->total_capacity = cxl_dstate->mem_size / (256 << 20);
>+    id->persistent_capacity = cxl_dstate->pmem_size / (256 << 20);
>+    id->volatile_capacity = cxl_dstate->vmem_size / (256 << 20);
>     id->lsa_size = cvc->get_lsa_size(ct3d);
>
>     *len = sizeof(*id);
>@@ -312,16 +314,16 @@ 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, 256 << 20)) {
>+    if ((!QEMU_IS_ALIGNED(cxl_dstate->mem_size, 256 << 20)) ||
>+        (!QEMU_IS_ALIGNED(cxl_dstate->vmem_size, 256 << 20)) ||
>+        (!QEMU_IS_ALIGNED(cxl_dstate->pmem_size, 256 << 20))) {
>         return CXL_MBOX_INTERNAL_ERROR;
>     }
>
>-    /* PMEM only */
>-    part_info->active_vmem = 0;
>+    part_info->active_vmem = cxl_dstate->vmem_size / (256 << 20);
>     part_info->next_vmem = 0;
>-    part_info->active_pmem = size / (256 << 20);
>+    part_info->active_pmem = cxl_dstate->pmem_size / (256 << 20);
>     part_info->next_pmem = 0;
>
>     *len = sizeof(*part_info);
>diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
>index 1837c1c83a..998461dac1 100644
>--- a/hw/mem/cxl_type3.c
>+++ b/hw/mem/cxl_type3.c
>@@ -100,18 +100,47 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
>     DeviceState *ds = DEVICE(ct3d);
>     MemoryRegion *mr;
>     char *name;
>+    bool is_pmem = false;
>
>-    if (!ct3d->hostmem) {
>-        error_setg(errp, "memdev property must be set");
>+    /*
>+     * FIXME: For now we only allow a single host memory region.
>+     * Handle the deprecated memdev property usage cases
>+     */
>+    if (!ct3d->hostmem && !ct3d->host_vmem && !ct3d->host_pmem) {
>+        error_setg(errp, "at least one memdev property must be set");
>         return false;
>+    } else if (ct3d->hostmem && (ct3d->host_vmem || ct3d->host_pmem)) {
>+        error_setg(errp, "deprecated [memdev] cannot be used with new "
>+                         "persistent and volatile memdev properties");
>+        return false;
>+    } else if (ct3d->hostmem) {
>+        warn_report("memdev is deprecated and defaults to pmem. "
>+                    "Use (persistent|volatile)-memdev instead.");
>+        is_pmem = true;
>+    } else {
>+        if (ct3d->host_vmem && ct3d->host_pmem) {
>+            error_setg(errp, "Multiple memory devices not supported yet");
>+            return false;
>+        }
>+        is_pmem = !!ct3d->host_pmem;
>+        ct3d->hostmem = ct3d->host_pmem ? ct3d->host_pmem : ct3d->host_vmem;

This hides requirement details as to the necessary changes that are needed for
volatile support - for example, build_dvsecs(). Imo using two backends (without
breaking current configs, of course) should be the initial version, not something
to leave pending.

Thanks,
Davidlohr

Re: [PATCH 2/2] hw/cxl: Allow CXL type-3 devices to be persistent or volatile

[Davidlohr Bueso]

On Mon, 10 Oct 2022, Davidlohr Bueso wrote:

>This hides requirement details as to the necessary changes that are needed for
>volatile support - for example, build_dvsecs(). Imo using two backends (without
>breaking current configs, of course) should be the initial version, not something
>to leave pending.

Minimally this is along the lines I was thinking of. I rebased some of my original
patches on top of yours. It builds and passes tests/qtest/cxl-test, but certainly
untested otherwise. The original code did show the volatile support as per cxl-list.

As such users can still use memdev which will map to the pmemdev. One thing which I
had not explored was the lsa + vmem thing, so the below prevents this for the time
being, fyi.

Thanks,
Davidlohr

----8<----------------------------------------------------

diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index e8341a818467..cd079dbddd9a 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -18,14 +18,21 @@ static void build_dvsecs(CXLType3Dev *ct3d)
  {
      CXLComponentState *cxl_cstate = &ct3d->cxl_cstate;
      uint8_t *dvsec;
+    uint64_t size = 0;
+
+    if (ct3d->hostvmem) {
+        size += ct3d->hostvmem->size;
+    }
+    if (ct3d->hostpmem) {
+        size += ct3d->hostpmem->size;
+    }

      dvsec = (uint8_t *)&(CXLDVSECDevice){
-        .cap = 0x1e,
+        .cap = 0x1e, /* one HDM range */
	 .ctrl = 0x2,
	 .status2 = 0x2,
-        .range1_size_hi = ct3d->hostmem->size >> 32,
-        .range1_size_lo = (2 << 5) | (2 << 2) | 0x3 |
-        (ct3d->hostmem->size & 0xF0000000),
+        .range1_size_hi = size >> 32,
+        .range1_size_lo = (2 << 5) | (2 << 2) | 0x3 | (size & 0xF0000000),
	 .range1_base_hi = 0,
	 .range1_base_lo = 0,
      };
@@ -98,70 +105,60 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
  static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
  {
      DeviceState *ds = DEVICE(ct3d);
-    MemoryRegion *mr;
      char *name;
-    bool is_pmem = false;

-    /*
-     * FIXME: For now we only allow a single host memory region.
-     * Handle the deprecated memdev property usage cases
-     */
-    if (!ct3d->hostmem && !ct3d->host_vmem && !ct3d->host_pmem) {
+    if (!ct3d->hostvmem && !ct3d->hostpmem) {
	 error_setg(errp, "at least one memdev property must be set");
	 return false;
-    } else if (ct3d->hostmem && (ct3d->host_vmem || ct3d->host_pmem)) {
-        error_setg(errp, "deprecated [memdev] cannot be used with new "
-                         "persistent and volatile memdev properties");
-        return false;
-    } else if (ct3d->hostmem) {
-        warn_report("memdev is deprecated and defaults to pmem. "
-                    "Use (persistent|volatile)-memdev instead.");
-        is_pmem = true;
-    } else {
-        if (ct3d->host_vmem && ct3d->host_pmem) {
-            error_setg(errp, "Multiple memory devices not supported yet");
-            return false;
-        }
-        is_pmem = !!ct3d->host_pmem;
-        ct3d->hostmem = ct3d->host_pmem ? ct3d->host_pmem : ct3d->host_vmem;
      }

-    /*
-     * for now, since there is only one memdev, we can set the type
-     * based on whether this was a ram region or file region
-     */
-    mr = host_memory_backend_get_memory(ct3d->hostmem);
-    if (!mr) {
-        error_setg(errp, "memdev property must be set");
+    /* TODO: volatile devices may have LSA */
+    if (ct3d->hostvmem && ct3d->lsa) {
+        error_setg(errp, "lsa property must be set");
	 return false;
      }

-    /*
-     * FIXME: This code should eventually enumerate each memory region and
-     * report vmem and pmem capacity separate, but for now just set to one
-     */
-    memory_region_set_nonvolatile(mr, is_pmem);
-    memory_region_set_enabled(mr, true);
-    host_memory_backend_set_mapped(ct3d->hostmem, true);
-
      if (ds->id) {
	 name = g_strdup_printf("cxl-type3-dpa-space:%s", ds->id);
      } else {
	 name = g_strdup("cxl-type3-dpa-space");
      }
-    address_space_init(&ct3d->hostmem_as, mr, name);
-    g_free(name);

-    /* FIXME: When multiple regions are supported, this needs to aggregate */
-    ct3d->cxl_dstate.mem_size = ct3d->hostmem->size;
-    ct3d->cxl_dstate.vmem_size = is_pmem ? 0 : ct3d->hostmem->size;
-    ct3d->cxl_dstate.pmem_size = is_pmem ? ct3d->hostmem->size : 0;
+    if (ct3d->hostvmem) {
+        MemoryRegion *vmr;

-    if (!ct3d->lsa) {
-        error_setg(errp, "lsa property must be set");
-        return false;
+        vmr = host_memory_backend_get_memory(ct3d->hostvmem);
+        if (!vmr) {
+            error_setg(errp, "volatile-memdev property must be set");
+            return false;
+        }
+
+        memory_region_set_nonvolatile(vmr, false);
+        memory_region_set_enabled(vmr, true);
+        host_memory_backend_set_mapped(ct3d->hostvmem, true);
+        address_space_init(&ct3d->hostvmem_as, vmr, name);
+        ct3d->cxl_dstate.vmem_size = ct3d->hostvmem->size;
+        ct3d->cxl_dstate.mem_size += ct3d->hostvmem->size;
      }

+    if (ct3d->hostpmem) {
+        MemoryRegion *pmr;
+
+        pmr = host_memory_backend_get_memory(ct3d->hostpmem);
+        if (!pmr) {
+            error_setg(errp, "legacy memdev or persistent-memdev property must be set");
+            return false;
+        }
+
+        memory_region_set_nonvolatile(pmr, true);
+        memory_region_set_enabled(pmr, true);
+        host_memory_backend_set_mapped(ct3d->hostpmem, true);
+        address_space_init(&ct3d->hostpmem_as, pmr, name);
+        ct3d->cxl_dstate.pmem_size = ct3d->hostpmem->size;
+        ct3d->cxl_dstate.mem_size += ct3d->hostpmem->size;
+    }
+    g_free(name);
+
      return true;
  }

@@ -210,7 +207,13 @@ static void ct3_exit(PCIDevice *pci_dev)
      ComponentRegisters *regs = &cxl_cstate->crb;

      g_free(regs->special_ops);
-    address_space_destroy(&ct3d->hostmem_as);
+
+    if (ct3d->hostvmem) {
+        address_space_destroy(&ct3d->hostvmem_as);
+    }
+    if (ct3d->hostpmem) {
+        address_space_destroy(&ct3d->hostpmem_as);
+    }
  }

  /* TODO: Support multiple HDM decoders and DPA skip */
@@ -249,47 +252,86 @@ MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
			    unsigned size, MemTxAttrs attrs)
  {
      CXLType3Dev *ct3d = CXL_TYPE3(d);
-    uint64_t dpa_offset;
-    MemoryRegion *mr;
+    uint64_t total_size = 0, dpa_offset;
+    MemoryRegion *vmr, *pmr;

-    /* TODO support volatile region */
-    mr = host_memory_backend_get_memory(ct3d->hostmem);
-    if (!mr) {
+    vmr = host_memory_backend_get_memory(ct3d->hostvmem);
+    pmr = host_memory_backend_get_memory(ct3d->hostpmem);
+    if (!vmr && !pmr) {
	 return MEMTX_ERROR;
      }

+    if (vmr) {
+        total_size += vmr->size;
+    }
+    if (pmr) {
+        total_size += pmr->size;
+    }
      if (!cxl_type3_dpa(ct3d, host_addr, &dpa_offset)) {
	 return MEMTX_ERROR;
      }
-
-    if (dpa_offset > int128_get64(mr->size)) {
+    if (dpa_offset > total_size) {
	 return MEMTX_ERROR;
      }

-    return address_space_read(&ct3d->hostmem_as, dpa_offset, attrs, data, size);
+    if (vmr) {
+        /* volatile starts at DPA 0 */
+        if (dpa_offset <= int128_get64(vmr->size)) {
+            return address_space_read(&ct3d->hostvmem_as,
+                                  dpa_offset, attrs, data, size);
+        }
+    }
+    if (pmr) {
+        if (dpa_offset > int128_get64(pmr->size)) {
+            return MEMTX_ERROR;
+        }
+        return address_space_read(&ct3d->hostpmem_as, dpa_offset,
+                                  attrs, data, size);
+    }
+
+    return MEMTX_ERROR;
  }

  MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
			     unsigned size, MemTxAttrs attrs)
  {
      CXLType3Dev *ct3d = CXL_TYPE3(d);
-    uint64_t dpa_offset;
-    MemoryRegion *mr;
+    uint64_t total_size = 0, dpa_offset;
+    MemoryRegion *vmr, *pmr;

-    mr = host_memory_backend_get_memory(ct3d->hostmem);
-    if (!mr) {
+    vmr = host_memory_backend_get_memory(ct3d->hostpmem);
+    pmr = host_memory_backend_get_memory(ct3d->hostpmem);
+    if (!vmr && !pmr) {
	 return MEMTX_OK;
      }
-
+    if (vmr) {
+        total_size += vmr->size;
+    }
+    if (pmr) {
+        total_size += pmr->size;
+    }
      if (!cxl_type3_dpa(ct3d, host_addr, &dpa_offset)) {
	 return MEMTX_OK;
      }
+    if (dpa_offset > total_size) {
+        return MEMTX_ERROR;
+    }

-    if (dpa_offset > int128_get64(mr->size)) {
-        return MEMTX_OK;
+    if (vmr) {
+        if (dpa_offset <= int128_get64(vmr->size)) {
+                return address_space_write(&ct3d->hostvmem_as,
+                                  dpa_offset, attrs, &data, size);
+        }
      }
-    return address_space_write(&ct3d->hostmem_as, dpa_offset, attrs,
+    if (pmr) {
+        if (dpa_offset > int128_get64(pmr->size)) {
+            return MEMTX_OK;
+        }
+        return address_space_write(&ct3d->hostpmem_as, dpa_offset, attrs,
				&data, size);
+    }
+
+    return MEMTX_ERROR;
  }

  static void ct3d_reset(DeviceState *dev)
@@ -303,11 +345,11 @@ static void ct3d_reset(DeviceState *dev)
  }

  static Property ct3_props[] = {
-    DEFINE_PROP_LINK("memdev", CXLType3Dev, hostmem, TYPE_MEMORY_BACKEND,
-                     HostMemoryBackend *),
-    DEFINE_PROP_LINK("persistent-memdev", CXLType3Dev, host_pmem,
+    DEFINE_PROP_LINK("memdev", CXLType3Dev, hostpmem, TYPE_MEMORY_BACKEND,
+                     HostMemoryBackend *), /* for backward-compatibility */
+    DEFINE_PROP_LINK("persistent-memdev", CXLType3Dev, hostpmem,
		      TYPE_MEMORY_BACKEND, HostMemoryBackend *),
-    DEFINE_PROP_LINK("volatile-memdev", CXLType3Dev, host_vmem,
+    DEFINE_PROP_LINK("volatile-memdev", CXLType3Dev, hostvmem,
		      TYPE_MEMORY_BACKEND, HostMemoryBackend *),
      DEFINE_PROP_LINK("lsa", CXLType3Dev, lsa, TYPE_MEMORY_BACKEND,
		      HostMemoryBackend *),
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index fd96a5ea4e47..c81f92ecf093 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -237,14 +237,13 @@ struct CXLType3Dev {
      PCIDevice parent_obj;

      /* Properties */
-    /* TODO: remove hostmem when multi-dev is implemented */
-    HostMemoryBackend *hostmem;
-    HostMemoryBackend *host_vmem;
-    HostMemoryBackend *host_pmem;
+    HostMemoryBackend *hostvmem;
+    HostMemoryBackend *hostpmem;
      HostMemoryBackend *lsa;

      /* State */
-    AddressSpace hostmem_as;
+    AddressSpace hostvmem_as;
+    AddressSpace hostpmem_as;
      CXLComponentState cxl_cstate;
      CXLDeviceState cxl_dstate;
  };

Re: [PATCH 2/2] hw/cxl: Allow CXL type-3 devices to be persistent or volatile

[Gregory Price]

Hang tight, I'm whipping up a multi-region patch that will support a
vmem and pmem region and such.  Finally got oriented enough to figure
out the DPA decoding a bit.  I will probably need some help validating
the decoder logic and the CDAT table logic.

I will integrate the suggestions below into that patch set.

Jonathan i'm building on top of your gitlab branch and will make a
branch available for review when done.

On Mon, Oct 10, 2022 at 12:36:54PM -0700, Davidlohr Bueso wrote:
> On Mon, 10 Oct 2022, Davidlohr Bueso wrote:
> 
> > This hides requirement details as to the necessary changes that are needed for
> > volatile support - for example, build_dvsecs(). Imo using two backends (without
> > breaking current configs, of course) should be the initial version, not something
> > to leave pending.
> 
> Minimally this is along the lines I was thinking of. I rebased some of my original
> patches on top of yours. It builds and passes tests/qtest/cxl-test, but certainly
> untested otherwise. The original code did show the volatile support as per cxl-list.
> 
> As such users can still use memdev which will map to the pmemdev. One thing which I
> had not explored was the lsa + vmem thing, so the below prevents this for the time
> being, fyi.
> 
> Thanks,
> Davidlohr
> 
> ----8<----------------------------------------------------
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index e8341a818467..cd079dbddd9a 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -18,14 +18,21 @@ static void build_dvsecs(CXLType3Dev *ct3d)
>  {
>      CXLComponentState *cxl_cstate = &ct3d->cxl_cstate;
>      uint8_t *dvsec;
> +    uint64_t size = 0;
> +
> +    if (ct3d->hostvmem) {
> +        size += ct3d->hostvmem->size;
> +    }
> +    if (ct3d->hostpmem) {
> +        size += ct3d->hostpmem->size;
> +    }
> 
>      dvsec = (uint8_t *)&(CXLDVSECDevice){
> -        .cap = 0x1e,
> +        .cap = 0x1e, /* one HDM range */
> 	 .ctrl = 0x2,
> 	 .status2 = 0x2,
> -        .range1_size_hi = ct3d->hostmem->size >> 32,
> -        .range1_size_lo = (2 << 5) | (2 << 2) | 0x3 |
> -        (ct3d->hostmem->size & 0xF0000000),
> +        .range1_size_hi = size >> 32,
> +        .range1_size_lo = (2 << 5) | (2 << 2) | 0x3 | (size & 0xF0000000),
> 	 .range1_base_hi = 0,
> 	 .range1_base_lo = 0,
>      };
> @@ -98,70 +105,60 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
>  static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
>  {
>      DeviceState *ds = DEVICE(ct3d);
> -    MemoryRegion *mr;
>      char *name;
> -    bool is_pmem = false;
> 
> -    /*
> -     * FIXME: For now we only allow a single host memory region.
> -     * Handle the deprecated memdev property usage cases
> -     */
> -    if (!ct3d->hostmem && !ct3d->host_vmem && !ct3d->host_pmem) {
> +    if (!ct3d->hostvmem && !ct3d->hostpmem) {
> 	 error_setg(errp, "at least one memdev property must be set");
> 	 return false;
> -    } else if (ct3d->hostmem && (ct3d->host_vmem || ct3d->host_pmem)) {
> -        error_setg(errp, "deprecated [memdev] cannot be used with new "
> -                         "persistent and volatile memdev properties");
> -        return false;
> -    } else if (ct3d->hostmem) {
> -        warn_report("memdev is deprecated and defaults to pmem. "
> -                    "Use (persistent|volatile)-memdev instead.");
> -        is_pmem = true;
> -    } else {
> -        if (ct3d->host_vmem && ct3d->host_pmem) {
> -            error_setg(errp, "Multiple memory devices not supported yet");
> -            return false;
> -        }
> -        is_pmem = !!ct3d->host_pmem;
> -        ct3d->hostmem = ct3d->host_pmem ? ct3d->host_pmem : ct3d->host_vmem;
>      }
> 
> -    /*
> -     * for now, since there is only one memdev, we can set the type
> -     * based on whether this was a ram region or file region
> -     */
> -    mr = host_memory_backend_get_memory(ct3d->hostmem);
> -    if (!mr) {
> -        error_setg(errp, "memdev property must be set");
> +    /* TODO: volatile devices may have LSA */
> +    if (ct3d->hostvmem && ct3d->lsa) {
> +        error_setg(errp, "lsa property must be set");
> 	 return false;
>      }
> 
> -    /*
> -     * FIXME: This code should eventually enumerate each memory region and
> -     * report vmem and pmem capacity separate, but for now just set to one
> -     */
> -    memory_region_set_nonvolatile(mr, is_pmem);
> -    memory_region_set_enabled(mr, true);
> -    host_memory_backend_set_mapped(ct3d->hostmem, true);
> -
>      if (ds->id) {
> 	 name = g_strdup_printf("cxl-type3-dpa-space:%s", ds->id);
>      } else {
> 	 name = g_strdup("cxl-type3-dpa-space");
>      }
> -    address_space_init(&ct3d->hostmem_as, mr, name);
> -    g_free(name);
> 
> -    /* FIXME: When multiple regions are supported, this needs to aggregate */
> -    ct3d->cxl_dstate.mem_size = ct3d->hostmem->size;
> -    ct3d->cxl_dstate.vmem_size = is_pmem ? 0 : ct3d->hostmem->size;
> -    ct3d->cxl_dstate.pmem_size = is_pmem ? ct3d->hostmem->size : 0;
> +    if (ct3d->hostvmem) {
> +        MemoryRegion *vmr;
> 
> -    if (!ct3d->lsa) {
> -        error_setg(errp, "lsa property must be set");
> -        return false;
> +        vmr = host_memory_backend_get_memory(ct3d->hostvmem);
> +        if (!vmr) {
> +            error_setg(errp, "volatile-memdev property must be set");
> +            return false;
> +        }
> +
> +        memory_region_set_nonvolatile(vmr, false);
> +        memory_region_set_enabled(vmr, true);
> +        host_memory_backend_set_mapped(ct3d->hostvmem, true);
> +        address_space_init(&ct3d->hostvmem_as, vmr, name);
> +        ct3d->cxl_dstate.vmem_size = ct3d->hostvmem->size;
> +        ct3d->cxl_dstate.mem_size += ct3d->hostvmem->size;
>      }
> 
> +    if (ct3d->hostpmem) {
> +        MemoryRegion *pmr;
> +
> +        pmr = host_memory_backend_get_memory(ct3d->hostpmem);
> +        if (!pmr) {
> +            error_setg(errp, "legacy memdev or persistent-memdev property must be set");
> +            return false;
> +        }
> +
> +        memory_region_set_nonvolatile(pmr, true);
> +        memory_region_set_enabled(pmr, true);
> +        host_memory_backend_set_mapped(ct3d->hostpmem, true);
> +        address_space_init(&ct3d->hostpmem_as, pmr, name);
> +        ct3d->cxl_dstate.pmem_size = ct3d->hostpmem->size;
> +        ct3d->cxl_dstate.mem_size += ct3d->hostpmem->size;
> +    }
> +    g_free(name);
> +
>      return true;
>  }
> 
> @@ -210,7 +207,13 @@ static void ct3_exit(PCIDevice *pci_dev)
>      ComponentRegisters *regs = &cxl_cstate->crb;
> 
>      g_free(regs->special_ops);
> -    address_space_destroy(&ct3d->hostmem_as);
> +
> +    if (ct3d->hostvmem) {
> +        address_space_destroy(&ct3d->hostvmem_as);
> +    }
> +    if (ct3d->hostpmem) {
> +        address_space_destroy(&ct3d->hostpmem_as);
> +    }
>  }
> 
>  /* TODO: Support multiple HDM decoders and DPA skip */
> @@ -249,47 +252,86 @@ MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
> 			    unsigned size, MemTxAttrs attrs)
>  {
>      CXLType3Dev *ct3d = CXL_TYPE3(d);
> -    uint64_t dpa_offset;
> -    MemoryRegion *mr;
> +    uint64_t total_size = 0, dpa_offset;
> +    MemoryRegion *vmr, *pmr;
> 
> -    /* TODO support volatile region */
> -    mr = host_memory_backend_get_memory(ct3d->hostmem);
> -    if (!mr) {
> +    vmr = host_memory_backend_get_memory(ct3d->hostvmem);
> +    pmr = host_memory_backend_get_memory(ct3d->hostpmem);
> +    if (!vmr && !pmr) {
> 	 return MEMTX_ERROR;
>      }
> 
> +    if (vmr) {
> +        total_size += vmr->size;
> +    }
> +    if (pmr) {
> +        total_size += pmr->size;
> +    }
>      if (!cxl_type3_dpa(ct3d, host_addr, &dpa_offset)) {
> 	 return MEMTX_ERROR;
>      }
> -
> -    if (dpa_offset > int128_get64(mr->size)) {
> +    if (dpa_offset > total_size) {
> 	 return MEMTX_ERROR;
>      }
> 
> -    return address_space_read(&ct3d->hostmem_as, dpa_offset, attrs, data, size);
> +    if (vmr) {
> +        /* volatile starts at DPA 0 */
> +        if (dpa_offset <= int128_get64(vmr->size)) {
> +            return address_space_read(&ct3d->hostvmem_as,
> +                                  dpa_offset, attrs, data, size);
> +        }
> +    }
> +    if (pmr) {
> +        if (dpa_offset > int128_get64(pmr->size)) {
> +            return MEMTX_ERROR;
> +        }
> +        return address_space_read(&ct3d->hostpmem_as, dpa_offset,
> +                                  attrs, data, size);
> +    }
> +
> +    return MEMTX_ERROR;
>  }
> 
>  MemTxResult cxl_type3_write(PCIDevice *d, hwaddr host_addr, uint64_t data,
> 			     unsigned size, MemTxAttrs attrs)
>  {
>      CXLType3Dev *ct3d = CXL_TYPE3(d);
> -    uint64_t dpa_offset;
> -    MemoryRegion *mr;
> +    uint64_t total_size = 0, dpa_offset;
> +    MemoryRegion *vmr, *pmr;
> 
> -    mr = host_memory_backend_get_memory(ct3d->hostmem);
> -    if (!mr) {
> +    vmr = host_memory_backend_get_memory(ct3d->hostpmem);
> +    pmr = host_memory_backend_get_memory(ct3d->hostpmem);
> +    if (!vmr && !pmr) {
> 	 return MEMTX_OK;
>      }
> -
> +    if (vmr) {
> +        total_size += vmr->size;
> +    }
> +    if (pmr) {
> +        total_size += pmr->size;
> +    }
>      if (!cxl_type3_dpa(ct3d, host_addr, &dpa_offset)) {
> 	 return MEMTX_OK;
>      }
> +    if (dpa_offset > total_size) {
> +        return MEMTX_ERROR;
> +    }
> 
> -    if (dpa_offset > int128_get64(mr->size)) {
> -        return MEMTX_OK;
> +    if (vmr) {
> +        if (dpa_offset <= int128_get64(vmr->size)) {
> +                return address_space_write(&ct3d->hostvmem_as,
> +                                  dpa_offset, attrs, &data, size);
> +        }
>      }
> -    return address_space_write(&ct3d->hostmem_as, dpa_offset, attrs,
> +    if (pmr) {
> +        if (dpa_offset > int128_get64(pmr->size)) {
> +            return MEMTX_OK;
> +        }
> +        return address_space_write(&ct3d->hostpmem_as, dpa_offset, attrs,
> 				&data, size);
> +    }
> +
> +    return MEMTX_ERROR;
>  }
> 
>  static void ct3d_reset(DeviceState *dev)
> @@ -303,11 +345,11 @@ static void ct3d_reset(DeviceState *dev)
>  }
> 
>  static Property ct3_props[] = {
> -    DEFINE_PROP_LINK("memdev", CXLType3Dev, hostmem, TYPE_MEMORY_BACKEND,
> -                     HostMemoryBackend *),
> -    DEFINE_PROP_LINK("persistent-memdev", CXLType3Dev, host_pmem,
> +    DEFINE_PROP_LINK("memdev", CXLType3Dev, hostpmem, TYPE_MEMORY_BACKEND,
> +                     HostMemoryBackend *), /* for backward-compatibility */
> +    DEFINE_PROP_LINK("persistent-memdev", CXLType3Dev, hostpmem,
> 		      TYPE_MEMORY_BACKEND, HostMemoryBackend *),
> -    DEFINE_PROP_LINK("volatile-memdev", CXLType3Dev, host_vmem,
> +    DEFINE_PROP_LINK("volatile-memdev", CXLType3Dev, hostvmem,
> 		      TYPE_MEMORY_BACKEND, HostMemoryBackend *),
>      DEFINE_PROP_LINK("lsa", CXLType3Dev, lsa, TYPE_MEMORY_BACKEND,
> 		      HostMemoryBackend *),
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index fd96a5ea4e47..c81f92ecf093 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -237,14 +237,13 @@ struct CXLType3Dev {
>      PCIDevice parent_obj;
> 
>      /* Properties */
> -    /* TODO: remove hostmem when multi-dev is implemented */
> -    HostMemoryBackend *hostmem;
> -    HostMemoryBackend *host_vmem;
> -    HostMemoryBackend *host_pmem;
> +    HostMemoryBackend *hostvmem;
> +    HostMemoryBackend *hostpmem;
>      HostMemoryBackend *lsa;
> 
>      /* State */
> -    AddressSpace hostmem_as;
> +    AddressSpace hostvmem_as;
> +    AddressSpace hostpmem_as;
>      CXLComponentState cxl_cstate;
>      CXLDeviceState cxl_dstate;
>  };

Re: [PATCH 1/2] hw/cxl: set cxl-type3 device type to PCI_CLASS_MEMORY_CXL

[Michael S. Tsirkin]

On Thu, Oct 06, 2022 at 07:37:01PM -0400, Gregory Price wrote:
> Current code sets to STORAGE_EXPRESS and then overrides it.
> 
> Signed-off-by: Gregory Price <gregory.price@memverge.com>

If you expect me to merge it you need to CC me.
Also, do we need this separately from the series?

> ---
>  hw/mem/cxl_type3.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index ada2108fac..1837c1c83a 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -146,7 +146,6 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>      }
>  
>      pci_config_set_prog_interface(pci_conf, 0x10);
> -    pci_config_set_class(pci_conf, PCI_CLASS_MEMORY_CXL);
>  
>      pcie_endpoint_cap_init(pci_dev, 0x80);
>      cxl_cstate->dvsec_offset = 0x100;
> @@ -335,7 +334,7 @@ static void ct3_class_init(ObjectClass *oc, void *data)
>  
>      pc->realize = ct3_realize;
>      pc->exit = ct3_exit;
> -    pc->class_id = PCI_CLASS_STORAGE_EXPRESS;
> +    pc->class_id = PCI_CLASS_MEMORY_CXL;
>      pc->vendor_id = PCI_VENDOR_ID_INTEL;
>      pc->device_id = 0xd93; /* LVF for now */
>      pc->revision = 1;
> -- 
> 2.37.3
> 
> 

RE: [PATCH 1/2] hw/cxl: set cxl-type3 device type to PCI_CLASS_MEMORY_CXL

[Gregory Price]

I believe this was dropped from my line because Jonathan carried a similar commit on his branch.

Happy to push it up again as a separate commit if that is what you want.

Noted for the future on upstreams

-----Original Message-----
From: Michael S. Tsirkin <mst@redhat.com> 
Sent: Wednesday, October 26, 2022 4:06 PM
To: Gregory Price <gourry.memverge@gmail.com>
Cc: qemu-devel@nongnu.org; jonathan.cameron@huawei.com; linux-cxl@vger.kernel.org; alison.schofield@intel.com; dave@stgolabs.net; a.manzanares@samsung.com; bwidawsk@kernel.org; Gregory Price <gregory.price@memverge.com>
Subject: Re: [PATCH 1/2] hw/cxl: set cxl-type3 device type to PCI_CLASS_MEMORY_CXL

On Thu, Oct 06, 2022 at 07:37:01PM -0400, Gregory Price wrote:
> Current code sets to STORAGE_EXPRESS and then overrides it.
> 
> Signed-off-by: Gregory Price <gregory.price@memverge.com>

If you expect me to merge it you need to CC me.
Also, do we need this separately from the series?

> ---
>  hw/mem/cxl_type3.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c index 
> ada2108fac..1837c1c83a 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -146,7 +146,6 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>      }
>  
>      pci_config_set_prog_interface(pci_conf, 0x10);
> -    pci_config_set_class(pci_conf, PCI_CLASS_MEMORY_CXL);
>  
>      pcie_endpoint_cap_init(pci_dev, 0x80);
>      cxl_cstate->dvsec_offset = 0x100; @@ -335,7 +334,7 @@ static void 
> ct3_class_init(ObjectClass *oc, void *data)
>  
>      pc->realize = ct3_realize;
>      pc->exit = ct3_exit;
> -    pc->class_id = PCI_CLASS_STORAGE_EXPRESS;
> +    pc->class_id = PCI_CLASS_MEMORY_CXL;
>      pc->vendor_id = PCI_VENDOR_ID_INTEL;
>      pc->device_id = 0xd93; /* LVF for now */
>      pc->revision = 1;
> --
> 2.37.3
> 
> 

Re: [PATCH 1/2] hw/cxl: set cxl-type3 device type to PCI_CLASS_MEMORY_CXL

[Michael S. Tsirkin]

He does but in the end he sends patches not pull requests.
I don't care really as long as someone will send it up.


On Wed, Oct 26, 2022 at 08:09:45PM +0000, Gregory Price wrote:
> I believe this was dropped from my line because Jonathan carried a similar commit on his branch.
> 
> Happy to push it up again as a separate commit if that is what you want.
> 
> Noted for the future on upstreams
> 
> -----Original Message-----
> From: Michael S. Tsirkin <mst@redhat.com> 
> Sent: Wednesday, October 26, 2022 4:06 PM
> To: Gregory Price <gourry.memverge@gmail.com>
> Cc: qemu-devel@nongnu.org; jonathan.cameron@huawei.com; linux-cxl@vger.kernel.org; alison.schofield@intel.com; dave@stgolabs.net; a.manzanares@samsung.com; bwidawsk@kernel.org; Gregory Price <gregory.price@memverge.com>
> Subject: Re: [PATCH 1/2] hw/cxl: set cxl-type3 device type to PCI_CLASS_MEMORY_CXL
> 
> On Thu, Oct 06, 2022 at 07:37:01PM -0400, Gregory Price wrote:
> > Current code sets to STORAGE_EXPRESS and then overrides it.
> > 
> > Signed-off-by: Gregory Price <gregory.price@memverge.com>
> 
> If you expect me to merge it you need to CC me.
> Also, do we need this separately from the series?
> 
> > ---
> >  hw/mem/cxl_type3.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c index 
> > ada2108fac..1837c1c83a 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -146,7 +146,6 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
> >      }
> >  
> >      pci_config_set_prog_interface(pci_conf, 0x10);
> > -    pci_config_set_class(pci_conf, PCI_CLASS_MEMORY_CXL);
> >  
> >      pcie_endpoint_cap_init(pci_dev, 0x80);
> >      cxl_cstate->dvsec_offset = 0x100; @@ -335,7 +334,7 @@ static void 
> > ct3_class_init(ObjectClass *oc, void *data)
> >  
> >      pc->realize = ct3_realize;
> >      pc->exit = ct3_exit;
> > -    pc->class_id = PCI_CLASS_STORAGE_EXPRESS;
> > +    pc->class_id = PCI_CLASS_MEMORY_CXL;
> >      pc->vendor_id = PCI_VENDOR_ID_INTEL;
> >      pc->device_id = 0xd93; /* LVF for now */
> >      pc->revision = 1;
> > --
> > 2.37.3
> > 
> > 

Re: [PATCH 1/2] hw/cxl: set cxl-type3 device type to PCI_CLASS_MEMORY_CXL

[Gregory Price]

On Wed, Oct 26, 2022 at 04:11:29PM -0400, Michael S. Tsirkin wrote:
> He does but in the end he sends patches not pull requests.
> I don't care really as long as someone will send it up.
> 

Jonathan will submit this, it's not a critical issue so it can wait for
the larger feature set.

Re: [PATCH 1/2] hw/cxl: set cxl-type3 device type to PCI_CLASS_MEMORY_CXL

[Jonathan Cameron via]

On Wed, 26 Oct 2022 17:07:53 -0400
Gregory Price <gregory.price@memverge.com> wrote:

> On Wed, Oct 26, 2022 at 04:11:29PM -0400, Michael S. Tsirkin wrote:
> > He does but in the end he sends patches not pull requests.
> > I don't care really as long as someone will send it up.
> >   
> 
> Jonathan will submit this, it's not a critical issue so it can wait for
> the larger feature set.

Sure, I'll roll it with a few other similarly minor fixes in a few days as
a patch set.

Jonathan