Re: [PATCH v14 3/5] hw/cxl-host: Allow split of establishing memory address and mmio setup.
On Mon, 9 Jun 2025 01:15:10 +
"Zhijian Li (Fujitsu)" 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
> > ---
> > 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) {
> > -
Re: [PATCH v14 3/5] hw/cxl-host: Allow split of establishing memory address and mmio setup.
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
> ---
> 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
