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

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

[Jonathan Cameron via]

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!

Based on master + the two DCD series Michael Tsirkin has queued.

Based-on: [PATCH v8 00/14] Enabling DCD emulation support in Qemu
Based-on: https://lore.kernel.org/qemu-devel/20240523174651.1089554-1-nifan.cxl@gmail.com/
Based-on: [PATCH qemu 0/2] hw/cxl: DCD tweaks and improvements.
Based-on: https://lore.kernel.org/qemu-devel/20240625170805.359278-1-Jonathan.Cameron@huawei.com/

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.

Li Zhijian (1):
  hw/cxl: Get rid of unused cfmw_list

 include/hw/cxl/cxl.h       |  1 -
 hw/cxl/cxl-host.c          |  1 -
 hw/cxl/cxl-mailbox-utils.c |  7 -------
 hw/mem/cxl_type3.c         | 15 +++++++++++++++
 4 files changed, 15 insertions(+), 9 deletions(-)

-- 
2.43.0

[PATCH 1/3] hw/cxl: Get rid of unused cfmw_list

[Jonathan Cameron via]

From: Li Zhijian <lizhijian@fujitsu.com>

There is no user for this member. All '-M cxl-fmw.N' options have
been parsed and saved to CXLState.fixed_windows.

Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 include/hw/cxl/cxl.h | 1 -
 hw/cxl/cxl-host.c    | 1 -
 2 files changed, 2 deletions(-)

