On Fri, May 16, 2025 at 05:44:34AM +0000, Zhijian Li (Fujitsu) wrote:
>
>
> On 13/05/2025 19:14, 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.
> Make sense.
>
> Minor comments inline
>
> >
> > 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 <jonathan.came...@huawei.com>
> > ---
> > 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 | 83 +++++++++++--------
> > hw/cxl/cxl-host-stubs.c | 6 +-
> > hw/cxl/cxl-host.c | 169 +++++++++++++++++++++++++++++++-------
> > hw/i386/pc.c | 51 ++++++------
> > 6 files changed, 223 insertions(+), 93 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..20806e5dd4 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,56 +136,62 @@ 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 int cedt_build_cfmws(Object *obj, void *opaque)
>
> Too much unrelated indent updates in this function
I think that is because now the function only process one cfw, while it
used to process all. So the loop is no longer needed within this
function.
fan
>
>
> > {
> > - GList *it;
> > + struct CXLFixedWindow *fw;
> > + Aml *cedt = opaque;
> > + GArray *table_data = cedt->buf;
> > + int i;
> >
> > - for (it = cxls->fixed_windows; it; it = it->next) {
> > - CXLFixedWindow *fw = it->data;
> > - int i;
> > + if (!object_dynamic_cast(obj, TYPE_CXL_FMW)) {
> > + return 0;
>
> Never reach here? It seems the caller has ensured passing TYPE_CXL_FMW obj.
>
>
>
>
> > + }
> > + fw = CXL_FMW(obj);
> >
> > - /* 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, fw->enc_int_ways, 1);
> > + /* Host Bridge Interleave Ways */
> > + build_append_int_noprefix(table_data, fw->enc_int_ways, 1);
> >
> > - /* Host Bridge Interleave Arithmetic */
> > - build_append_int_noprefix(table_data, 0, 1);
> > + /* Host Bridge Interleave Arithmetic */
> > + build_append_int_noprefix(table_data, 0, 1);
> >
> > - /* Reserved */
> > - build_append_int_noprefix(table_data, 0, 2);
> > + /* Reserved */
> > + build_append_int_noprefix(table_data, 0, 2);
> >
> > - /* Host Bridge Interleave Granularity */
> > - build_append_int_noprefix(table_data, fw->enc_int_gran, 4);
> > + /* Host Bridge Interleave Granularity */
> > + build_append_int_noprefix(table_data, fw->enc_int_gran, 4);
> >
> > - /* Window Restrictions */
> > - build_append_int_noprefix(table_data, 0x0f, 2); /* No restrictions
> > */
> > + /* Window Restrictions */
> > + build_append_int_noprefix(table_data, 0x0f, 2);
> >
> > - /* QTG ID */
> > - build_append_int_noprefix(table_data, 0, 2);
> > + /* QTG ID */
> > + build_append_int_noprefix(table_data, 0, 2);
> >
> > - /* Host Bridge List (list of UIDs - currently bus_nr) */
> > - for (i = 0; i < fw->num_targets; i++) {
> > - g_assert(fw->target_hbs[i]);
> > - build_append_int_noprefix(table_data,
> > PXB_DEV(fw->target_hbs[i])->bus_nr, 4);
> > - }
> > + /* Host Bridge List (list of UIDs - currently bus_nr) */
> > + for (i = 0; i < fw->num_targets; i++) {
> > + g_assert(fw->target_hbs[i]);
> > + build_append_int_noprefix(table_data,
> > + PXB_DEV(fw->target_hbs[i])->bus_nr, 4);
> > }
> > +
> > + return 0;
> > }
> >
> > static int cxl_foreach_pxb_hb(Object *obj, void *opaque)
> > @@ -202,6 +209,7 @@ void cxl_build_cedt(GArray *table_offsets, GArray
> > *table_data,
> > BIOSLinker *linker, const char *oem_id,
> > const char *oem_table_id, CXLState *cxl_state)
> > {
> > + GSList *cfmws_list, *iter;
> > Aml *cedt;
> > AcpiTable table = { .sig = "CEDT", .rev = 1, .oem_id = oem_id,
> > .oem_table_id = oem_table_id };
> > @@ -213,7 +221,12 @@ void cxl_build_cedt(GArray *table_offsets, GArray
> > *table_data,
> > /* reserve space for CEDT header */
> >
> > object_child_foreach_recursive(object_get_root(), cxl_foreach_pxb_hb,
> > cedt);
> > - cedt_build_cfmws(cedt->buf, cxl_state);
> > +
> > + cfmws_list = cxl_fmws_get_all_sorted();
> > + for (iter = cfmws_list; iter; iter = iter->next) {
> > + cedt_build_cfmws(iter->data, cedt);
> > + }
> > + g_slist_free(cfmws_list);
> >
> > /* copy AML table into ACPI tables blob and patch header there */
> > g_array_append_vals(table_data, cedt->buf->data, cedt->buf->len);
> > diff --git a/hw/cxl/cxl-host-stubs.c b/hw/cxl/cxl-host-stubs.c
> > index cae4afcdde..13eb6bf6a4 100644
> > --- a/hw/cxl/cxl-host-stubs.c
> > +++ b/hw/cxl/cxl-host-stubs.c
> > @@ -8,8 +8,12 @@
> > #include "hw/cxl/cxl.h"
> > #include "hw/cxl/cxl_host.h"
> >
> > -void cxl_fmws_link_targets(CXLState *stat, Error **errp) {};
> > +void cxl_fmws_link_targets(Error **errp) {};
> > void cxl_machine_init(Object *obj, CXLState *state) {};
> > 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)
> > +{
> > + return base;
> > +};
> >
> > const MemoryRegionOps cfmws_ops;
> > diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
> > index b7aa429ddf..438f2920e1 100644
> > --- a/hw/cxl/cxl-host.c
> > +++ b/hw/cxl/cxl-host.c
> > @@ -22,12 +22,12 @@
> > #include "hw/pci/pcie_port.h"
> > #include "hw/pci-bridge/pci_expander_bridge.h"
> >
> > -static void cxl_fixed_memory_window_config(CXLState *cxl_state,
> > - CXLFixedMemoryWindowOptions
> > *object,
> > +static void cxl_fixed_memory_window_config(CXLFixedMemoryWindowOptions
> > *object,
> > int index, Error **errp)
> > {
> > ERRP_GUARD();
> > - g_autofree CXLFixedWindow *fw = g_malloc0(sizeof(*fw));
> > + DeviceState *dev = qdev_new(TYPE_CXL_FMW);
> > + CXLFixedWindow *fw = CXL_FMW(dev);
> > strList *target;
> > int i;
> >
> > @@ -67,35 +67,41 @@ static void cxl_fixed_memory_window_config(CXLState
> > *cxl_state,
> > fw->targets[i] = g_strdup(target->value);
> > }
> >
> > - cxl_state->fixed_windows = g_list_append(cxl_state->fixed_windows,
> > - g_steal_pointer(&fw));
> > + sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp);
> > +
> > + return;
> > }
> >
> > -void cxl_fmws_link_targets(CXLState *cxl_state, Error **errp)
> > +static int cxl_fmws_link(Object *obj, void *opaque)
> > {
> > - if (cxl_state && cxl_state->fixed_windows) {
> > - GList *it;
> > -
> > - for (it = cxl_state->fixed_windows; it; it = it->next) {
> > - CXLFixedWindow *fw = it->data;
> > - int i;
> > -
> > - for (i = 0; i < fw->num_targets; i++) {
> > - Object *o;
> > - bool ambig;
> > -
> > - o = object_resolve_path_type(fw->targets[i],
> > - TYPE_PXB_CXL_DEV,
> > - &ambig);
> > - if (!o) {
> > - error_setg(errp, "Could not resolve CXLFM target %s",
> > - fw->targets[i]);
> > - return;
> > - }
> > - fw->target_hbs[i] = PXB_CXL_DEV(o);
> > - }
> > + struct CXLFixedWindow *fw;
> > + int i;
> > +
> > + if (!object_dynamic_cast(obj, TYPE_CXL_FMW)) {
> > + return 0;
> > + }
> > + fw = CXL_FMW(obj);
> > +
> > + for (i = 0; i < fw->num_targets; i++) {
> > + Object *o;
> > + bool ambig;
> > +
> > + o = object_resolve_path_type(fw->targets[i], TYPE_PXB_CXL_DEV,
> > + &ambig);
> > + if (!o) {
> > + error_setg(&error_fatal, "Could not resolve CXLFM target %s",
> > + fw->targets[i]);
> > + return 1;
> > }
> > + fw->target_hbs[i] = PXB_CXL_DEV(o);
> > }
> > + return 0;
> > +}
> > +
> > +void cxl_fmws_link_targets(Error **errp)
> > +{
> > + /* Order doesn't matter for this, so no need to build list */
> > + object_child_foreach_recursive(object_get_root(), cxl_fmws_link, NULL);
> > }
> >
> > static bool cxl_hdm_find_target(uint32_t *cache_mem, hwaddr addr,
> > @@ -335,7 +341,7 @@ static void machine_set_cfmw(Object *obj, Visitor *v,
> > const char *name,
> > }
> >
> > for (it = cfmw_list, index = 0; it; it = it->next, index++) {
> > - cxl_fixed_memory_window_config(state, it->value, index, errp);
> > + cxl_fixed_memory_window_config(it->value, index, errp);
> > }
> > state->cfmw_list = cfmw_list;
> > }
> > @@ -373,3 +379,110 @@ void cxl_hook_up_pxb_registers(PCIBus *bus, CXLState
> > *state, Error **errp)
> > }
> > }
> > }
> > +
> > +struct cfmw_update_state {
> > + hwaddr base;
> > + hwaddr maxaddr;
> > +};
> > +
> > +static void cxl_fmws_update(Object *obj, void *opaque)
> > +{
> > + struct CXLFixedWindow *fw;
> > + struct cfmw_update_state *s = opaque;
> > +
> > + if (!object_dynamic_cast(obj, TYPE_CXL_FMW)) {
>
>
> Never reach here? It seems the caller has ensured passing TYPE_CXL_FMW obj.
>
>
> Thanks
> Zhijian
>
> > + return;
> > + }
> > +
> > + fw = CXL_FMW(obj);
> > + if (s->base + fw->size <= s->maxaddr) {
> > + fw->base = s->base;
> > + sysbus_mmio_map(SYS_BUS_DEVICE(fw), 0, fw->base);
> > + s->base += fw->size;
> > + }
> > +
> > + return;
> > +}
> > +
> > +static int cxl_fmws_find(Object *obj, void *opaque)
> > +{
> > + GSList **list = opaque;
> > +
> > + if (!object_dynamic_cast(obj, TYPE_CXL_FMW)) {
> > + return 0;
> > + }
> > + *list = g_slist_prepend(*list, obj);
> > +
> > + return 0;
> > +}
> > +
> > +static GSList *cxl_fmws_get_all(void)
> > +{
> > + GSList *list = NULL;
> > +
> > + object_child_foreach_recursive(object_get_root(), cxl_fmws_find,
> > &list);
> > +
> > + return list;
> > +}
> > +
> > +static gint cfmws_cmp(gconstpointer a, gconstpointer b, gpointer d)
> > +{
> > + const struct CXLFixedWindow *ap = a;
> > + const struct CXLFixedWindow *bp = b;
> > +
> > + return ap->index > bp->index;
> > +}
> > +
> > +GSList *cxl_fmws_get_all_sorted(void)
> > +{
> > + return g_slist_sort_with_data(cxl_fmws_get_all(), cfmws_cmp, NULL);
> > +}
> > +
> > +hwaddr cxl_fmws_set_memmap_and_update_mmio(hwaddr base, hwaddr max_addr)
> > +{
> > + GSList *cfmws_list, *iter;
> > +
> > + struct cfmw_update_state cfmwss = {
> > + .base = base,
> > + .maxaddr = max_addr,
> > + };
> > + cfmws_list = cxl_fmws_get_all_sorted();
> > + for (iter = cfmws_list; iter; iter = iter->next) {
> > + cxl_fmws_update(iter->data, &cfmwss);
> > + }
> > + g_slist_free(cfmws_list);
> > +
> > + return cfmwss.base;
> > +}
> > +
> > +static void cxl_fmw_realize(DeviceState *dev, Error **errp)
> > +{
> > + CXLFixedWindow *fw = CXL_FMW(dev);
> > +
> > + memory_region_init_io(&fw->mr, OBJECT(dev), &cfmws_ops, fw,
> > + "cxl-fixed-memory-region", fw->size);
> > + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &fw->mr);
> > +}
> > +
> > +static void cxl_fmw_class_init(ObjectClass *klass, const void *data)
> > +{
> > + DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > + dc->desc = "CXL Fixed Memory Window";
> > + dc->realize = cxl_fmw_realize;
> > + /* Reason - created by machines as tightly coupled to machine memory
> > map */
> > + dc->user_creatable = false;
> > +}
> > +
> > +static const TypeInfo cxl_fmw_info = {
> > + .name = TYPE_CXL_FMW,
> > + .parent = TYPE_SYS_BUS_DEVICE,
> > + .instance_size = sizeof(CXLFixedWindow),
> > + .class_init = cxl_fmw_class_init,
> > +};
> > +
> > +static void cxl_host_register_types(void)
> > +{
> > + type_register_static(&cxl_fmw_info);
> > +}
> > +type_init(cxl_host_register_types)
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 70656157ca..9978398876 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -630,7 +630,7 @@ void pc_machine_done(Notifier *notifier, void *data)
> > &error_fatal);
> >
> > if (pcms->cxl_devices_state.is_enabled) {
> > - cxl_fmws_link_targets(&pcms->cxl_devices_state, &error_fatal);
> > + cxl_fmws_link_targets(&error_fatal);
> > }
> >
> > /* set the number of CPUs */
> > @@ -739,20 +739,28 @@ static uint64_t pc_get_cxl_range_start(PCMachineState
> > *pcms)
> > return cxl_base;
> > }
> >
> > -static uint64_t pc_get_cxl_range_end(PCMachineState *pcms)
> > +static int cxl_get_fmw_end(Object *obj, void *opaque)
> > {
> > - uint64_t start = pc_get_cxl_range_start(pcms) + MiB;
> > + struct CXLFixedWindow *fw;
> > + uint64_t *start = opaque;
> >
> > - if (pcms->cxl_devices_state.fixed_windows) {
> > - GList *it;
> > -
> > - start = ROUND_UP(start, 256 * MiB);
> > - for (it = pcms->cxl_devices_state.fixed_windows; it; it =
> > it->next) {
> > - CXLFixedWindow *fw = it->data;
> > - start += fw->size;
> > - }
> > + if (!object_dynamic_cast(obj, TYPE_CXL_FMW)) {
> > + return 0;
> > }
> > + fw = CXL_FMW(obj);
> > +
> > + *start += fw->size;
> >
> > + return 0;
> > +}
> > +
> > +static uint64_t pc_get_cxl_range_end(PCMachineState *pcms)
> > +{
> > + uint64_t start = pc_get_cxl_range_start(pcms) + MiB;
> > +
> > + /* Ordering doesn't matter so no need to build a sorted list */
> > + object_child_foreach_recursive(object_get_root(), cxl_get_fmw_end,
> > + &start);
> > return start;
> > }
> >
> > @@ -954,23 +962,10 @@ void pc_memory_init(PCMachineState *pcms,
> > cxl_base = pc_get_cxl_range_start(pcms);
> > memory_region_init(mr, OBJECT(machine), "cxl_host_reg", cxl_size);
> > memory_region_add_subregion(system_memory, cxl_base, mr);
> > - cxl_resv_end = cxl_base + cxl_size;
> > - if (pcms->cxl_devices_state.fixed_windows) {
> > - hwaddr cxl_fmw_base;
> > - GList *it;
> > -
> > - cxl_fmw_base = ROUND_UP(cxl_base + cxl_size, 256 * MiB);
> > - for (it = pcms->cxl_devices_state.fixed_windows; it; it =
> > it->next) {
> > - CXLFixedWindow *fw = it->data;
> > -
> > - fw->base = cxl_fmw_base;
> > - memory_region_init_io(&fw->mr, OBJECT(machine),
> > &cfmws_ops, fw,
> > - "cxl-fixed-memory-region", fw->size);
> > - memory_region_add_subregion(system_memory, fw->base,
> > &fw->mr);
> > - cxl_fmw_base += fw->size;
> > - cxl_resv_end = cxl_fmw_base;
> > - }
> > - }
> > + cxl_base = ROUND_UP(cxl_base + cxl_size, 256 * MiB);
> > +
> > + cxl_resv_end = cxl_fmws_set_memmap_and_update_mmio(cxl_base,
> > + maxphysaddr);
> > }
> >
> > /* Initialize PC system firmware */
--
Fan Ni (From gmail)
