[PATCH 0/3] memory: Display AddressSpace zero-based in 'info mtree'

[PATCH 0/3] memory: Display AddressSpace zero-based in 'info mtree'

[Philippe Mathieu-Daudé]

Hi,

I have been confused for some years by AddressSpace being displayed
based on the physical address of their first MemoryRegion.
The actual fix is quite trivial for my needs, but I might not see
all the possible side effects. Altough the change are restricted
to mtree_info() which is only available on the monitor.

Last patch is a minor cleanup. I ended not using it.

Regards,

Phil.

Philippe Mathieu-Daudé (3):
  memory: Better name 'offset' argument in mtree_print_mr()
  memory: Provide 'base address' argument to mtree_print_mr()
  memory: Make memory_region_to_absolute_addr() take a const
    MemoryRegion

 softmmu/memory.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

-- 
2.26.2

[PATCH 1/3] memory: Better name 'offset' argument in mtree_print_mr()

[Philippe Mathieu-Daudé]

The 'base' argument of mtree_print_mr() actually represents
an offset, not a base address. Rename it as such.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 softmmu/memory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 874a8fccdee..e4d93b2fd6f 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -2925,7 +2925,7 @@ static void mtree_print_mr_owner(const MemoryRegion *mr)
 }
 
 static void mtree_print_mr(const MemoryRegion *mr, unsigned int level,
-                           hwaddr base,
+                           hwaddr offset,
                            MemoryRegionListHead *alias_print_queue,
                            bool owner, bool display_disabled)
 {
@@ -2939,7 +2939,7 @@ static void mtree_print_mr(const MemoryRegion *mr, unsigned int level,
         return;
     }
 
-    cur_start = base + mr->addr;
+    cur_start = offset + mr->addr;
     cur_end = cur_start + MR_SIZE(mr->size);
 
     /*
@@ -2947,7 +2947,7 @@ static void mtree_print_mr(const MemoryRegion *mr, unsigned int level,
      * happen normally. When it happens, we dump something to warn the
      * user who is observing this.
      */
-    if (cur_start < base || cur_end < cur_start) {
+    if (cur_start < offset || cur_end < cur_start) {
         qemu_printf("[DETECTED OVERFLOW!] ");
     }
 
-- 
2.26.2

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

[Philippe Mathieu-Daudé]

Currently AdressSpace are display in 'info mtree' based on
the physical address of their first MemoryRegion. This is
rather confusing.

Provide a 'base' address argument to mtree_print_mr() and
use it in mtree_info() to display AdressSpace always based
at address 0.

Display behavior of MemoryRegions and FlatViews is not modified.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 softmmu/memory.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index e4d93b2fd6f..991d9227a88 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -2925,7 +2925,7 @@ static void mtree_print_mr_owner(const MemoryRegion *mr)
 }
 
 static void mtree_print_mr(const MemoryRegion *mr, unsigned int level,
-                           hwaddr offset,
+                           hwaddr offset, hwaddr base,
                            MemoryRegionListHead *alias_print_queue,
                            bool owner, bool display_disabled)
 {
@@ -2974,7 +2974,7 @@ static void mtree_print_mr(const MemoryRegion *mr, unsigned int level,
             qemu_printf(TARGET_FMT_plx "-" TARGET_FMT_plx
                         " (prio %d, %s%s): alias %s @%s " TARGET_FMT_plx
                         "-" TARGET_FMT_plx "%s",
-                        cur_start, cur_end,
+                        cur_start - base, cur_end - base,
                         mr->priority,
                         mr->nonvolatile ? "nv-" : "",
                         memory_region_type((MemoryRegion *)mr),
@@ -2995,7 +2995,7 @@ static void mtree_print_mr(const MemoryRegion *mr, unsigned int level,
             }
             qemu_printf(TARGET_FMT_plx "-" TARGET_FMT_plx
                         " (prio %d, %s%s): %s%s",
-                        cur_start, cur_end,
+                        cur_start - base, cur_end - base,
                         mr->priority,
                         mr->nonvolatile ? "nv-" : "",
                         memory_region_type((MemoryRegion *)mr),
@@ -3028,7 +3028,7 @@ static void mtree_print_mr(const MemoryRegion *mr, unsigned int level,
     }
 
     QTAILQ_FOREACH(ml, &submr_print_queue, mrqueue) {
-        mtree_print_mr(ml->mr, level + 1, cur_start,
+        mtree_print_mr(ml->mr, level + 1, cur_start, base,
                        alias_print_queue, owner, display_disabled);
     }
 
@@ -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,
+                       &ml_head, owner, disabled);
         qemu_printf("\n");
     }
 
     /* print aliased regions */
     QTAILQ_FOREACH(ml, &ml_head, mrqueue) {
         qemu_printf("memory-region: %s\n", memory_region_name(ml->mr));
-        mtree_print_mr(ml->mr, 1, 0, &ml_head, owner, disabled);
+        mtree_print_mr(ml->mr, 1, 0, 0, &ml_head, owner, disabled);
         qemu_printf("\n");
     }
 
-- 
2.26.2

[PATCH 3/3] memory: Make memory_region_to_absolute_addr() take a const MemoryRegion

[Philippe Mathieu-Daudé]

There is no reason to not have memory_region_to_absolute_addr()
work with a const MemoryRegion. Else we get:

softmmu/memory.c: error: passing argument 1 of ‘memory_region_to_absolute_addr’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
 6666 |     myaddr = memory_region_to_absolute_addr(constmr, addr);
      |                                             ^~
softmmu/memory.c:410:60: note: expected ‘MemoryRegion *’ but argument is of type ‘const MemoryRegion *’
  410 | static hwaddr memory_region_to_absolute_addr(MemoryRegion *mr, hwaddr offset)
      |                                              ~~~~~~~~~~~~~~^~

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 softmmu/memory.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 991d9227a88..6d1e96ba37d 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -407,9 +407,10 @@ static inline uint64_t memory_region_shift_write_access(uint64_t *value,
     return tmp;
 }
 
-static hwaddr memory_region_to_absolute_addr(MemoryRegion *mr, hwaddr offset)
+static hwaddr memory_region_to_absolute_addr(const MemoryRegion *mr,
+                                             hwaddr offset)
 {
-    MemoryRegion *root;
+    const MemoryRegion *root;
     hwaddr abs_addr = offset;
 
     abs_addr += mr->addr;
-- 
2.26.2

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

[Peter Xu]

Phil,

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?

Thanks,

> +                       &ml_head, owner, disabled);
>          qemu_printf("\n");
>      }
>  
>      /* print aliased regions */
>      QTAILQ_FOREACH(ml, &ml_head, mrqueue) {
>          qemu_printf("memory-region: %s\n", memory_region_name(ml->mr));
> -        mtree_print_mr(ml->mr, 1, 0, &ml_head, owner, disabled);
> +        mtree_print_mr(ml->mr, 1, 0, 0, &ml_head, owner, disabled);
>          qemu_printf("\n");
>      }
>  
> -- 
> 2.26.2
> 

-- 
Peter Xu

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

[Philippe Mathieu-Daudé]

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.

FYI I also have to modify:

-- >8 --
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index e19bc9f1c19..41a77e15752 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -2889,7 +2889,7 @@ MemTxResult address_space_read_full(AddressSpace
*as, hwaddr addr,
     if (len > 0) {
         RCU_READ_LOCK_GUARD();
         fv = address_space_to_flatview(as);
-        result = flatview_read(fv, addr, attrs, buf, len);
+        result = flatview_read(fv, as->root->addr + addr, attrs, buf, len);
     }

     return result;
@@ -2905,7 +2905,7 @@ MemTxResult address_space_write(AddressSpace *as,
hwaddr addr,
     if (len > 0) {
         RCU_READ_LOCK_GUARD();
         fv = address_space_to_flatview(as);
-        result = flatview_write(fv, addr, attrs, buf, len);
+        result = flatview_write(fv, as->root->addr + addr, attrs, buf,
len);
     }

     return result;
@@ -3117,7 +3117,8 @@ bool address_space_access_valid(AddressSpace *as,
hwaddr addr,

     RCU_READ_LOCK_GUARD();
     fv = address_space_to_flatview(as);
-    result = flatview_access_valid(fv, addr, len, is_write, attrs);
+    result = flatview_access_valid(fv, as->root->addr + addr, len,
+                                   is_write, attrs);
     return result;
 }
---

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

[Philippe Mathieu-Daudé]

+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?

Thanks,

Phil.

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

[Peter Xu]

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.

Thanks,

>   // here we have as.root.addr = 0
> 
>   sysbus_init_mmio(sbd, &mr);
> }

-- 
Peter Xu

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.

[PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io()

[Philippe Mathieu-Daudé]

No need to create a local ISA I/O MemoryRegion, use get_system_io().

This partly reverts commit 5c63bcf7501527b844f61624957bdba254d75bfc.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/mips/jazz.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index 1a0888a0fd5..9ac9361b7eb 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -158,7 +158,6 @@ static void mips_jazz_init(MachineState *machine,
     rc4030_dma *dmas;
     IOMMUMemoryRegion *rc4030_dma_mr;
     MemoryRegion *isa_mem = g_new(MemoryRegion, 1);
-    MemoryRegion *isa_io = g_new(MemoryRegion, 1);
     MemoryRegion *rtc = g_new(MemoryRegion, 1);
     MemoryRegion *i8042 = g_new(MemoryRegion, 1);
     MemoryRegion *dma_dummy = g_new(MemoryRegion, 1);
@@ -259,11 +258,10 @@ static void mips_jazz_init(MachineState *machine,
     memory_region_add_subregion(address_space, 0x8000d000, dma_dummy);
 
     /* ISA bus: IO space at 0x90000000, mem space at 0x91000000 */
-    memory_region_init(isa_io, NULL, "isa-io", 0x00010000);
     memory_region_init(isa_mem, NULL, "isa-mem", 0x01000000);
-    memory_region_add_subregion(address_space, 0x90000000, isa_io);
+    memory_region_add_subregion(address_space, 0x90000000, get_system_io());
     memory_region_add_subregion(address_space, 0x91000000, isa_mem);
-    isa_bus = isa_bus_new(NULL, isa_mem, isa_io, &error_abort);
+    isa_bus = isa_bus_new(NULL, isa_mem, get_system_io(), &error_abort);
 
     /* ISA devices */
     i8259 = i8259_init(isa_bus, env->irq[4]);
-- 
2.26.2

[PATCH 2/2] NOTFORMERGE memory: Ensure AddressSpace physical base address is zero

[Philippe Mathieu-Daudé]

AddressSpaces aren't suppose to have a physical base address.

Add an assertion to demostrate the problem:

  $ echo info mtree | qemu-system-mips64el -M magnum -S -monitor stdio
  QEMU 5.2.50 monitor - type 'help' for more information
  (qemu) info mtree
  address-space: memory
    0000000000000000-ffffffffffffffff (prio 0, i/o): system
      0000000000000000-0000000007ffffff (prio 0, ram): mips_jazz.ram
      000000001fc00000-000000001fc7dfff (prio 0, rom): mips_jazz.bios
      0000000040000000-00000000407fffff (prio 0, ram): vram
      0000000060000000-000000006007ffff (prio 0, rom): g364fb.rom
      0000000060080000-00000000601fffff (prio 0, i/o): ctrl
      0000000080000000-00000000800002ff (prio 0, i/o): rc4030.chipset
      0000000080001000-00000000800010ff (prio 0, i/o): dp8393x-regs
      0000000080002000-000000008000200f (prio 0, i/o): esp-regs
      0000000080003000-0000000080003007 (prio 0, i/o): fdc
      0000000080004000-0000000080004fff (prio 0, i/o): rtc
      0000000080005000-0000000080005fff (prio 0, i/o): i8042
      0000000080006000-0000000080006007 (prio 0, i/o): serial
      0000000080008000-0000000080008007 (prio 0, i/o): parallel
      0000000080009000-000000008000afff (prio 0, i/o): nvram
      000000008000b000-000000008000bfff (prio 0, rom): dp8393x-prom
      000000008000d000-000000008000dfff (prio 0, i/o): dummy_dma
      000000008000f000-000000008000f000 (prio 0, i/o): led
      0000000090000000-000000009000ffff (prio 0, i/o): io
        0000000090000000-0000000090000007 (prio 0, i/o): dma-chan
        0000000090000008-000000009000000f (prio 0, i/o): dma-cont
        0000000090000020-0000000090000021 (prio 0, i/o): pic
        0000000090000040-0000000090000043 (prio 0, i/o): pit
        0000000090000061-0000000090000061 (prio 0, i/o): pcspk
        0000000090000070-0000000090000071 (prio 0, i/o): rtc
          0000000090000070-0000000090000070 (prio 0, i/o): rtc-index
        0000000090000081-0000000090000083 (prio 0, i/o): dma-page
        0000000090000087-0000000090000087 (prio 0, i/o): dma-page
        0000000090000089-000000009000008b (prio 0, i/o): dma-page
        000000009000008f-000000009000008f (prio 0, i/o): dma-page
        00000000900000a0-00000000900000a1 (prio 0, i/o): pic
        00000000900000c0-00000000900000cf (prio 0, i/o): dma-chan
        00000000900000d0-00000000900000df (prio 0, i/o): dma-cont
        00000000900004d0-00000000900004d0 (prio 0, i/o): elcr
        00000000900004d1-00000000900004d1 (prio 0, i/o): elcr
      0000000091000000-0000000091ffffff (prio 0, i/o): isa-mem
      00000000f0000000-00000000f0000fff (prio 0, i/o): rc4030.jazzio
      00000000fff00000-00000000fff7dfff (prio 0, rom): alias mips_jazz.bios @mips_jazz.bios 0000000000000000-000000000007dfff

  address-space: I/O
    0000000090000000-000000009000ffff (prio 0, i/o): io
      0000000090000000-0000000090000007 (prio 0, i/o): dma-chan
      0000000090000008-000000009000000f (prio 0, i/o): dma-cont
      0000000090000020-0000000090000021 (prio 0, i/o): pic
      0000000090000040-0000000090000043 (prio 0, i/o): pit
      0000000090000061-0000000090000061 (prio 0, i/o): pcspk
      0000000090000070-0000000090000071 (prio 0, i/o): rtc
        0000000090000070-0000000090000070 (prio 0, i/o): rtc-index
      0000000090000081-0000000090000083 (prio 0, i/o): dma-page
      0000000090000087-0000000090000087 (prio 0, i/o): dma-page
      0000000090000089-000000009000008b (prio 0, i/o): dma-page
      000000009000008f-000000009000008f (prio 0, i/o): dma-page
      00000000900000a0-00000000900000a1 (prio 0, i/o): pic
      00000000900000c0-00000000900000cf (prio 0, i/o): dma-chan
      00000000900000d0-00000000900000df (prio 0, i/o): dma-cont
      00000000900004d0-00000000900004d0 (prio 0, i/o): elcr
      00000000900004d1-00000000900004d1 (prio 0, i/o): elcr

  qemu-system-mips64el: softmmu/memory.c:3193: mtree_info: Assertion `!as->root->addr' failed.
  Aborted (core dumped)

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 softmmu/memory.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 874a8fccdee..164e7971e5b 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -3190,6 +3190,8 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
         qemu_printf("address-space: %s\n", as->name);
         mtree_print_mr(as->root, 1, 0, &ml_head, owner, disabled);
         qemu_printf("\n");
+        /* address spaces aren't suppose to have a physical base address */
+        assert(!as->root->addr);
     }
 
     /* print aliased regions */
-- 
2.26.2

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

[Peter Xu]

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

Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io()

[Peter Xu]

On Wed, Mar 10, 2021 at 08:12:54PM +0100, Philippe Mathieu-Daudé wrote:
> No need to create a local ISA I/O MemoryRegion, use get_system_io().
> 
> This partly reverts commit 5c63bcf7501527b844f61624957bdba254d75bfc.

I think it's not a clean revert of that, see below.

> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/mips/jazz.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
> index 1a0888a0fd5..9ac9361b7eb 100644
> --- a/hw/mips/jazz.c
> +++ b/hw/mips/jazz.c
> @@ -158,7 +158,6 @@ static void mips_jazz_init(MachineState *machine,
>      rc4030_dma *dmas;
>      IOMMUMemoryRegion *rc4030_dma_mr;
>      MemoryRegion *isa_mem = g_new(MemoryRegion, 1);
> -    MemoryRegion *isa_io = g_new(MemoryRegion, 1);
>      MemoryRegion *rtc = g_new(MemoryRegion, 1);
>      MemoryRegion *i8042 = g_new(MemoryRegion, 1);
>      MemoryRegion *dma_dummy = g_new(MemoryRegion, 1);
> @@ -259,11 +258,10 @@ static void mips_jazz_init(MachineState *machine,
>      memory_region_add_subregion(address_space, 0x8000d000, dma_dummy);
>  
>      /* ISA bus: IO space at 0x90000000, mem space at 0x91000000 */
> -    memory_region_init(isa_io, NULL, "isa-io", 0x00010000);
>      memory_region_init(isa_mem, NULL, "isa-mem", 0x01000000);
> -    memory_region_add_subregion(address_space, 0x90000000, isa_io);
> +    memory_region_add_subregion(address_space, 0x90000000, get_system_io());

The old code has an alias created just for adding subregion into address_space:

-    /* ISA IO space at 0x90000000 */
-    memory_region_init_alias(isa, NULL, "isa_mmio",
-                             get_system_io(), 0, 0x01000000);
-    memory_region_add_subregion(address_space, 0x90000000, isa);
-    isa_mem_base = 0x11000000;

While you didn't revert that part.  Maybe that's the issue?

-- 
Peter Xu

Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io()

[Philippe Mathieu-Daudé]

On 3/10/21 8:49 PM, Peter Xu wrote:
> On Wed, Mar 10, 2021 at 08:12:54PM +0100, Philippe Mathieu-Daudé wrote:
>> No need to create a local ISA I/O MemoryRegion, use get_system_io().
>>
>> This partly reverts commit 5c63bcf7501527b844f61624957bdba254d75bfc.
> 
> I think it's not a clean revert of that, see below.
> 
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/mips/jazz.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
>> index 1a0888a0fd5..9ac9361b7eb 100644
>> --- a/hw/mips/jazz.c
>> +++ b/hw/mips/jazz.c
>> @@ -158,7 +158,6 @@ static void mips_jazz_init(MachineState *machine,
>>      rc4030_dma *dmas;
>>      IOMMUMemoryRegion *rc4030_dma_mr;
>>      MemoryRegion *isa_mem = g_new(MemoryRegion, 1);
>> -    MemoryRegion *isa_io = g_new(MemoryRegion, 1);
>>      MemoryRegion *rtc = g_new(MemoryRegion, 1);
>>      MemoryRegion *i8042 = g_new(MemoryRegion, 1);
>>      MemoryRegion *dma_dummy = g_new(MemoryRegion, 1);
>> @@ -259,11 +258,10 @@ static void mips_jazz_init(MachineState *machine,
>>      memory_region_add_subregion(address_space, 0x8000d000, dma_dummy);
>>  
>>      /* ISA bus: IO space at 0x90000000, mem space at 0x91000000 */
>> -    memory_region_init(isa_io, NULL, "isa-io", 0x00010000);
>>      memory_region_init(isa_mem, NULL, "isa-mem", 0x01000000);
>> -    memory_region_add_subregion(address_space, 0x90000000, isa_io);
>> +    memory_region_add_subregion(address_space, 0x90000000, get_system_io());
> 
> The old code has an alias created just for adding subregion into address_space:
> 
> -    /* ISA IO space at 0x90000000 */
> -    memory_region_init_alias(isa, NULL, "isa_mmio",
> -                             get_system_io(), 0, 0x01000000);
> -    memory_region_add_subregion(address_space, 0x90000000, isa);
> -    isa_mem_base = 0x11000000;
> 
> While you didn't revert that part.  Maybe that's the issue?

Hmm I'll have a look. This is not the series I'm working on, which
is much bigger and not ready for posting yet. I simply looked for
something similar (a bus mapped into sysbus) and remembered the
ISA bus from Jazz machines. I'll see if I can find a better PoC.

Thanks for having a look at this patch,

Phil.

Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io()

[Peter Xu]

On Wed, Mar 10, 2021 at 09:11:40PM +0100, Philippe Mathieu-Daudé wrote:
> >>      /* ISA bus: IO space at 0x90000000, mem space at 0x91000000 */
> >> -    memory_region_init(isa_io, NULL, "isa-io", 0x00010000);
> >>      memory_region_init(isa_mem, NULL, "isa-mem", 0x01000000);
> >> -    memory_region_add_subregion(address_space, 0x90000000, isa_io);
> >> +    memory_region_add_subregion(address_space, 0x90000000, get_system_io());
> > 
> > The old code has an alias created just for adding subregion into address_space:
> > 
> > -    /* ISA IO space at 0x90000000 */
> > -    memory_region_init_alias(isa, NULL, "isa_mmio",
> > -                             get_system_io(), 0, 0x01000000);
> > -    memory_region_add_subregion(address_space, 0x90000000, isa);
> > -    isa_mem_base = 0x11000000;
> > 
> > While you didn't revert that part.  Maybe that's the issue?
> 
> Hmm I'll have a look. This is not the series I'm working on, which
> is much bigger and not ready for posting yet. I simply looked for
> something similar (a bus mapped into sysbus) and remembered the
> ISA bus from Jazz machines. I'll see if I can find a better PoC.

Yeah no worry - it's just that I feel one memory_region_init_alias() call is
probably missing in your huge series somewhere, so that you'll take that alias
MR as subregion rather than the real MR (which is the root of one AS).

-- 
Peter Xu

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.

Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io()

[Jiaxun Yang]

On Thu, Mar 11, 2021, at 3:12 AM, Philippe Mathieu-Daudé wrote:
> No need to create a local ISA I/O MemoryRegion, use get_system_io().
> 
> This partly reverts commit 5c63bcf7501527b844f61624957bdba254d75bfc.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

No luck to boot arcboot with current jazz :-(

https://virtuallyfun.com/wordpress/2011/03/06/windows-nt-4-0-mips-revisited/

> ---
>  hw/mips/jazz.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
>
-- 
- Jiaxun

Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io()

[Philippe Mathieu-Daudé]

On 3/10/21 9:29 PM, Peter Xu wrote:
> On Wed, Mar 10, 2021 at 09:11:40PM +0100, Philippe Mathieu-Daudé wrote:
>>>>      /* ISA bus: IO space at 0x90000000, mem space at 0x91000000 */
>>>> -    memory_region_init(isa_io, NULL, "isa-io", 0x00010000);
>>>>      memory_region_init(isa_mem, NULL, "isa-mem", 0x01000000);
>>>> -    memory_region_add_subregion(address_space, 0x90000000, isa_io);
>>>> +    memory_region_add_subregion(address_space, 0x90000000, get_system_io());
>>>
>>> The old code has an alias created just for adding subregion into address_space:
>>>
>>> -    /* ISA IO space at 0x90000000 */
>>> -    memory_region_init_alias(isa, NULL, "isa_mmio",
>>> -                             get_system_io(), 0, 0x01000000);
>>> -    memory_region_add_subregion(address_space, 0x90000000, isa);
>>> -    isa_mem_base = 0x11000000;
>>>
>>> While you didn't revert that part.  Maybe that's the issue?
>>
>> Hmm I'll have a look. This is not the series I'm working on, which
>> is much bigger and not ready for posting yet. I simply looked for
>> something similar (a bus mapped into sysbus) and remembered the
>> ISA bus from Jazz machines. I'll see if I can find a better PoC.
> 
> Yeah no worry - it's just that I feel one memory_region_init_alias() call is
> probably missing in your huge series somewhere, so that you'll take that alias
> MR as subregion rather than the real MR (which is the root of one AS).

OK, with your earlier comments start + Mark other comment I start
to understand better.

So far:

(1a) AddressSpace is a physical view, its base address must be zero

(1b) AddressSpace aperture is fixed (depends on hardware design,
not changeable at runtime

Therefore due to (1a):
(2) AddressSpace root MemoryRegion is a container and must not be
mmio-mapped anywhere (in particular not on SysBus).

(3) If hardware has a MMIO view of an AddressSpace, it has to be
via a MemoryRegion alias. That way the alias handles paddr offset
adjustment to the zero-based AddressSpace root container MR.
Aliasing allows resizing the alias size without modifying the AS
aperture size (1b).

I'll start adding assertions for (1a) and (2) in the code base and
see if (3) adjustments are required.

Thanks both!

Phil.

Re: [PATCH 3/3] memory: Make memory_region_to_absolute_addr() take a const MemoryRegion

[Philippe Mathieu-Daudé]

Cc'ing qemu-trivial@ for this single patch.

On 3/6/21 12:54 AM, Philippe Mathieu-Daudé wrote:
> There is no reason to not have memory_region_to_absolute_addr()
> work with a const MemoryRegion. Else we get:
> 
> softmmu/memory.c: error: passing argument 1 of ‘memory_region_to_absolute_addr’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
>  6666 |     myaddr = memory_region_to_absolute_addr(constmr, addr);
>       |                                             ^~
> softmmu/memory.c:410:60: note: expected ‘MemoryRegion *’ but argument is of type ‘const MemoryRegion *’
>   410 | static hwaddr memory_region_to_absolute_addr(MemoryRegion *mr, hwaddr offset)
>       |                                              ~~~~~~~~~~~~~~^~
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  softmmu/memory.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 991d9227a88..6d1e96ba37d 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -407,9 +407,10 @@ static inline uint64_t memory_region_shift_write_access(uint64_t *value,
>      return tmp;
>  }
>  
> -static hwaddr memory_region_to_absolute_addr(MemoryRegion *mr, hwaddr offset)
> +static hwaddr memory_region_to_absolute_addr(const MemoryRegion *mr,
> +                                             hwaddr offset)
>  {
> -    MemoryRegion *root;
> +    const MemoryRegion *root;
>      hwaddr abs_addr = offset;
>  
>      abs_addr += mr->addr;
> 

Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io()

[Philippe Mathieu-Daudé]

+Aspeed team

On 3/11/21 1:18 PM, Philippe Mathieu-Daudé wrote:
> On 3/10/21 9:29 PM, Peter Xu wrote:

>> Yeah no worry - it's just that I feel one memory_region_init_alias() call is
>> probably missing in your huge series somewhere, so that you'll take that alias
>> MR as subregion rather than the real MR (which is the root of one AS).
> 
> OK, with your earlier comments start + Mark other comment I start
> to understand better.
> 
> So far:
> 
> (1a) AddressSpace is a physical view, its base address must be zero
> 
> (1b) AddressSpace aperture is fixed (depends on hardware design,
> not changeable at runtime
> 
> Therefore due to (1a):
> (2) AddressSpace root MemoryRegion is a container and must not be
> mmio-mapped anywhere (in particular not on SysBus).
> 
> (3) If hardware has a MMIO view of an AddressSpace, it has to be
> via a MemoryRegion alias. That way the alias handles paddr offset
> adjustment to the zero-based AddressSpace root container MR.
> Aliasing allows resizing the alias size without modifying the AS
> aperture size (1b).
> 
> I'll start adding assertions for (1a) and (2) in the code base and
> see if (3) adjustments are required.

So using:

-- >8 --
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 874a8fccdee..8ce2d7f83b9 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -713,6 +713,12 @@ static MemoryRegion
*memory_region_get_flatview_root(MemoryRegion *mr)
                 continue;
             }
         }
+        if (mr && mr->addr) {
+            error_report("Detected flatview root memory region '%s' with"
+                         " non-zero base address (0x%"HWADDR_PRIx"):
aborting",
+                         memory_region_name(mr), mr->addr);
+            abort();
+        }

         return mr;
     }
---

I get:

$ ./qemu-system-arm -M ast2600-evb
qemu-system-arm: Detected flatview root memory region
'aspeed.fmc-ast2600.flash' with non-zero base address (0x20000000): aborting
Aborted (core dumped)

Indeed:

$ ./qemu-system-arm -M ast2600-evb -S -monitor stdio
QEMU 5.2.50 monitor - type 'help' for more information
(qemu) info mtree
address-space: dma-dram
  0000000080000000-000000017fffffff (prio 0, i/o): aspeed-ram-container
    0000000080000000-00000000bfffffff (prio 0, ram): ram
    00000000c0000000-00000000ffffffff (prio 0, i/o): max_ram

address-space: aspeed.fmc-ast2600-dma-flash
  0000000020000000-000000002fffffff (prio 0, i/o): aspeed.fmc-ast2600.flash
    0000000020000000-0000000027ffffff (prio 0, i/o): aspeed.fmc-ast2600.0
    0000000028000000-000000002fffffff (prio 0, i/o): aspeed.fmc-ast2600.1

address-space: aspeed.fmc-ast2600-dma-dram
  0000000080000000-000000017fffffff (prio 0, i/o): aspeed-ram-container
    0000000080000000-00000000bfffffff (prio 0, ram): ram
    00000000c0000000-00000000ffffffff (prio 0, i/o): max_ram

address-space: aspeed.spi1-ast2600-dma-flash
  0000000030000000-000000003fffffff (prio 0, i/o): aspeed.spi1-ast2600.flash
    0000000030000000-0000000037ffffff (prio 0, i/o): aspeed.spi1-ast2600.0

address-space: aspeed.spi1-ast2600-dma-dram
  0000000080000000-000000017fffffff (prio 0, i/o): aspeed-ram-container
    0000000080000000-00000000bfffffff (prio 0, ram): ram
    00000000c0000000-00000000ffffffff (prio 0, i/o): max_ram

address-space: aspeed.spi2-ast2600-dma-flash
  0000000050000000-000000005fffffff (prio 0, i/o): aspeed.spi2-ast2600.flash
    0000000050000000-0000000057ffffff (prio 0, i/o): aspeed.spi2-ast2600.0

address-space: aspeed.spi2-ast2600-dma-dram
  0000000080000000-000000017fffffff (prio 0, i/o): aspeed-ram-container
    0000000080000000-00000000bfffffff (prio 0, ram): ram
    00000000c0000000-00000000ffffffff (prio 0, i/o): max_ram

Many address spaces not zero-based...

Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io()

[Peter Xu]

On Thu, Mar 11, 2021 at 05:21:49PM +0100, Philippe Mathieu-Daudé wrote:
> +Aspeed team
> 
> On 3/11/21 1:18 PM, Philippe Mathieu-Daudé wrote:
> > On 3/10/21 9:29 PM, Peter Xu wrote:
> 
> >> Yeah no worry - it's just that I feel one memory_region_init_alias() call is
> >> probably missing in your huge series somewhere, so that you'll take that alias
> >> MR as subregion rather than the real MR (which is the root of one AS).
> > 
> > OK, with your earlier comments start + Mark other comment I start
> > to understand better.
> > 
> > So far:
> > 
> > (1a) AddressSpace is a physical view, its base address must be zero
> > 
> > (1b) AddressSpace aperture is fixed (depends on hardware design,
> > not changeable at runtime
> > 
> > Therefore due to (1a):
> > (2) AddressSpace root MemoryRegion is a container and must not be
> > mmio-mapped anywhere (in particular not on SysBus).
> > 
> > (3) If hardware has a MMIO view of an AddressSpace, it has to be
> > via a MemoryRegion alias. That way the alias handles paddr offset
> > adjustment to the zero-based AddressSpace root container MR.
> > Aliasing allows resizing the alias size without modifying the AS
> > aperture size (1b).
> > 
> > I'll start adding assertions for (1a) and (2) in the code base and
> > see if (3) adjustments are required.
> 
> So using:
> 
> -- >8 --
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 874a8fccdee..8ce2d7f83b9 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -713,6 +713,12 @@ static MemoryRegion
> *memory_region_get_flatview_root(MemoryRegion *mr)
>                  continue;
>              }
>          }
> +        if (mr && mr->addr) {
> +            error_report("Detected flatview root memory region '%s' with"
> +                         " non-zero base address (0x%"HWADDR_PRIx"):
> aborting",
> +                         memory_region_name(mr), mr->addr);
> +            abort();
> +        }
> 
>          return mr;
>      }
> ---

Maybe it works, but it looks a bit odd to test here.  What I meant was
something like attached.

> 
> I get:
> 
> $ ./qemu-system-arm -M ast2600-evb
> qemu-system-arm: Detected flatview root memory region
> 'aspeed.fmc-ast2600.flash' with non-zero base address (0x20000000): aborting
> Aborted (core dumped)
> 
> Indeed:
> 
> $ ./qemu-system-arm -M ast2600-evb -S -monitor stdio
> QEMU 5.2.50 monitor - type 'help' for more information
> (qemu) info mtree
> address-space: dma-dram
>   0000000080000000-000000017fffffff (prio 0, i/o): aspeed-ram-container
>     0000000080000000-00000000bfffffff (prio 0, ram): ram
>     00000000c0000000-00000000ffffffff (prio 0, i/o): max_ram
> 
> address-space: aspeed.fmc-ast2600-dma-flash
>   0000000020000000-000000002fffffff (prio 0, i/o): aspeed.fmc-ast2600.flash
>     0000000020000000-0000000027ffffff (prio 0, i/o): aspeed.fmc-ast2600.0
>     0000000028000000-000000002fffffff (prio 0, i/o): aspeed.fmc-ast2600.1
> 
> address-space: aspeed.fmc-ast2600-dma-dram
>   0000000080000000-000000017fffffff (prio 0, i/o): aspeed-ram-container
>     0000000080000000-00000000bfffffff (prio 0, ram): ram
>     00000000c0000000-00000000ffffffff (prio 0, i/o): max_ram
> 
> address-space: aspeed.spi1-ast2600-dma-flash
>   0000000030000000-000000003fffffff (prio 0, i/o): aspeed.spi1-ast2600.flash
>     0000000030000000-0000000037ffffff (prio 0, i/o): aspeed.spi1-ast2600.0
> 
> address-space: aspeed.spi1-ast2600-dma-dram
>   0000000080000000-000000017fffffff (prio 0, i/o): aspeed-ram-container
>     0000000080000000-00000000bfffffff (prio 0, ram): ram
>     00000000c0000000-00000000ffffffff (prio 0, i/o): max_ram
> 
> address-space: aspeed.spi2-ast2600-dma-flash
>   0000000050000000-000000005fffffff (prio 0, i/o): aspeed.spi2-ast2600.flash
>     0000000050000000-0000000057ffffff (prio 0, i/o): aspeed.spi2-ast2600.0
> 
> address-space: aspeed.spi2-ast2600-dma-dram
>   0000000080000000-000000017fffffff (prio 0, i/o): aspeed-ram-container
>     0000000080000000-00000000bfffffff (prio 0, ram): ram
>     00000000c0000000-00000000ffffffff (prio 0, i/o): max_ram
> 
> Many address spaces not zero-based...

Maybe it's still legal to make the root mr a subregion of another, so maybe I'm
completely wrong... then the patch attached won't make any sense either.  It's
just that in my mind each MR should have a "parent" - for normal MR it's the
container MR, then for root MR it's easier to see the AS as its "parent".

Maybe Paolo could clarify this..

Thanks,

-- 
Peter Xu

Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io()

[Peter Xu]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

On Thu, Mar 11, 2021 at 05:21:49PM +0100, Philippe Mathieu-Daudé wrote:
> So using:
> 
> -- >8 --
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 874a8fccdee..8ce2d7f83b9 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -713,6 +713,12 @@ static MemoryRegion
> *memory_region_get_flatview_root(MemoryRegion *mr)
>                  continue;
>              }
>          }
> +        if (mr && mr->addr) {
> +            error_report("Detected flatview root memory region '%s' with"
> +                         " non-zero base address (0x%"HWADDR_PRIx"):
> aborting",
> +                         memory_region_name(mr), mr->addr);
> +            abort();
> +        }
> 
>          return mr;
>      }

(Attaching again...)

-- 
Peter Xu

[-- Attachment #2: 0001-memory-Make-sure-root-MR-won-t-be-added-as-subregion.patch --]
[-- Type: text/plain, Size: 1658 bytes --]

From 4fe7614f2087117aa912fd3d33d43ba02f2b50b1 Mon Sep 17 00:00:00 2001
From: Peter Xu <peterx@redhat.com>
Date: Thu, 11 Mar 2021 11:40:21 -0500
Subject: [PATCH] memory: Make sure root MR won't be added as subregion

Add a bool for MR to mark whether this MR is a root MR of an AS.  We bail out
asap if this MR is added as a subregion of another MR.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/exec/memory.h | 1 +
 softmmu/memory.c      | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index c6fb714e499..211f10e877e 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -477,6 +477,7 @@ struct MemoryRegion {
     bool ram_device;
     bool enabled;
     bool warning_printed; /* For reservations */
+    bool is_root_mr;
     uint8_t vga_logging_count;
     MemoryRegion *alias;
     hwaddr alias_offset;
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 874a8fccdee..aebaa956258 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -2442,6 +2442,7 @@ static void memory_region_add_subregion_common(MemoryRegion *mr,
                                                MemoryRegion *subregion)
 {
     assert(!subregion->container);
+    assert(!subregion->is_root_mr);
     subregion->container = mr;
     subregion->addr = offset;
     memory_region_update_container_subregions(subregion);
@@ -2819,6 +2820,7 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
 {
     memory_region_ref(root);
     as->root = root;
+    root->is_root_mr = true;
     as->current_map = NULL;
     as->ioeventfd_nb = 0;
     as->ioeventfds = NULL;
-- 
2.26.2

Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io()

[Philippe Mathieu-Daudé]

On 3/11/21 5:21 PM, Philippe Mathieu-Daudé wrote:
> +Aspeed team
> 
> On 3/11/21 1:18 PM, Philippe Mathieu-Daudé wrote:
>> On 3/10/21 9:29 PM, Peter Xu wrote:
> 
>>> Yeah no worry - it's just that I feel one memory_region_init_alias() call is
>>> probably missing in your huge series somewhere, so that you'll take that alias
>>> MR as subregion rather than the real MR (which is the root of one AS).
>>
>> OK, with your earlier comments start + Mark other comment I start
>> to understand better.
>>
>> So far:
>>
>> (1a) AddressSpace is a physical view, its base address must be zero
>>
>> (1b) AddressSpace aperture is fixed (depends on hardware design,
>> not changeable at runtime
>>
>> Therefore due to (1a):
>> (2) AddressSpace root MemoryRegion is a container and must not be
>> mmio-mapped anywhere (in particular not on SysBus).
>>
>> (3) If hardware has a MMIO view of an AddressSpace, it has to be
>> via a MemoryRegion alias. That way the alias handles paddr offset
>> adjustment to the zero-based AddressSpace root container MR.
>> Aliasing allows resizing the alias size without modifying the AS
>> aperture size (1b).
>>
>> I'll start adding assertions for (1a) and (2) in the code base and
>> see if (3) adjustments are required.
> 
> So using:
> 
> -- >8 --
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 874a8fccdee..8ce2d7f83b9 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -713,6 +713,12 @@ static MemoryRegion
> *memory_region_get_flatview_root(MemoryRegion *mr)
>                  continue;
>              }
>          }
> +        if (mr && mr->addr) {
> +            error_report("Detected flatview root memory region '%s' with"
> +                         " non-zero base address (0x%"HWADDR_PRIx"):
> aborting",
> +                         memory_region_name(mr), mr->addr);
> +            abort();
> +        }
> 
>          return mr;
>      }
> ---
> 
> I get:
> 
> $ ./qemu-system-arm -M ast2600-evb
> qemu-system-arm: Detected flatview root memory region
> 'aspeed.fmc-ast2600.flash' with non-zero base address (0x20000000): aborting
> Aborted (core dumped)

Another one (PPC):

$ ./qemu-system-ppc -S -monitor stdio -M 40p
QEMU 5.2.50 monitor - type 'help' for more information
(qemu) qemu-system-ppc: Detected flatview root memory region 'pci-io'
with non-zero base address (0x80000000): aborting
Aborted (core dumped)

$ ./qemu-system-ppc -S -monitor stdio -M 40p
QEMU 5.2.50 monitor - type 'help' for more information
(qemu) info mtree
address-space: raven-io
  0000000080000000-00000000bf7fffff (prio 0, i/o): pci-io
    0000000080000000-0000000080000007 (prio 0, i/o): dma-chan
    0000000080000008-000000008000000f (prio 0, i/o): dma-cont
    0000000080000020-0000000080000021 (prio 0, i/o): pic
    0000000080000040-0000000080000043 (prio 0, i/o): pit
    0000000080000060-0000000080000060 (prio 0, i/o): i8042-data
    0000000080000061-0000000080000061 (prio 0, i/o): pcspk
    0000000080000064-0000000080000064 (prio 0, i/o): i8042-cmd
    0000000080000070-0000000080000071 (prio 0, i/o): rtc
      0000000080000070-0000000080000070 (prio 0, i/o): rtc-index
    0000000080000074-0000000080000077 (prio 0, i/o): m48t59
    0000000080000081-0000000080000083 (prio 0, i/o): dma-page
    0000000080000087-0000000080000087 (prio 0, i/o): dma-page
    ...

address-space: lsi-pci-io
  0000000080000000-00000000bf7fffff (prio 0, i/o): pci-io
    0000000080000000-0000000080000007 (prio 0, i/o): dma-chan
    0000000080000008-000000008000000f (prio 0, i/o): dma-cont
    0000000080000020-0000000080000021 (prio 0, i/o): pic
    0000000080000040-0000000080000043 (prio 0, i/o): pit
    0000000080000060-0000000080000060 (prio 0, i/o): i8042-data
    0000000080000061-0000000080000061 (prio 0, i/o): pcspk
    0000000080000064-0000000080000064 (prio 0, i/o): i8042-cmd
    0000000080000070-0000000080000071 (prio 0, i/o): rtc
      0000000080000070-0000000080000070 (prio 0, i/o): rtc-index
      ...

memory-region: pci-memory
  00000000c0000000-00000000feffffff (prio 0, i/o): pci-memory
    00000000c00a0000-00000000c00bffff (prio 1, i/o): vga-lowmem

Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io()

[Philippe Mathieu-Daudé]

On 3/11/21 6:27 PM, Peter Xu wrote:
> On Thu, Mar 11, 2021 at 05:21:49PM +0100, Philippe Mathieu-Daudé wrote:
>> +Aspeed team
>>
>> On 3/11/21 1:18 PM, Philippe Mathieu-Daudé wrote:
>>> On 3/10/21 9:29 PM, Peter Xu wrote:
>>
>>>> Yeah no worry - it's just that I feel one memory_region_init_alias() call is
>>>> probably missing in your huge series somewhere, so that you'll take that alias
>>>> MR as subregion rather than the real MR (which is the root of one AS).
>>>
>>> OK, with your earlier comments start + Mark other comment I start
>>> to understand better.
>>>
>>> So far:
>>>
>>> (1a) AddressSpace is a physical view, its base address must be zero
>>>
>>> (1b) AddressSpace aperture is fixed (depends on hardware design,
>>> not changeable at runtime
>>>
>>> Therefore due to (1a):
>>> (2) AddressSpace root MemoryRegion is a container and must not be
>>> mmio-mapped anywhere (in particular not on SysBus).
>>>
>>> (3) If hardware has a MMIO view of an AddressSpace, it has to be
>>> via a MemoryRegion alias. That way the alias handles paddr offset
>>> adjustment to the zero-based AddressSpace root container MR.
>>> Aliasing allows resizing the alias size without modifying the AS
>>> aperture size (1b).
>>>
>>> I'll start adding assertions for (1a) and (2) in the code base and
>>> see if (3) adjustments are required.
>>
>> So using:
>>
>> -- >8 --
>> diff --git a/softmmu/memory.c b/softmmu/memory.c
>> index 874a8fccdee..8ce2d7f83b9 100644
>> --- a/softmmu/memory.c
>> +++ b/softmmu/memory.c
>> @@ -713,6 +713,12 @@ static MemoryRegion
>> *memory_region_get_flatview_root(MemoryRegion *mr)
>>                  continue;
>>              }
>>          }
>> +        if (mr && mr->addr) {
>> +            error_report("Detected flatview root memory region '%s' with"
>> +                         " non-zero base address (0x%"HWADDR_PRIx"):
>> aborting",
>> +                         memory_region_name(mr), mr->addr);
>> +            abort();
>> +        }
>>
>>          return mr;
>>      }
>> ---
> 
> Maybe it works, but it looks a bit odd to test here.  What I meant was
> something like attached.
> 

> Maybe it's still legal to make the root mr a subregion of another, so maybe I'm
> completely wrong... then the patch attached won't make any sense either.

Maybe it does, and we need to rework some boards code.
With your patch applied:

$ ./qemu-system-ppc -M 40p
qemu-system-ppc: softmmu/memory.c:2445:
memory_region_add_subregion_common: Assertion `!subregion->is_root_mr'
failed.
Aborted (core dumped)

$ ./qemu-system-arm -M ast2600-evb
qemu-system-arm: softmmu/memory.c:2445:
memory_region_add_subregion_common: Assertion `!subregion->is_root_mr'
failed.
Aborted (core dumped)

>  It's
> just that in my mind each MR should have a "parent" - for normal MR it's the
> container MR, then for root MR it's easier to see the AS as its "parent".
> 
> Maybe Paolo could clarify this..
> 
> Thanks,
> 

Re: [PATCH 1/2] hw/mips/jazz: Use generic I/O bus via get_system_io()

[Cédric Le Goater]

On 3/11/21 6:27 PM, Peter Xu wrote:
> On Thu, Mar 11, 2021 at 05:21:49PM +0100, Philippe Mathieu-Daudé wrote:
>> +Aspeed team
>>
>> On 3/11/21 1:18 PM, Philippe Mathieu-Daudé wrote:
>>> On 3/10/21 9:29 PM, Peter Xu wrote:
>>
>>>> Yeah no worry - it's just that I feel one memory_region_init_alias() call is
>>>> probably missing in your huge series somewhere, so that you'll take that alias
>>>> MR as subregion rather than the real MR (which is the root of one AS).
>>>
>>> OK, with your earlier comments start + Mark other comment I start
>>> to understand better.
>>>
>>> So far:
>>>
>>> (1a) AddressSpace is a physical view, its base address must be zero
>>>
>>> (1b) AddressSpace aperture is fixed (depends on hardware design,
>>> not changeable at runtime
>>>
>>> Therefore due to (1a):
>>> (2) AddressSpace root MemoryRegion is a container and must not be
>>> mmio-mapped anywhere (in particular not on SysBus).
>>>
>>> (3) If hardware has a MMIO view of an AddressSpace, it has to be
>>> via a MemoryRegion alias. That way the alias handles paddr offset
>>> adjustment to the zero-based AddressSpace root container MR.
>>> Aliasing allows resizing the alias size without modifying the AS
>>> aperture size (1b).
>>>
>>> I'll start adding assertions for (1a) and (2) in the code base and
>>> see if (3) adjustments are required.
>>
>> So using:
>>
>> -- >8 --
>> diff --git a/softmmu/memory.c b/softmmu/memory.c
>> index 874a8fccdee..8ce2d7f83b9 100644
>> --- a/softmmu/memory.c
>> +++ b/softmmu/memory.c
>> @@ -713,6 +713,12 @@ static MemoryRegion
>> *memory_region_get_flatview_root(MemoryRegion *mr)
>>                  continue;
>>              }
>>          }
>> +        if (mr && mr->addr) {
>> +            error_report("Detected flatview root memory region '%s' with"
>> +                         " non-zero base address (0x%"HWADDR_PRIx"):
>> aborting",
>> +                         memory_region_name(mr), mr->addr);
>> +            abort();
>> +        }
>>
>>          return mr;
>>      }
>> ---
> 
> Maybe it works, but it looks a bit odd to test here.  What I meant was
> something like attached.
> 
>>
>> I get:
>>
>> $ ./qemu-system-arm -M ast2600-evb
>> qemu-system-arm: Detected flatview root memory region
>> 'aspeed.fmc-ast2600.flash' with non-zero base address (0x20000000): aborting
>> Aborted (core dumped)
>>
>> Indeed:
>>
>> $ ./qemu-system-arm -M ast2600-evb -S -monitor stdio
>> QEMU 5.2.50 monitor - type 'help' for more information
>> (qemu) info mtree
>> address-space: dma-dram
>>   0000000080000000-000000017fffffff (prio 0, i/o): aspeed-ram-container
>>     0000000080000000-00000000bfffffff (prio 0, ram): ram
>>     00000000c0000000-00000000ffffffff (prio 0, i/o): max_ram
>>
>> address-space: aspeed.fmc-ast2600-dma-flash
>>   0000000020000000-000000002fffffff (prio 0, i/o): aspeed.fmc-ast2600.flash
>>     0000000020000000-0000000027ffffff (prio 0, i/o): aspeed.fmc-ast2600.0
>>     0000000028000000-000000002fffffff (prio 0, i/o): aspeed.fmc-ast2600.1
>>
>> address-space: aspeed.fmc-ast2600-dma-dram
>>   0000000080000000-000000017fffffff (prio 0, i/o): aspeed-ram-container
>>     0000000080000000-00000000bfffffff (prio 0, ram): ram
>>     00000000c0000000-00000000ffffffff (prio 0, i/o): max_ram
>>
>> address-space: aspeed.spi1-ast2600-dma-flash
>>   0000000030000000-000000003fffffff (prio 0, i/o): aspeed.spi1-ast2600.flash
>>     0000000030000000-0000000037ffffff (prio 0, i/o): aspeed.spi1-ast2600.0
>>
>> address-space: aspeed.spi1-ast2600-dma-dram
>>   0000000080000000-000000017fffffff (prio 0, i/o): aspeed-ram-container
>>     0000000080000000-00000000bfffffff (prio 0, ram): ram
>>     00000000c0000000-00000000ffffffff (prio 0, i/o): max_ram
>>
>> address-space: aspeed.spi2-ast2600-dma-flash
>>   0000000050000000-000000005fffffff (prio 0, i/o): aspeed.spi2-ast2600.flash
>>     0000000050000000-0000000057ffffff (prio 0, i/o): aspeed.spi2-ast2600.0
>>
>> address-space: aspeed.spi2-ast2600-dma-dram
>>   0000000080000000-000000017fffffff (prio 0, i/o): aspeed-ram-container
>>     0000000080000000-00000000bfffffff (prio 0, ram): ram
>>     00000000c0000000-00000000ffffffff (prio 0, i/o): max_ram
>>
>> Many address spaces not zero-based...
> 
> Maybe it's still legal to make the root mr a subregion of another, so maybe I'm
> completely wrong... 

what is the problem you are trying to solve ? 

C. 




> then the patch attached won't make any sense either.  It's
> just that in my mind each MR should have a "parent" - for normal MR it's the
> container MR, then for root MR it's easier to see the AS as its "parent".
> 
> Maybe Paolo could clarify this..
> 
> Thanks,
>