[PATCH qemu v2 0/3] hw/cxl: Misc minor improvements

[PATCH qemu v2 0/3] hw/cxl: Misc minor improvements

[Jonathan Cameron]

v2: Switch patch 1 from a patch that removes the cfmws_list element
    of CXLState to a fix that makes use of it to fix a crash.
    Now based on master as precursors merged.

Before pushing on to more significant features a few unrelated patches
doing tidying up + one to avoid people setting the memory backend both
for a CXL type 3 device and as normal RAM in an attempt to get SRAT to
cover it correctly.  We've had multiple 'bug' reports from this and if
nothing else I'd like to stop getting those!

Fan Ni (1):
  hw/cxl/cxl-mailbox-utils: remove unneeded mailbox output payload space
    zeroing

Jonathan Cameron (1):
  hw/cxl: Check for multiple mappings of memory backends.

Zhao Liu (1):
  hw/cxl/cxl-host: Fix segmentation fault when getting cxl-fmw property

 hw/cxl/cxl-host.c          |  3 ++-
 hw/cxl/cxl-mailbox-utils.c |  7 -------
 hw/mem/cxl_type3.c         | 15 +++++++++++++++
 3 files changed, 17 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] hw/cxl/cxl-host: Fix segmentation fault when getting cxl-fmw property

[Jonathan Cameron]

From: Zhao Liu <zhao1.liu@intel.com>

QEMU crashes (Segmentation fault) when getting cxl-fmw property via
qmp:

(QEMU) qom-get path=machine property=cxl-fmw

This issue is caused by accessing wrong callback (opaque) type in
machine_get_cfmw().

cxl_machine_init() sets the callback as `CXLState *` type but
machine_get_cfmw() treats the callback as
`CXLFixedMemoryWindowOptionsList **`.

Fix this error by casting opaque to `CXLState *` type in
machine_get_cfmw().

