Re: [PATCH v14 3/5] hw/cxl-host: Allow split of establishing memory address and mmio setup.

[Jonathan Cameron]

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)