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

[Peter Xu <peterx@redhat.com>]

On Wed, Mar 10, 2021 at 08:09:59PM +0100, Philippe Mathieu-Daudé wrote:
> On 3/10/21 6:06 PM, Peter Xu wrote:
> > Phil,
> > 
> > On Tue, Mar 09, 2021 at 10:54:18PM +0100, 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);
> > 
> > Could I ask why you need to set this sysbus mmio region as root MR of as?
> > What's AS used for here?
> > 
> > Btw, normally I see these regions should be initialized with
> > memory_region_init_io(), since normally that MR will need a MemoryRegionOps
> > bound to it to trap MMIOs, iiuc.
> 
> This is the pattern for buses:
> 
> static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>                                          const char *name, int devfn,
>                                          Error **errp)
> {
>     ...
>     memory_region_init(&pci_dev->bus_master_container_region,
>                        OBJECT(pci_dev),
>                        "bus master container", UINT64_MAX);
>     address_space_init(&pci_dev->bus_master_as,
>                        &pci_dev->bus_master_container_region,
>                        pci_dev->name);
>     ....

This one is the pci bus address space container as its name shows, IMHO it
should never be added as subregion of another MR, but only the top parent.

> 
> AUXBus *aux_bus_init(DeviceState *parent, const char *name)
> {
>     AUXBus *bus;
>     Object *auxtoi2c;
> 
>     bus = AUX_BUS(qbus_create(TYPE_AUX_BUS, parent, name));
>     auxtoi2c = object_new_with_props(TYPE_AUXTOI2C, OBJECT(bus), "i2c",
>                                      &error_abort, NULL);
> 
>     bus->bridge = AUXTOI2C(auxtoi2c);
> 
>     /* Memory related. */
>     bus->aux_io = g_malloc(sizeof(*bus->aux_io));
>     memory_region_init(bus->aux_io, OBJECT(bus), "aux-io", 1 * MiB);
>     address_space_init(&bus->aux_addr_space, bus->aux_io, "aux-io");
>     return bus;
> }

This one seems similar - it's only used in aux_map_slave() as the parent MR, so
its mr->addr should still always be zero.

> 
> static void artist_realizefn(DeviceState *dev, Error **errp)
> {
>     ...
>     memory_region_init(&s->mem_as_root, OBJECT(dev), "artist", ~0ull);
>     address_space_init(&s->as, &s->mem_as_root, "artist");

This one is similar with above - mem_as_root is the top MR.

> 
> PCI host:
> 
> PCIBus *gt64120_register(qemu_irq *pic)
> {
>     ...
>     memory_region_init(&d->pci0_mem, OBJECT(dev), "pci0-mem", 4 * GiB);
>     address_space_init(&d->pci0_mem_as, &d->pci0_mem, "pci0-mem");

For this one, I even think pci0_mem_as can be removed since it's never
referenced after initialized..

> 
> Also:
> 
> static void pnv_xive_realize(DeviceState *dev, Error **errp)
> {
>     ...
>     /*
>      * The XiveSource and XiveENDSource objects are realized with the
>      * maximum allowed HW configuration. The ESB MMIO regions will be
>      * resized dynamically when the controller is configured by the FW
>      * to limit accesses to resources not provisioned.
>      */
>     memory_region_init(&xive->ipi_mmio, OBJECT(xive), "xive-vc-ipi",
>                        PNV9_XIVE_VC_SIZE);
>     address_space_init(&xive->ipi_as, &xive->ipi_mmio, "xive-vc-ipi");
>     memory_region_init(&xive->end_mmio, OBJECT(xive), "xive-vc-end",
>                        PNV9_XIVE_VC_SIZE);
>     address_space_init(&xive->end_as, &xive->end_mmio, "xive-vc-end");

Similar here - both are top MRs.

> 
> And:
> 
> static void memory_map_init(void)
> {
>     ...
>     memory_region_init(system_memory, NULL, "system", UINT64_MAX);
>     address_space_init(&address_space_memory, system_memory, "memory");

This is the system address space, system_memory should never be added as a
subregion to other memory region, afaiu..

> 
> Or:
> 
> static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque,
>                                           int devfn)
> {
>     ...
>     /*
>      * Memory region relationships looks like (Address range shows
>      * only lower 32 bits to make it short in length...):
>      *
>      * |-----------------+-------------------+----------|
>      * | Name            | Address range     | Priority |
>      * |-----------------+-------------------+----------+
>      * | amdvi_root      | 00000000-ffffffff |        0 |
>      * |  amdvi_iommu    | 00000000-ffffffff |        1 |
>      * |  amdvi_iommu_ir | fee00000-feefffff |       64 |
>      * |-----------------+-------------------+----------|
>      */
>     memory_region_init(&amdvi_dev_as->root, OBJECT(s),
>                        "amdvi_root", UINT64_MAX);
>     address_space_init(&amdvi_dev_as->as, &amdvi_dev_as->root, name);
>     memory_region_init_io(&amdvi_dev_as->iommu_ir, OBJECT(s),
>                           &amdvi_ir_ops, s, "amd_iommu_ir",
>                           AMDVI_INT_ADDR_SIZE);
>     memory_region_add_subregion_overlap(&amdvi_dev_as->root,
>                                         AMDVI_INT_ADDR_FIRST,
>                                         &amdvi_dev_as->iommu_ir,
>                                         64);
>     memory_region_add_subregion_overlap(&amdvi_dev_as->root, 0,
>                            MEMORY_REGION(&amdvi_dev_as->iommu), 1);

I think this is similar too.

The thing is in your example you passed over the root MR to sysbus_init_mmio()
where it would be further added into the system memory layout as sub-mr.  Maybe
you should create two MRs, one as root MR, the other one as the one passed into
sysbus_init_mmio()?

-- 
Peter Xu