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
