On arm/virt the memory map is set up before any devices are brought
up.  To enable this provide split functions to establish the fw->base
and later to actually map it.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
v14: Update wrt to changes in previous patch.
     Add a do_cfwms_set_memmap_and_update_mmio() utility function
     to reduce code duplication. (Zhijian)
---
 include/hw/cxl/cxl_host.h |  2 ++
 hw/cxl/cxl-host-stubs.c   |  2 ++
 hw/cxl/cxl-host.c         | 43 +++++++++++++++++++++++++++++++++++----
 3 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/include/hw/cxl/cxl_host.h b/include/hw/cxl/cxl_host.h
index 6dce2cde07..aee9d573d6 100644
--- a/include/hw/cxl/cxl_host.h
+++ b/include/hw/cxl/cxl_host.h
@@ -16,6 +16,8 @@
 void cxl_machine_init(Object *obj, CXLState *state);
 void cxl_fmws_link_targets(Error **errp);
 void cxl_hook_up_pxb_registers(PCIBus *bus, CXLState *state, Error **errp);
+hwaddr cxl_fmws_set_memmap(hwaddr base, hwaddr max_addr);
+void cxl_fmws_update_mmio(void);
 hwaddr cxl_fmws_set_memmap_and_update_mmio(hwaddr base, hwaddr max_addr);
 GSList *cxl_fmws_get_all_sorted(void);
 
diff --git a/hw/cxl/cxl-host-stubs.c b/hw/cxl/cxl-host-stubs.c
index 13eb6bf6a4..d9e38618d6 100644
--- a/hw/cxl/cxl-host-stubs.c
+++ b/hw/cxl/cxl-host-stubs.c
@@ -11,6 +11,8 @@
 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(hwaddr base, hwaddr max_addr) { return base; };
+void cxl_fmws_update_mmio(void) {};
 hwaddr cxl_fmws_set_memmap_and_update_mmio(hwaddr base, hwaddr max_addr)
 {
     return base;
diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
index 016a4fdc6a..a1b9980035 100644
--- a/hw/cxl/cxl-host.c
+++ b/hw/cxl/cxl-host.c
@@ -378,11 +378,14 @@ void cxl_hook_up_pxb_registers(PCIBus *bus, CXLState *state, Error **errp)
     }
 }
 
-static void cxl_fmws_update(CXLFixedWindow *fw, hwaddr *base, hwaddr max_addr)
+static void cxl_fmws_update(CXLFixedWindow *fw, hwaddr *base, hwaddr max_addr,
+                            bool update_mmio)
 {
     if (*base + fw->size <= max_addr) {
         fw->base = *base;
-        sysbus_mmio_map(SYS_BUS_DEVICE(fw), 0, fw->base);
+        if (update_mmio) {
+            sysbus_mmio_map(SYS_BUS_DEVICE(fw), 0, fw->base);
+        }
         *base += fw->size;
     }
 }
@@ -421,19 +424,51 @@ 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)
+static hwaddr do_cxl_fmws_set_memmap_and_update_mmio(hwaddr base,
+                                                     hwaddr max_addr,
+                                                     bool update_mmio)
 {
     GSList *cfmws_list, *iter;
 
     cfmws_list = cxl_fmws_get_all_sorted();
     for (iter = cfmws_list; iter; iter = iter->next) {
-        cxl_fmws_update(CXL_FMW(iter->data), &base, max_addr);
+        cxl_fmws_update(CXL_FMW(iter->data), &base, max_addr, update_mmio);
     }
     g_slist_free(cfmws_list);
 
     return base;
 }
 
