Re: [PATCH v14 2/5] hw/cxl: Make the CXL fixed memory windows devices.
On Tue, 10 Jun 2025 07:53:31 +0900
Itaru Kitayama wrote:
> On Thu, May 29, 2025 at 04:08:01PM +0100, Jonathan Cameron wrote:
> > On Wed, 28 May 2025 12:07:23 +0100
> > Jonathan Cameron wrote:
> >
> > > Previously these somewhat device like structures were tracked using a list
> > > in the CXLState in each machine. This is proving restrictive in a few
> > > cases where we need to iterate through these without being aware of the
> > > machine type. Just make them sysbus devices.
> > >
> > > Restrict them to not user created as they need to be visible to early
> > > stages of machine init given effects on the memory map.
> > >
> > > This change both simplifies state tracking and enables features needed
> > > for performance optimization and hotness tracking by making it possible
> > > to retrieve the fixed memory window on actions elsewhere in the topology.
> > >
> > > In some cases the ordering of the Fixed Memory Windows matters.
> > > For those utility functions provide a GSList sorted by the window index.
> > > This ensures that we get consistency across:
> > > - ordering in the command line
> > > - ordering of the host PA ranges
> > > - ordering of ACPI CEDT structures describing the CFMWS.
> > >
> > > Other aspects don't have this constraint. For those direct iteration
> > > of the underlying hash structures is fine.
> > >
> > > Signed-off-by: Jonathan Cameron
> >
> > I'll not post v15 for a while to give time for review, but I just realized
> > this snippet was in a patch I was carrying on top of this and should have
> > been in this patch.
> >
> > diff --git a/include/hw/cxl/cxl.h b/include/hw/cxl/cxl.h
> > index a610795c87..de66ab8c35 100644
> > --- a/include/hw/cxl/cxl.h
> > +++ b/include/hw/cxl/cxl.h
> > @@ -46,7 +46,6 @@ typedef struct CXLState {
> > bool is_enabled;
> > MemoryRegion host_mr;
> > unsigned int next_mr_idx;
> > -GList *fixed_windows;
> > CXLFixedMemoryWindowOptionsList *cfmw_list;
> > } CXLState;
>
> With this one line removed on top of v14, today's Dave's cxl/next kernel makes
> cxl test suite ran through without a single failure.
>
> Tested-by: Itaru Kitayama
>
> Jonathan, Zhi jian of Fujitsu gave you feedback on the QEMU core CXL
> emulation code,
> are you still waiting on any other reviewers to take a look at the series v14
> (or
> v14-ish)?
No - obviously extra review is always good but I wasn't waiting on any.
Just been distracted so not sent it out yet. Should get it out in next day or
two.
Jonathan
>
> Thanks,
> Itaru.
>
> >
Re: [PATCH v14 2/5] hw/cxl: Make the CXL fixed memory windows devices.
On Thu, May 29, 2025 at 04:08:01PM +0100, Jonathan Cameron wrote:
> On Wed, 28 May 2025 12:07:23 +0100
> Jonathan Cameron wrote:
>
> > Previously these somewhat device like structures were tracked using a list
> > in the CXLState in each machine. This is proving restrictive in a few
> > cases where we need to iterate through these without being aware of the
> > machine type. Just make them sysbus devices.
> >
> > Restrict them to not user created as they need to be visible to early
> > stages of machine init given effects on the memory map.
> >
> > This change both simplifies state tracking and enables features needed
> > for performance optimization and hotness tracking by making it possible
> > to retrieve the fixed memory window on actions elsewhere in the topology.
> >
> > In some cases the ordering of the Fixed Memory Windows matters.
> > For those utility functions provide a GSList sorted by the window index.
> > This ensures that we get consistency across:
> > - ordering in the command line
> > - ordering of the host PA ranges
> > - ordering of ACPI CEDT structures describing the CFMWS.
> >
> > Other aspects don't have this constraint. For those direct iteration
> > of the underlying hash structures is fine.
> >
> > Signed-off-by: Jonathan Cameron
>
> I'll not post v15 for a while to give time for review, but I just realized
> this snippet was in a patch I was carrying on top of this and should have
> been in this patch.
>
> diff --git a/include/hw/cxl/cxl.h b/include/hw/cxl/cxl.h
> index a610795c87..de66ab8c35 100644
> --- a/include/hw/cxl/cxl.h
> +++ b/include/hw/cxl/cxl.h
> @@ -46,7 +46,6 @@ typedef struct CXLState {
> bool is_enabled;
> MemoryRegion host_mr;
> unsigned int next_mr_idx;
> -GList *fixed_windows;
> CXLFixedMemoryWindowOptionsList *cfmw_list;
> } CXLState;
With this one line removed on top of v14, today's Dave's cxl/next kernel makes
cxl test suite ran through without a single failure.
Tested-by: Itaru Kitayama
Jonathan, Zhi jian of Fujitsu gave you feedback on the QEMU core CXL emulation
code,
are you still waiting on any other reviewers to take a look at the series v14
(or
v14-ish)?
Thanks,
Itaru.
>
Re: [PATCH v14 2/5] hw/cxl: Make the CXL fixed memory windows devices.
On 28/05/2025 19:07, Jonathan Cameron via wrote: > Previously these somewhat device like structures were tracked using a list > in the CXLState in each machine. This is proving restrictive in a few > cases where we need to iterate through these without being aware of the > machine type. Just make them sysbus devices. > > Restrict them to not user created as they need to be visible to early > stages of machine init given effects on the memory map. > > This change both simplifies state tracking and enables features needed > for performance optimization and hotness tracking by making it possible > to retrieve the fixed memory window on actions elsewhere in the topology. > > In some cases the ordering of the Fixed Memory Windows matters. > For those utility functions provide a GSList sorted by the window index. > This ensures that we get consistency across: > - ordering in the command line > - ordering of the host PA ranges > - ordering of ACPI CEDT structures describing the CFMWS. > > Other aspects don't have this constraint. For those direct iteration > of the underlying hash structures is fine. > > Signed-off-by: Jonathan Cameron LGTM, Reviewed-by: Li Zhijian
Re: [PATCH v14 2/5] hw/cxl: Make the CXL fixed memory windows devices.
On Wed, 28 May 2025 12:07:23 +0100
Jonathan Cameron wrote:
> Previously these somewhat device like structures were tracked using a list
> in the CXLState in each machine. This is proving restrictive in a few
> cases where we need to iterate through these without being aware of the
> machine type. Just make them sysbus devices.
>
> Restrict them to not user created as they need to be visible to early
> stages of machine init given effects on the memory map.
>
> This change both simplifies state tracking and enables features needed
> for performance optimization and hotness tracking by making it possible
> to retrieve the fixed memory window on actions elsewhere in the topology.
>
> In some cases the ordering of the Fixed Memory Windows matters.
> For those utility functions provide a GSList sorted by the window index.
> This ensures that we get consistency across:
> - ordering in the command line
> - ordering of the host PA ranges
> - ordering of ACPI CEDT structures describing the CFMWS.
>
> Other aspects don't have this constraint. For those direct iteration
> of the underlying hash structures is fine.
>
> Signed-off-by: Jonathan Cameron
I'll not post v15 for a while to give time for review, but I just realized
this snippet was in a patch I was carrying on top of this and should have
been in this patch.
diff --git a/include/hw/cxl/cxl.h b/include/hw/cxl/cxl.h
index a610795c87..de66ab8c35 100644
--- a/include/hw/cxl/cxl.h
+++ b/include/hw/cxl/cxl.h
@@ -46,7 +46,6 @@ typedef struct CXLState {
bool is_enabled;
MemoryRegion host_mr;
unsigned int next_mr_idx;
-GList *fixed_windows;
CXLFixedMemoryWindowOptionsList *cfmw_list;
} CXLState;
[PATCH v14 2/5] hw/cxl: Make the CXL fixed memory windows devices.
Previously these somewhat device like structures were tracked using a list
in the CXLState in each machine. This is proving restrictive in a few
cases where we need to iterate through these without being aware of the
machine type. Just make them sysbus devices.
Restrict them to not user created as they need to be visible to early
stages of machine init given effects on the memory map.
This change both simplifies state tracking and enables features needed
for performance optimization and hotness tracking by making it possible
to retrieve the fixed memory window on actions elsewhere in the topology.
In some cases the ordering of the Fixed Memory Windows matters.
For those utility functions provide a GSList sorted by the window index.
This ensures that we get consistency across:
- ordering in the command line
- ordering of the host PA ranges
- ordering of ACPI CEDT structures describing the CFMWS.
Other aspects don't have this constraint. For those direct iteration
of the underlying hash structures is fine.
Signed-off-by: Jonathan Cameron
---
v14: Drop some unnecessary checks on device type. (Zhijian)
Use explicit pointer types given we know what they are at the caller.
- cedt_build_cfmws
- cxl_fmws_update
Cleanup a few things that are left overs from earlier approaches:
- Don't return unnecessarily at end of functions.
- Drop return values that are unused and always zero.
I think Peter Maydell suggested this a long time back when
the original CXL support series was under review but not 100% sure.
---
include/hw/cxl/cxl.h | 3 +
include/hw/cxl/cxl_host.h | 4 +-
hw/acpi/cxl.c | 76 ++--
hw/cxl/cxl-host-stubs.c | 6 +-
hw/cxl/cxl-host.c | 148 ++
hw/i386/pc.c | 51 ++---
6 files changed, 194 insertions(+), 94 deletions(-)
diff --git a/include/hw/cxl/cxl.h b/include/hw/cxl/cxl.h
index b2bcce7ed6..a610795c87 100644
--- a/include/hw/cxl/cxl.h
+++ b/include/hw/cxl/cxl.h
@@ -27,6 +27,7 @@
typedef struct PXBCXLDev PXBCXLDev;
typedef struct CXLFixedWindow {
+SysBusDevice parent_obj;
int index;
uint64_t size;
char **targets;
@@ -38,6 +39,8 @@ typedef struct CXLFixedWindow {
MemoryRegion mr;
hwaddr base;
} CXLFixedWindow;
+#define TYPE_CXL_FMW "cxl-fmw"
+OBJECT_DECLARE_SIMPLE_TYPE(CXLFixedWindow, CXL_FMW)
typedef struct CXLState {
bool is_enabled;
diff --git a/include/hw/cxl/cxl_host.h b/include/hw/cxl/cxl_host.h
index c9bc9c7c50..6dce2cde07 100644
--- a/include/hw/cxl/cxl_host.h
+++ b/include/hw/cxl/cxl_host.h
@@ -14,8 +14,10 @@
#define CXL_HOST_H
void cxl_machine_init(Object *obj, CXLState *state);
-void cxl_fmws_link_targets(CXLState *stat, Error **errp);
+void cxl_fmws_link_targets(Error **errp);
void cxl_hook_up_pxb_registers(PCIBus *bus, CXLState *state, Error **errp);
+hwaddr cxl_fmws_set_memmap_and_update_mmio(hwaddr base, hwaddr max_addr);
+GSList *cxl_fmws_get_all_sorted(void);
extern const MemoryRegionOps cfmws_ops;
diff --git a/hw/acpi/cxl.c b/hw/acpi/cxl.c
index 9cd7905ea2..75d5b30bb8 100644
--- a/hw/acpi/cxl.c
+++ b/hw/acpi/cxl.c
@@ -22,6 +22,7 @@
#include "hw/pci/pci_bridge.h"
#include "hw/pci/pci_host.h"
#include "hw/cxl/cxl.h"
+#include "hw/cxl/cxl_host.h"
#include "hw/mem/memory-device.h"
#include "hw/acpi/acpi.h"
#include "hw/acpi/aml-build.h"
@@ -135,55 +136,52 @@ static void cedt_build_chbs(GArray *table_data, PXBCXLDev
*cxl)
* Interleave ways encoding in CXL 2.0 ECN: 3, 6, 12 and 16-way memory
* interleaving.
*/
-static void cedt_build_cfmws(GArray *table_data, CXLState *cxls)
+static void cedt_build_cfmws(CXLFixedWindow *fw, Aml *cedt)
{
-GList *it;
+GArray *table_data = cedt->buf;
+int i;
-for (it = cxls->fixed_windows; it; it = it->next) {
-CXLFixedWindow *fw = it->data;
-int i;
-
-/* Type */
-build_append_int_noprefix(table_data, 1, 1);
+/* Type */
+build_append_int_noprefix(table_data, 1, 1);
-/* Reserved */
-build_append_int_noprefix(table_data, 0, 1);
+/* Reserved */
+build_append_int_noprefix(table_data, 0, 1);
-/* Record Length */
-build_append_int_noprefix(table_data, 36 + 4 * fw->num_targets, 2);
+/* Record Length */
+build_append_int_noprefix(table_data, 36 + 4 * fw->num_targets, 2);
-/* Reserved */
-build_append_int_noprefix(table_data, 0, 4);
+/* Reserved */
+build_append_int_noprefix(table_data, 0, 4);
-/* Base HPA */
-build_append_int_noprefix(table_data, fw->mr.addr, 8);
+/* Base HPA */
+build_append_int_noprefix(table_data, fw->mr.addr, 8);
-/* Window Size */
-build_append_int_noprefix(table_data, fw->size, 8);
+/* Window Size */
+build_append_int_noprefix(table_data, fw->size, 8);
-/* Host Bridge Interleave Ways */
-build_append_int_noprefix(table_data,
