Re: [PATCH 2/3] memory: Provide 'base address' argument to mtree_print_mr()

[Mark Cave-Ayland]

On 09/03/2021 21:54, Philippe Mathieu-Daudé wrote:

> +Peter/Mark/Edgar for SoC modelling
>
> On 3/9/21 10:39 AM, Philippe Mathieu-Daudé wrote:
> > Hi Peter,
> >
> > On 3/9/21 12:40 AM, Peter Xu wrote:> On Sat, Mar 06, 2021 at 12:54:13AM
> > +0100, Philippe Mathieu-Daudé wrote:
> > > > @@ -3188,14 +3188,15 @@ void mtree_info(bool flatview, bool dispatch_tree, bool 
> > > > owner, bool disabled)
> > > >   
> > > >       QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> > > >           qemu_printf("address-space: %s\n", as->name);
> > > > -        mtree_print_mr(as->root, 1, 0, &ml_head, owner, disabled);
> > > > +        mtree_print_mr(as->root, 1, 0, as->root->addr,
> > >
> > > Root MR of any address space should have mr->addr==0, right?
> > >
> > > I'm slightly confused on what this patch wanted to do if so, since then "base"
> > > will always be zero..  Or am I wrong?
> >
> > That is what I am expecting too... Maybe the problem is elsewhere
> > when I create the address space... The simpler way to
> > figure it out is add an assertion. I haven't figure out my
> > issue yet, I'll follow up later with a proof-of-concept series
> > which triggers the assertion.
>
> To trigger I simply use:
>
> mydevice_realize()
> {
>    memory_region_init(&mr, obj, name, size);
>
>    address_space_init(&as, &mr, name);
>    // here we have as.root.addr = 0
>
>    sysbus_init_mmio(sbd, &mr);
> }
>
> soc_realize()
> {
>      ...
>      sysbus_realize(SYS_BUS_DEVICE(&mydevice), &error_abort);
>      sysbus_mmio_map(SYS_BUS_DEVICE(&mydevice), 0, NONZERO_ADDRESS);
>      ...
> }
>
> sysbus_mmio_map
> -> sysbus_mmio_map_common
>     -> memory_region_add_subregion
>        -> memory_region_add_subregion_common
>
> Which does:
>
> static void memory_region_add_subregion_common(MemoryRegion *mr,
>                                                 hwaddr offset,
>                                                 MemoryRegion *subregion)
> {
>      assert(!subregion->container);
>      subregion->container = mr;
>      subregion->addr = offset;
>      // subregion = &as.root
>      // offset = NONZERO_ADDRESS
>      // so here as.root.addr becomes non-zero
>
> Is that use case incorrect? If so:
> - where to add the proper assertions to avoid having address spaces
>    with non-zero base address?
> - what is the proper design?
>
> Else what should we fix?

I think I've seen something like this before with the sun4u IOMMU - I tried to get 
the offset of the IOMMU address space memory region from its parent in 
sun4u_translate_iommu() but I ended up getting back an absolute address instead. I 
think the conclusion I came to was that it wouldn't work with a memory region at the 
root of an address space, so I'd be interested to know what the behaviour here should be.

In terms of soc_realize() I believe there are 2 options for getting the memory 
region: 1) use object_resolve_path_component() to retrieve the memory region as a QOM 
property or 2) use sysbus_mmio_get_region(). Once you have a reference to the memory 
region you can add it to the parent using memory_region_add_subregion() as needed. 
There is an existing example of 2) in hw/sparc64/sun4u.c in ebus_realize().


ATB,

Mark.