diff --git a/include/hw/cxl/cxl.h b/include/hw/cxl/cxl.h
index 75e47b6864..e3ecbef038 100644
--- a/include/hw/cxl/cxl.h
+++ b/include/hw/cxl/cxl.h
@@ -43,7 +43,6 @@ typedef struct CXLState {
     MemoryRegion host_mr;
     unsigned int next_mr_idx;
     GList *fixed_windows;
-    CXLFixedMemoryWindowOptionsList *cfmw_list;
 } CXLState;
 
 struct CXLHost {
diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
index c5f5fcfd64..926d3d3da7 100644
--- a/hw/cxl/cxl-host.c
+++ b/hw/cxl/cxl-host.c
@@ -335,7 +335,6 @@ static void machine_set_cfmw(Object *obj, Visitor *v, const char *name,
     for (it = cfmw_list; it; it = it->next) {
         cxl_fixed_memory_window_config(state, it->value, errp);
     }
-    state->cfmw_list = cfmw_list;
 }
 
 void cxl_machine_init(Object *obj, CXLState *state)
-- 
2.43.0

Re: [PATCH 1/3] hw/cxl: Get rid of unused cfmw_list

[Zhijian Li (Fujitsu) via]

Jonathan,


There is a new user for cfmw_list now
https://lore.kernel.org/qemu-devel/20240704093404.1848132-1-zhao1.liu@linux.intel.com/

So I think we should drop this patch.


On 02/07/2024 22:34, Jonathan Cameron wrote:
> From: Li Zhijian <lizhijian@fujitsu.com>
> 
> There is no user for this member. All '-M cxl-fmw.N' options have
> been parsed and saved to CXLState.fixed_windows.
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>   include/hw/cxl/cxl.h | 1 -
>   hw/cxl/cxl-host.c    | 1 -
>   2 files changed, 2 deletions(-)
> 
> diff --git a/include/hw/cxl/cxl.h b/include/hw/cxl/cxl.h
> index 75e47b6864..e3ecbef038 100644
> --- a/include/hw/cxl/cxl.h
> +++ b/include/hw/cxl/cxl.h
> @@ -43,7 +43,6 @@ typedef struct CXLState {
>       MemoryRegion host_mr;
>       unsigned int next_mr_idx;
>       GList *fixed_windows;
> -    CXLFixedMemoryWindowOptionsList *cfmw_list;
>   } CXLState;
>   
>   struct CXLHost {
> diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
> index c5f5fcfd64..926d3d3da7 100644
> --- a/hw/cxl/cxl-host.c
> +++ b/hw/cxl/cxl-host.c
> @@ -335,7 +335,6 @@ static void machine_set_cfmw(Object *obj, Visitor *v, const char *name,
>       for (it = cfmw_list; it; it = it->next) {
>           cxl_fixed_memory_window_config(state, it->value, errp);
>       }
> -    state->cfmw_list = cfmw_list;
>   }
>   
>   void cxl_machine_init(Object *obj, CXLState *state)

Re: [PATCH 1/3] hw/cxl: Get rid of unused cfmw_list

[Zhao Liu]

On Fri, Jul 05, 2024 at 01:04:51AM +0000, Zhijian Li (Fujitsu) wrote:
> Date: Fri, 5 Jul 2024 01:04:51 +0000
> From: "Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com>
> Subject: Re: [PATCH 1/3] hw/cxl: Get rid of unused cfmw_list
> 
> Jonathan,
> 
> 
> There is a new user for cfmw_list now
> https://lore.kernel.org/qemu-devel/20240704093404.1848132-1-zhao1.liu@linux.intel.com/
> 
> So I think we should drop this patch.

Hi Zhijian,

I'm not a "real" user and that bug was originally found by code reading.

So that fix won't block your drop. :-)

And I think the fix is worth being laned before cfmw_list gets dropped,
for being able to port backwards to stable QEMU.
 
> On 02/07/2024 22:34, Jonathan Cameron wrote:
> > From: Li Zhijian <lizhijian@fujitsu.com>
> > 
> > There is no user for this member. All '-M cxl-fmw.N' options have
> > been parsed and saved to CXLState.fixed_windows.
> > 
> > Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >   include/hw/cxl/cxl.h | 1 -
> >   hw/cxl/cxl-host.c    | 1 -
> >   2 files changed, 2 deletions(-)
> > 
> > diff --git a/include/hw/cxl/cxl.h b/include/hw/cxl/cxl.h
> > index 75e47b6864..e3ecbef038 100644
> > --- a/include/hw/cxl/cxl.h
> > +++ b/include/hw/cxl/cxl.h
> > @@ -43,7 +43,6 @@ typedef struct CXLState {
> >       MemoryRegion host_mr;
> >       unsigned int next_mr_idx;
> >       GList *fixed_windows;
> > -    CXLFixedMemoryWindowOptionsList *cfmw_list;
> >   } CXLState;
> >   
> >   struct CXLHost {
> > diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
> > index c5f5fcfd64..926d3d3da7 100644
> > --- a/hw/cxl/cxl-host.c
> > +++ b/hw/cxl/cxl-host.c
> > @@ -335,7 +335,6 @@ static void machine_set_cfmw(Object *obj, Visitor *v, const char *name,
> >       for (it = cfmw_list; it; it = it->next) {
> >           cxl_fixed_memory_window_config(state, it->value, errp);
> >       }
> > -    state->cfmw_list = cfmw_list;
> >   }
> >   
> >   void cxl_machine_init(Object *obj, CXLState *state)

Re: [PATCH 1/3] hw/cxl: Get rid of unused cfmw_list

[Zhijian Li (Fujitsu) via]

On 05/07/2024 10:15, Zhao Liu wrote:
>> There is a new user for cfmw_list now
>> https://lore.kernel.org/qemu-devel/20240704093404.1848132-1-zhao1.liu@linux.intel.com/
>>
>> So I think we should drop this patch.

> Hi Zhijian,
> 
> I'm not a "real" user and that bug was originally found by code reading.
> 
> So that fix won't block your drop. 🙂


"hw/cxl: Get rid of unused cfmw_list" is no longer needed.



> 
> And I think the fix is worth being laned before cfmw_list gets dropped,
> for being able to port backwards to stable QEMU.

Your fix[0] requires CXLState.cfmw_list, and I think CXLState.cfmw_list was designed for *get* purpose
but got mistake at that time.

[0] https://lore.kernel.org/qemu-devel/20240704093404.1848132-1-zhao1.liu@linux.intel.com/


>   
>> On 02/07/2024 22:34, Jonathan Cameron wrote:
>>> From: Li Zhijian<lizhijian@fujitsu.com>
>>>
>>> There is no user for this member. All '-M cxl-fmw.N' options have
>>> been parsed and saved to CXLState.fixed_windows.
>>>
>>> Signed-off-by: Li Zhijian<lizhijian@fujitsu.com>
>>> Signed-off-by: Jonathan Cameron<Jonathan.Cameron@huawei.com>
>>> ---
>>>    include/hw/cxl/cxl.h | 1 -
>>>    hw/cxl/cxl-host.c    | 1 -
>>>    2 files changed, 2 deletions(-)
>>>
>>> diff --git a/include/hw/cxl/cxl.h b/include/hw/cxl/cxl.h
>>> index 75e47b6864..e3ecbef038 100644
>>> --- a/include/hw/cxl/cxl.h
>>> +++ b/include/hw/cxl/cxl.h
>>> @@ -43,7 +43,6 @@ typedef struct CXLState {
>>>        MemoryRegion host_mr;
>>>        unsigned int next_mr_idx;
>>>        GList *fixed_windows;
>>> -    CXLFixedMemoryWindowOptionsList *cfmw_list;
>>>    } CXLState;
>>>    

Re: [PATCH 1/3] hw/cxl: Get rid of unused cfmw_list

[Jonathan Cameron via]

On Fri, 5 Jul 2024 02:29:34 +0000
"Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com> wrote:

> On 05/07/2024 10:15, Zhao Liu wrote:
> >> There is a new user for cfmw_list now
> >> https://lore.kernel.org/qemu-devel/20240704093404.1848132-1-zhao1.liu@linux.intel.com/
> >>
> >> So I think we should drop this patch.  
> 
> > Hi Zhijian,
> > 
> > I'm not a "real" user and that bug was originally found by code reading.
> > 
> > So that fix won't block your drop. 🙂  
> 
> 
> "hw/cxl: Get rid of unused cfmw_list" is no longer needed.
> 
> 
> 
> > 
> > And I think the fix is worth being laned before cfmw_list gets dropped,
> > for being able to port backwards to stable QEMU.  
> 
> Your fix[0] requires CXLState.cfmw_list, and I think CXLState.cfmw_list was designed for *get* purpose
> but got mistake at that time.
> 
> [0] https://lore.kernel.org/qemu-devel/20240704093404.1848132-1-zhao1.liu@linux.intel.com/
Ok.  Michael, please ignore this one as the lack of use is result of another bug. 
I hadn't gotten to that fix yet, so missed the connection.

The rest of this series is unrelated.

Jonathan

> 
> 
> >     
> >> On 02/07/2024 22:34, Jonathan Cameron wrote:  
> >>> From: Li Zhijian<lizhijian@fujitsu.com>
> >>>
> >>> There is no user for this member. All '-M cxl-fmw.N' options have
> >>> been parsed and saved to CXLState.fixed_windows.
> >>>
> >>> Signed-off-by: Li Zhijian<lizhijian@fujitsu.com>
> >>> Signed-off-by: Jonathan Cameron<Jonathan.Cameron@huawei.com>
> >>> ---
> >>>    include/hw/cxl/cxl.h | 1 -
> >>>    hw/cxl/cxl-host.c    | 1 -
> >>>    2 files changed, 2 deletions(-)
> >>>
> >>> diff --git a/include/hw/cxl/cxl.h b/include/hw/cxl/cxl.h
> >>> index 75e47b6864..e3ecbef038 100644
> >>> --- a/include/hw/cxl/cxl.h
> >>> +++ b/include/hw/cxl/cxl.h
> >>> @@ -43,7 +43,6 @@ typedef struct CXLState {
> >>>        MemoryRegion host_mr;
> >>>        unsigned int next_mr_idx;
> >>>        GList *fixed_windows;
> >>> -    CXLFixedMemoryWindowOptionsList *cfmw_list;
> >>>    } CXLState;
> >>>     

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

[Jonathan Cameron via]

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 3/3] hw/cxl: Check for multiple mappings of memory backends.

[Jonathan Cameron via]

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

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

[Jonathan Cameron via]

On Tue, 2 Jul 2024 15:34:22 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> 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!
> 
> Based on master + the two DCD series Michael Tsirkin has queued.
> 
> Based-on: [PATCH v8 00/14] Enabling DCD emulation support in Qemu
> Based-on: https://lore.kernel.org/qemu-devel/20240523174651.1089554-1-nifan.cxl@gmail.com/
> Based-on: [PATCH qemu 0/2] hw/cxl: DCD tweaks and improvements.
> Based-on: https://lore.kernel.org/qemu-devel/20240625170805.359278-1-Jonathan.Cameron@huawei.com/

I'll send a v2 that replaces the unused cfmw_list with the fix
that makes us of it to avoid a crash.


> 
> 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.
> 
> Li Zhijian (1):
>   hw/cxl: Get rid of unused cfmw_list
> 
>  include/hw/cxl/cxl.h       |  1 -
>  hw/cxl/cxl-host.c          |  1 -
>  hw/cxl/cxl-mailbox-utils.c |  7 -------
>  hw/mem/cxl_type3.c         | 15 +++++++++++++++
>  4 files changed, 15 insertions(+), 9 deletions(-)
>