Fixes: 03b39fcf64bc ("hw/cxl: Make the CXL fixed memory window setup a machine parameter.")
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Li Zhijian <lizhijian@fujitsu.com>
Reviewed-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
Link: https://lore.kernel.org/r/20240704093404.1848132-1-zhao1.liu@linux.intel.com
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/cxl/cxl-host.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
index c5f5fcfd64..e9f2543c43 100644
--- a/hw/cxl/cxl-host.c
+++ b/hw/cxl/cxl-host.c
@@ -315,7 +315,8 @@ static void machine_set_cxl(Object *obj, Visitor *v, const char *name,
 static void machine_get_cfmw(Object *obj, Visitor *v, const char *name,
                              void *opaque, Error **errp)
 {
-    CXLFixedMemoryWindowOptionsList **list = opaque;
+    CXLState *state = opaque;
+    CXLFixedMemoryWindowOptionsList **list = &state->cfmw_list;
 
     visit_type_CXLFixedMemoryWindowOptionsList(v, name, list, errp);
 }
-- 
2.43.0

Re: [PATCH v2 1/3] hw/cxl/cxl-host: Fix segmentation fault when getting cxl-fmw property

[Michael Tokarev]

05.07.2024 14:39, Jonathan Cameron via wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> QEMU crashes (Segmentation fault) when getting cxl-fmw property via
> qmp:
> 
> (QEMU) qom-get path=machine property=cxl-fmw
> 
> This issue is caused by accessing wrong callback (opaque) type in
> machine_get_cfmw().
> 
> cxl_machine_init() sets the callback as `CXLState *` type but
> machine_get_cfmw() treats the callback as
> `CXLFixedMemoryWindowOptionsList **`.
> 
> Fix this error by casting opaque to `CXLState *` type in
> machine_get_cfmw().
> 
> Fixes: 03b39fcf64bc ("hw/cxl: Make the CXL fixed memory window setup a machine parameter.")
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Reviewed-by: Li Zhijian <lizhijian@fujitsu.com>
> Reviewed-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
> Link: https://lore.kernel.org/r/20240704093404.1848132-1-zhao1.liu@linux.intel.com
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Is this a stable@ material?

Thanks,

/mjt

Re: [PATCH v2 1/3] hw/cxl/cxl-host: Fix segmentation fault when getting cxl-fmw property

[Jonathan Cameron]

On Wed, 24 Jul 2024 07:53:48 +0300
Michael Tokarev <mjt@tls.msk.ru> wrote:

> 05.07.2024 14:39, Jonathan Cameron via wrote:
> > From: Zhao Liu <zhao1.liu@intel.com>
> > 
> > QEMU crashes (Segmentation fault) when getting cxl-fmw property via
> > qmp:
> > 
> > (QEMU) qom-get path=machine property=cxl-fmw
> > 
> > This issue is caused by accessing wrong callback (opaque) type in
> > machine_get_cfmw().
> > 
> > cxl_machine_init() sets the callback as `CXLState *` type but
> > machine_get_cfmw() treats the callback as
> > `CXLFixedMemoryWindowOptionsList **`.
> > 
> > Fix this error by casting opaque to `CXLState *` type in
> > machine_get_cfmw().
> > 
> > Fixes: 03b39fcf64bc ("hw/cxl: Make the CXL fixed memory window setup a machine parameter.")
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > Reviewed-by: Li Zhijian <lizhijian@fujitsu.com>
> > Reviewed-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
> > Link: https://lore.kernel.org/r/20240704093404.1848132-1-zhao1.liu@linux.intel.com
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> Is this a stable@ material?

Yes please

Thanks,

Jonathan

> 
> Thanks,
> 
> /mjt
> 

[PATCH v2 2/3] hw/cxl/cxl-mailbox-utils: remove unneeded mailbox output payload space zeroing

[Jonathan Cameron]

From: Fan Ni <fan.ni@samsung.com>

The whole mailbox output payload space is already zeroed after copying
out the input payload, which happens before processing the specific mailbox
command:
https://elixir.bootlin.com/qemu/v8.2.1/source/hw/cxl/cxl-device-utils.c#L204

Signed-off-by: Fan Ni <fan.ni@samsung.com>
Link: https://lore.kernel.org/r/20240221221824.1092966-1-nifan.cxl@gmail.com
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/cxl/cxl-mailbox-utils.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 74eeb6fde7..facec42dc8 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -235,7 +235,6 @@ static CXLRetCode cmd_events_get_records(const struct cxl_cmd *cmd,
     log_type = payload_in[0];
 
     pl = (CXLGetEventPayload *)payload_out;
-    memset(pl, 0, sizeof(*pl));
 
     max_recs = (cxlds->payload_size - CXL_EVENT_PAYLOAD_HDR_SIZE) /
                 CXL_EVENT_RECORD_SIZE;
@@ -273,7 +272,6 @@ static CXLRetCode cmd_events_get_interrupt_policy(const struct cxl_cmd *cmd,
     CXLEventLog *log;
 
     policy = (CXLEventInterruptPolicy *)payload_out;
-    memset(policy, 0, sizeof(*policy));
 
     log = &cxlds->event_logs[CXL_EVENT_TYPE_INFO];
     if (log->irq_enabled) {
@@ -372,7 +370,6 @@ static CXLRetCode cmd_infostat_identify(const struct cxl_cmd *cmd,
     QEMU_BUILD_BUG_ON(sizeof(*is_identify) != 18);
 
     is_identify = (void *)payload_out;
-    memset(is_identify, 0, sizeof(*is_identify));
     is_identify->pcie_vid = class->vendor_id;
     is_identify->pcie_did = class->device_id;
     if (object_dynamic_cast(OBJECT(cci->d), TYPE_CXL_USP)) {
@@ -606,7 +603,6 @@ static CXLRetCode cmd_infostat_bg_op_sts(const struct cxl_cmd *cmd,
     QEMU_BUILD_BUG_ON(sizeof(*bg_op_status) != 8);
 
     bg_op_status = (void *)payload_out;
-    memset(bg_op_status, 0, sizeof(*bg_op_status));
     bg_op_status->status = cci->bg.complete_pct << 1;
     if (cci->bg.runtime > 0) {
         bg_op_status->status |= 1U << 0;
@@ -647,7 +643,6 @@ static CXLRetCode cmd_firmware_update_get_info(const struct cxl_cmd *cmd,
     }
 
     fw_info = (void *)payload_out;
-    memset(fw_info, 0, sizeof(*fw_info));
 
     fw_info->slots_supported = 2;
     fw_info->slot_info = BIT(0) | BIT(3);
@@ -805,7 +800,6 @@ static CXLRetCode cmd_identify_memory_device(const struct cxl_cmd *cmd,
     }
 
     id = (void *)payload_out;
-    memset(id, 0, sizeof(*id));
 
     snprintf(id->fw_revision, 0x10, "BWFW VERSION %02d", 0);
 
@@ -1095,7 +1089,6 @@ static CXLRetCode cmd_media_get_poison_list(const struct cxl_cmd *cmd,
     out_pl_len = sizeof(*out) + record_count * sizeof(out->records[0]);
     assert(out_pl_len <= CXL_MAILBOX_MAX_PAYLOAD_SIZE);
 
-    memset(out, 0, out_pl_len);
     QLIST_FOREACH(ent, poison_list, node) {
         uint64_t start, stop;
 
-- 
2.43.0

[PATCH v2 3/3] hw/cxl: Check for multiple mappings of memory backends.

[Jonathan Cameron]

Similar protection to that provided for -numa memdev=x
to make sure that memory used to back a type3 device is not also mapped
as normal RAM, or for multiple type3 devices.

This is an easy footgun to remove and seems multiple people have
run into it.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/mem/cxl_type3.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 35ac59883a..e7fbbb4d51 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -737,6 +737,11 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
             error_setg(errp, "volatile memdev must have backing device");
             return false;
         }
+        if (host_memory_backend_is_mapped(ct3d->hostvmem)) {
+            error_setg(errp, "memory backend %s can't be used multiple times.",
+               object_get_canonical_path_component(OBJECT(ct3d->hostvmem)));
+            return false;
+        }
         memory_region_set_nonvolatile(vmr, false);
         memory_region_set_enabled(vmr, true);
         host_memory_backend_set_mapped(ct3d->hostvmem, true);
@@ -760,6 +765,11 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
             error_setg(errp, "persistent memdev must have backing device");
             return false;
         }
+        if (host_memory_backend_is_mapped(ct3d->hostpmem)) {
+            error_setg(errp, "memory backend %s can't be used multiple times.",
+               object_get_canonical_path_component(OBJECT(ct3d->hostpmem)));
+            return false;
+        }
         memory_region_set_nonvolatile(pmr, true);
         memory_region_set_enabled(pmr, true);
         host_memory_backend_set_mapped(ct3d->hostpmem, true);
@@ -790,6 +800,11 @@ static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
             return false;
         }
 
+        if (host_memory_backend_is_mapped(ct3d->dc.host_dc)) {
+            error_setg(errp, "memory backend %s can't be used multiple times.",
+               object_get_canonical_path_component(OBJECT(ct3d->dc.host_dc)));
+            return false;
+        }
         /*
          * Set DC regions as volatile for now, non-volatile support can
          * be added in the future if needed.
-- 
2.43.0