+hwaddr cxl_fmws_set_memmap(hwaddr base, hwaddr max_addr)
+{
+    return do_cxl_fmws_set_memmap_and_update_mmio(base, max_addr, false);
+}
+
+hwaddr cxl_fmws_set_memmap_and_update_mmio(hwaddr base, hwaddr max_addr)
+{
+    return do_cxl_fmws_set_memmap_and_update_mmio(base, max_addr, true);
+}
+
+static int cxl_fmws_mmio_map(Object *obj, void *opaque)
+{
+    struct CXLFixedWindow *fw;
+
+    if (!object_dynamic_cast(obj, TYPE_CXL_FMW)) {
+        return 0;
+    }
+    fw = CXL_FMW(obj);
+    sysbus_mmio_map(SYS_BUS_DEVICE(fw), 0, fw->base);
+
+    return 0;
+}
+
+void cxl_fmws_update_mmio(void)
+{
+    /* Ordering is not required for this */
+    object_child_foreach_recursive(object_get_root(), cxl_fmws_mmio_map,
+                                   NULL);
+}
+
 static void cxl_fmw_realize(DeviceState *dev, Error **errp)
 {
     CXLFixedWindow *fw = CXL_FMW(dev);
-- 
2.48.1

In patch 2/5, we introduced `cxl_fmws_set_memmap_and_update_mmio()`.

Initially, I assumed patch 3/5 would split `cxl_fmws_set_memmap_and_update_mmio()` into two steps:
1. Traverse CXLFixedWindow and update `fw->base`.
2. Call `sysbus_mmio_map(SYS_BUS_DEVICE(fw), 0, fw->base)`.
For example (my personal preference):
```c
hwaddr cxl_fmws_set_memmap_and_update_mmio(hwaddr base, hwaddr max_addr)
{
     hwaddr end = cxl_fmws_set_memmap(base, max_addr);
     cxl_fmws_update_mmio();
     return end;
}


If we had implemented this design in patch 2/5, patch 3/5 might not be necessary.
The only potential benefit I see in the current patch 3/5 is efficiency improvements
in cxl_fmws_set_memmap_and_update_mmio(), but since the function is typically
called only once and the GLib list (glist) is small, the practical impact should
be minimal.

I'm interested in others' perspectives on this.

Thanks
Zhijian


On 28/05/2025 19:07, Jonathan Cameron via wrote:
> On arm/virt the memory map is set up before any devices are brought
> up.  To enable this provide split functions to establish the fw->base
> and later to actually map it.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> v14: Update wrt to changes in previous patch.
>       Add a do_cfwms_set_memmap_and_update_mmio() utility function
>       to reduce code duplication. (Zhijian)
> ---
>   include/hw/cxl/cxl_host.h |  2 ++
>   hw/cxl/cxl-host-stubs.c   |  2 ++
>   hw/cxl/cxl-host.c         | 43 +++++++++++++++++++++++++++++++++++----
>   3 files changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/cxl/cxl_host.h b/include/hw/cxl/cxl_host.h
> index 6dce2cde07..aee9d573d6 100644
> --- a/include/hw/cxl/cxl_host.h
> +++ b/include/hw/cxl/cxl_host.h
> @@ -16,6 +16,8 @@
>   void cxl_machine_init(Object *obj, CXLState *state);
>   void cxl_fmws_link_targets(Error **errp);
>   void cxl_hook_up_pxb_registers(PCIBus *bus, CXLState *state, Error **errp);
> +hwaddr cxl_fmws_set_memmap(hwaddr base, hwaddr max_addr);
> +void cxl_fmws_update_mmio(void);
>   hwaddr cxl_fmws_set_memmap_and_update_mmio(hwaddr base, hwaddr max_addr);
>   GSList *cxl_fmws_get_all_sorted(void);
>   
> diff --git a/hw/cxl/cxl-host-stubs.c b/hw/cxl/cxl-host-stubs.c
> index 13eb6bf6a4..d9e38618d6 100644
> --- a/hw/cxl/cxl-host-stubs.c
> +++ b/hw/cxl/cxl-host-stubs.c
> @@ -11,6 +11,8 @@
>   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(hwaddr base, hwaddr max_addr) { return base; };
> +void cxl_fmws_update_mmio(void) {};
>   hwaddr cxl_fmws_set_memmap_and_update_mmio(hwaddr base, hwaddr max_addr)
>   {
>       return base;
> diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
> index 016a4fdc6a..a1b9980035 100644
> --- a/hw/cxl/cxl-host.c
> +++ b/hw/cxl/cxl-host.c
> @@ -378,11 +378,14 @@ void cxl_hook_up_pxb_registers(PCIBus *bus, CXLState *state, Error **errp)
>       }
>   }
>   
> -static void cxl_fmws_update(CXLFixedWindow *fw, hwaddr *base, hwaddr max_addr)
> +static void cxl_fmws_update(CXLFixedWindow *fw, hwaddr *base, hwaddr max_addr,
> +                            bool update_mmio)
>   {
>       if (*base + fw->size <= max_addr) {
>           fw->base = *base;
> -        sysbus_mmio_map(SYS_BUS_DEVICE(fw), 0, fw->base);
> +        if (update_mmio) {
> +            sysbus_mmio_map(SYS_BUS_DEVICE(fw), 0, fw->base);
> +        }
>           *base += fw->size;
>       }
>   }
> @@ -421,19 +424,51 @@ 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)
> +static hwaddr do_cxl_fmws_set_memmap_and_update_mmio(hwaddr base,
> +                                                     hwaddr max_addr,
> +                                                     bool update_mmio)
>   {
>       GSList *cfmws_list, *iter;
>   
>       cfmws_list = cxl_fmws_get_all_sorted();
>       for (iter = cfmws_list; iter; iter = iter->next) {
> -        cxl_fmws_update(CXL_FMW(iter->data), &base, max_addr);
> +        cxl_fmws_update(CXL_FMW(iter->data), &base, max_addr, update_mmio);
>       }
>       g_slist_free(cfmws_list);
>   
>       return base;
>   }
>   
> +hwaddr cxl_fmws_set_memmap(hwaddr base, hwaddr max_addr)
> +{
> +    return do_cxl_fmws_set_memmap_and_update_mmio(base, max_addr, false);
> +}
> +
> +hwaddr cxl_fmws_set_memmap_and_update_mmio(hwaddr base, hwaddr max_addr)
> +{
> +    return do_cxl_fmws_set_memmap_and_update_mmio(base, max_addr, true);
> +}
> +
> +static int cxl_fmws_mmio_map(Object *obj, void *opaque)
> +{
> +    struct CXLFixedWindow *fw;
> +
> +    if (!object_dynamic_cast(obj, TYPE_CXL_FMW)) {
> +        return 0;
> +    }
> +    fw = CXL_FMW(obj);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(fw), 0, fw->base);
> +
> +    return 0;
> +}
> +
> +void cxl_fmws_update_mmio(void)
> +{
> +    /* Ordering is not required for this */
> +    object_child_foreach_recursive(object_get_root(), cxl_fmws_mmio_map,
> +                                   NULL);
> +}
> +
>   static void cxl_fmw_realize(DeviceState *dev, Error **errp)
>   {
>       CXLFixedWindow *fw = CXL_FMW(dev);

On Mon, 9 Jun 2025 01:15:10 +0000
"Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com> wrote:

> In patch 2/5, we introduced `cxl_fmws_set_memmap_and_update_mmio()`.
> 
> Initially, I assumed patch 3/5 would split `cxl_fmws_set_memmap_and_update_mmio()` into two steps:
> 1. Traverse CXLFixedWindow and update `fw->base`.
> 2. Call `sysbus_mmio_map(SYS_BUS_DEVICE(fw), 0, fw->base)`.
> For example (my personal preference):
> ```c
> hwaddr cxl_fmws_set_memmap_and_update_mmio(hwaddr base, hwaddr max_addr)
> {
>      hwaddr end = cxl_fmws_set_memmap(base, max_addr);
>      cxl_fmws_update_mmio();
>      return end;
> }
> 
> 
> If we had implemented this design in patch 2/5, patch 3/5 might not be necessary.

At the time of patch 2 we had no justification for the split as for x86 that would
just look like a pointless double loop.

However you are right that this is too complex given it's not a performance path
and perhaps some commentary in the patch description will be enough that no
one minds.

I'll go a little further than you suggest and push the two calls in
your cxl_fmws_set_memmap_and_mmio() into pc.c (patch 2) as that wrapper
isn't adding much value.

I think it is a big enough change that I'll drop tags given on patch 2.

Thanks,

Jonathan


> The only potential benefit I see in the current patch 3/5 is efficiency improvements
> in cxl_fmws_set_memmap_and_update_mmio(), but since the function is typically
> called only once and the GLib list (glist) is small, the practical impact should
> be minimal.
> 
> I'm interested in others' perspectives on this.
> 
> Thanks
> Zhijian
> 
> 
> On 28/05/2025 19:07, Jonathan Cameron via wrote:
> > On arm/virt the memory map is set up before any devices are brought
> > up.  To enable this provide split functions to establish the fw->base
> > and later to actually map it.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> > v14: Update wrt to changes in previous patch.
> >       Add a do_cfwms_set_memmap_and_update_mmio() utility function
> >       to reduce code duplication. (Zhijian)
> > ---
> >   include/hw/cxl/cxl_host.h |  2 ++
> >   hw/cxl/cxl-host-stubs.c   |  2 ++
> >   hw/cxl/cxl-host.c         | 43 +++++++++++++++++++++++++++++++++++----
> >   3 files changed, 43 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/hw/cxl/cxl_host.h b/include/hw/cxl/cxl_host.h
> > index 6dce2cde07..aee9d573d6 100644
> > --- a/include/hw/cxl/cxl_host.h
> > +++ b/include/hw/cxl/cxl_host.h
> > @@ -16,6 +16,8 @@
> >   void cxl_machine_init(Object *obj, CXLState *state);
> >   void cxl_fmws_link_targets(Error **errp);
> >   void cxl_hook_up_pxb_registers(PCIBus *bus, CXLState *state, Error **errp);
> > +hwaddr cxl_fmws_set_memmap(hwaddr base, hwaddr max_addr);
> > +void cxl_fmws_update_mmio(void);
> >   hwaddr cxl_fmws_set_memmap_and_update_mmio(hwaddr base, hwaddr max_addr);
> >   GSList *cxl_fmws_get_all_sorted(void);
> >   
> > diff --git a/hw/cxl/cxl-host-stubs.c b/hw/cxl/cxl-host-stubs.c
> > index 13eb6bf6a4..d9e38618d6 100644
> > --- a/hw/cxl/cxl-host-stubs.c
> > +++ b/hw/cxl/cxl-host-stubs.c
> > @@ -11,6 +11,8 @@
> >   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(hwaddr base, hwaddr max_addr) { return base; };
> > +void cxl_fmws_update_mmio(void) {};
> >   hwaddr cxl_fmws_set_memmap_and_update_mmio(hwaddr base, hwaddr max_addr)
> >   {
> >       return base;
> > diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
> > index 016a4fdc6a..a1b9980035 100644
> > --- a/hw/cxl/cxl-host.c
> > +++ b/hw/cxl/cxl-host.c
> > @@ -378,11 +378,14 @@ void cxl_hook_up_pxb_registers(PCIBus *bus, CXLState *state, Error **errp)
> >       }
> >   }
> >   
> > -static void cxl_fmws_update(CXLFixedWindow *fw, hwaddr *base, hwaddr max_addr)
> > +static void cxl_fmws_update(CXLFixedWindow *fw, hwaddr *base, hwaddr max_addr,
> > +                            bool update_mmio)
> >   {
> >       if (*base + fw->size <= max_addr) {
> >           fw->base = *base;
> > -        sysbus_mmio_map(SYS_BUS_DEVICE(fw), 0, fw->base);
> > +        if (update_mmio) {
> > +            sysbus_mmio_map(SYS_BUS_DEVICE(fw), 0, fw->base);
> > +        }
> >           *base += fw->size;
> >       }
> >   }
> > @@ -421,19 +424,51 @@ 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)
> > +static hwaddr do_cxl_fmws_set_memmap_and_update_mmio(hwaddr base,
> > +                                                     hwaddr max_addr,
> > +                                                     bool update_mmio)
> >   {
> >       GSList *cfmws_list, *iter;
> >   
> >       cfmws_list = cxl_fmws_get_all_sorted();
> >       for (iter = cfmws_list; iter; iter = iter->next) {
> > -        cxl_fmws_update(CXL_FMW(iter->data), &base, max_addr);
> > +        cxl_fmws_update(CXL_FMW(iter->data), &base, max_addr, update_mmio);
> >       }
> >       g_slist_free(cfmws_list);
> >   
> >       return base;
> >   }
> >   
> > +hwaddr cxl_fmws_set_memmap(hwaddr base, hwaddr max_addr)
> > +{
> > +    return do_cxl_fmws_set_memmap_and_update_mmio(base, max_addr, false);
> > +}
> > +
> > +hwaddr cxl_fmws_set_memmap_and_update_mmio(hwaddr base, hwaddr max_addr)
> > +{
> > +    return do_cxl_fmws_set_memmap_and_update_mmio(base, max_addr, true);
> > +}
> > +
> > +static int cxl_fmws_mmio_map(Object *obj, void *opaque)
> > +{
> > +    struct CXLFixedWindow *fw;
> > +
> > +    if (!object_dynamic_cast(obj, TYPE_CXL_FMW)) {
> > +        return 0;
> > +    }
> > +    fw = CXL_FMW(obj);
> > +    sysbus_mmio_map(SYS_BUS_DEVICE(fw), 0, fw->base);
> > +
> > +    return 0;
> > +}
> > +
> > +void cxl_fmws_update_mmio(void)
> > +{
> > +    /* Ordering is not required for this */
> > +    object_child_foreach_recursive(object_get_root(), cxl_fmws_mmio_map,
> > +                                   NULL);
> > +}
> > +
> >   static void cxl_fmw_realize(DeviceState *dev, Error **errp)
> >   {
> >       CXLFixedWindow *fw = CXL_FMW(dev)