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

[Philippe Mathieu-Daudé]

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);
    ....

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;
}

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");

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");

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");

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");

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'll send a reproducer.