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.came...@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);
