[PATCH v2 00/11] memory: Forbid mapping AddressSpace root MemoryRegion

[PATCH v2 00/11] memory: Forbid mapping AddressSpace root MemoryRegion

[Philippe Mathieu-Daudé]

Hi,

This series is the result of a long thread with Peter:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg788366.html
and IRC chats...

AddressSpace are physical address view and shouldn't be using
non-zero base address. The correct way to map a MR used as AS
root is to use a MR alias.

Fix the current incorrect uses, then forbid further use.

Since v1:
- Split the Raven patch in multiple changes, easier to follow/review
  (https://www.mail-archive.com/qemu-devel@nongnu.org/msg791116.html)

Note, the Aspeed patches are already queued in Cédric tree. I had
to cherry-pick them from his tree to have the series pass CI.

Cédric Le Goater (1):
  hw/aspeed/smc: Use the RAM memory region for DMAs

Peter Xu (1):
  memory: Make sure root MR won't be added as subregion

Philippe Mathieu-Daudé (9):
  hw/arm/aspeed: Do not directly map ram container onto main address bus
  hw/arm/aspeed: Do not sysbus-map mmio flash region directly, use alias
  hw/pci-host: Rename Raven ASIC PCI bridge as raven.c
  hw/pci-host/raven: Add PCI_IO_BASE_ADDR definition
  hw/pci-host/raven: Assert PCI I/O AddressSpace is based at 0x80000000
  hw/pci-host/raven: Use MR alias for AS root, not sysbus mapped MR
  hw/pci-host/raven: Remove pointless alias mapping onto system bus
  hw/pci-host/prep: Do not directly map bus-master region onto main bus
  hw/pci-host/raven: Remove temporary assertion 'root MR is zero-based'

 include/exec/memory.h           |  1 +
 include/hw/ssi/aspeed_smc.h     |  1 +
 hw/arm/aspeed.c                 |  8 ++++++--
 hw/pci-host/{prep.c => raven.c} | 19 ++++++++++---------
 hw/ssi/aspeed_smc.c             | 10 +++++-----
 softmmu/memory.c                |  2 ++
 MAINTAINERS                     |  2 +-
 hw/pci-host/Kconfig             |  2 +-
 hw/pci-host/meson.build         |  2 +-
 hw/ppc/Kconfig                  |  2 +-
 10 files changed, 29 insertions(+), 20 deletions(-)
 rename hw/pci-host/{prep.c => raven.c} (96%)

-- 
2.26.3

[PATCH v2 01/11] hw/arm/aspeed: Do not directly map ram container onto main address bus

[Philippe Mathieu-Daudé]

The RAM container is exposed as an AddressSpace.
AddressSpaces root MemoryRegion must not be mapped into other
MemoryRegion, therefore map the RAM container using an alias.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/aspeed.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index a17b75f4940..daeef5b32a2 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -40,6 +40,7 @@ struct AspeedMachineState {
 
     AspeedSoCState soc;
     MemoryRegion ram_container;
+    MemoryRegion ram_container_alias;
     MemoryRegion max_ram;
     bool mmio_exec;
     char *fmc_model;
@@ -339,9 +340,12 @@ static void aspeed_machine_init(MachineState *machine)
     }
     qdev_realize(DEVICE(&bmc->soc), NULL, &error_abort);
 
+    memory_region_init_alias(&bmc->ram_container_alias, NULL,
+                             "ram-container-alias", &bmc->ram_container, 0,
+                             memory_region_size(&bmc->ram_container));
     memory_region_add_subregion(get_system_memory(),
                                 sc->memmap[ASPEED_DEV_SDRAM],
-                                &bmc->ram_container);
+                                &bmc->ram_container_alias);
 
     max_ram_size = object_property_get_uint(OBJECT(&bmc->soc), "max-ram-size",
                                             &error_abort);
-- 
2.26.3

Re: [PATCH v2 01/11] hw/arm/aspeed: Do not directly map ram container onto main address bus

[Peter Xu]

On Sat, Apr 17, 2021 at 12:30:18PM +0200, Philippe Mathieu-Daudé wrote:
> The RAM container is exposed as an AddressSpace.

I didn't see where did ram_container got exposed as an address space.

I see it's added as one subregion of get_system_memory(), which looks okay?

-- 
Peter Xu

Re: [PATCH v2 01/11] hw/arm/aspeed: Do not directly map ram container onto main address bus

[Cédric Le Goater]

On 4/20/21 8:28 PM, Peter Xu wrote:
> On Sat, Apr 17, 2021 at 12:30:18PM +0200, Philippe Mathieu-Daudé wrote:
>> The RAM container is exposed as an AddressSpace.
> 
> I didn't see where did ram_container got exposed as an address space.
> 
> I see it's added as one subregion of get_system_memory(), which looks okay? 
my version of this patch took a simpler approach. See below.

Thanks,

C.

--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -327,7 +327,7 @@ static void aspeed_machine_init(MachineState *machine)
     object_property_set_int(OBJECT(&bmc->soc), "num-cs", amc->num_cs,
                             &error_abort);
     object_property_set_link(OBJECT(&bmc->soc), "dram",
-                             OBJECT(&bmc->ram_container), &error_abort);
+                             OBJECT(machine->ram), &error_abort);
     if (machine->kernel_filename) {
         /*
          * When booting with a -kernel command line there is no u-boot

Re: [PATCH v2 01/11] hw/arm/aspeed: Do not directly map ram container onto main address bus

[Philippe Mathieu-Daudé]

On 4/21/21 7:53 AM, Cédric Le Goater wrote:
> On 4/20/21 8:28 PM, Peter Xu wrote:
>> On Sat, Apr 17, 2021 at 12:30:18PM +0200, Philippe Mathieu-Daudé wrote:
>>> The RAM container is exposed as an AddressSpace.
>>
>> I didn't see where did ram_container got exposed as an address space.

I guess I used the wrong base to git-publish and skipped the first patch =)

>> I see it's added as one subregion of get_system_memory(), which looks okay? 
> my version of this patch took a simpler approach. See below.
> 
> Thanks,
> 
> C.
> 
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -327,7 +327,7 @@ static void aspeed_machine_init(MachineState *machine)
>      object_property_set_int(OBJECT(&bmc->soc), "num-cs", amc->num_cs,
>                              &error_abort);
>      object_property_set_link(OBJECT(&bmc->soc), "dram",
> -                             OBJECT(&bmc->ram_container), &error_abort);
> +                             OBJECT(machine->ram), &error_abort);

This will work as long as no board maps the main memory elsewhere than
0x0. Using the alias make it more robust (and also is good API example
for the usual "use API via copy/pasting" style when adding new board)
IMHO.

>      if (machine->kernel_filename) {
>          /*
>           * When booting with a -kernel command line there is no u-boot
> 
> 

[PATCH v2 02/11] hw/aspeed/smc: Use the RAM memory region for DMAs

[Philippe Mathieu-Daudé]

From: Cédric Le Goater <clg@kaod.org>

Instead of passing the memory address space region, simply use the RAM
memory region instead. This simplifies RAM accesses.

This patch breaks migration compatibility.

Fixes: c4e1f0b48322 ("aspeed/smc: Add support for DMAs")
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Message-Id: <20210407171637.777743-2-clg@kaod.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed.c     | 2 +-
 hw/ssi/aspeed_smc.c | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index daeef5b32a2..4033ffd314c 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -328,7 +328,7 @@ static void aspeed_machine_init(MachineState *machine)
     object_property_set_int(OBJECT(&bmc->soc), "num-cs", amc->num_cs,
                             &error_abort);
     object_property_set_link(OBJECT(&bmc->soc), "dram",
-                             OBJECT(&bmc->ram_container), &error_abort);
+                             OBJECT(machine->ram), &error_abort);
     if (machine->kernel_filename) {
         /*
          * When booting with a -kernel command line there is no u-boot
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 16addee4dc8..6f72fb028e5 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -178,8 +178,7 @@
  *   0: 4 bytes
  *   0x7FFFFF: 32M bytes
  */
-#define DMA_DRAM_ADDR(s, val)   ((s)->sdram_base | \
-                                 ((val) & (s)->ctrl->dma_dram_mask))
+#define DMA_DRAM_ADDR(s, val)   ((val) & (s)->ctrl->dma_dram_mask)
 #define DMA_FLASH_ADDR(s, val)  ((s)->ctrl->flash_window_base | \
                                 ((val) & (s)->ctrl->dma_flash_mask))
 #define DMA_LENGTH(val)         ((val) & 0x01FFFFFC)
-- 
2.26.3

[PATCH v2 03/11] hw/arm/aspeed: Do not sysbus-map mmio flash region directly, use alias

[Philippe Mathieu-Daudé]

The flash mmio region is exposed as an AddressSpace.
AddressSpaces must not be sysbus-mapped, therefore map
the region using an alias.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
[ clg : Fix DMA_FLASH_ADDR() ]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Message-Id: <20210312182851.1922972-3-f4bug@amsat.org>
---
 include/hw/ssi/aspeed_smc.h | 1 +
 hw/ssi/aspeed_smc.c         | 7 ++++---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index 16c03fe64f3..e3c96cecbd8 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -84,6 +84,7 @@ struct AspeedSMCState {
 
     MemoryRegion mmio;
     MemoryRegion mmio_flash;
+    MemoryRegion mmio_flash_alias;
 
     qemu_irq irq;
     int irqline;
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 6f72fb028e5..e06a3b0c5bd 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -179,8 +179,7 @@
  *   0x7FFFFF: 32M bytes
  */
 #define DMA_DRAM_ADDR(s, val)   ((val) & (s)->ctrl->dma_dram_mask)
-#define DMA_FLASH_ADDR(s, val)  ((s)->ctrl->flash_window_base | \
-                                ((val) & (s)->ctrl->dma_flash_mask))
+#define DMA_FLASH_ADDR(s, val)  ((val) & (s)->ctrl->dma_flash_mask)
 #define DMA_LENGTH(val)         ((val) & 0x01FFFFFC)
 
 /* Flash opcodes. */
@@ -1385,7 +1384,9 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
     memory_region_init_io(&s->mmio_flash, OBJECT(s),
                           &aspeed_smc_flash_default_ops, s, name,
                           s->ctrl->flash_window_size);
-    sysbus_init_mmio(sbd, &s->mmio_flash);
+    memory_region_init_alias(&s->mmio_flash_alias, OBJECT(s), name,
+                             &s->mmio_flash, 0, s->ctrl->flash_window_size);
+    sysbus_init_mmio(sbd, &s->mmio_flash_alias);
 
     s->flashes = g_new0(AspeedSMCFlash, s->ctrl->max_peripherals);
 
-- 
2.26.3

[PATCH v2 04/11] hw/pci-host: Rename Raven ASIC PCI bridge as raven.c

[Philippe Mathieu-Daudé]

The ASIC PCI bridge chipset from Motorola is named 'Raven'.
This chipset is used in the PowerPC Reference Platform (PReP),
but not restricted to it. Rename it accordingly.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci-host/{prep.c => raven.c} | 0
 MAINTAINERS                     | 2 +-
 hw/pci-host/Kconfig             | 2 +-
 hw/pci-host/meson.build         | 2 +-
 hw/ppc/Kconfig                  | 2 +-
 5 files changed, 4 insertions(+), 4 deletions(-)
 rename hw/pci-host/{prep.c => raven.c} (100%)

diff --git a/hw/pci-host/prep.c b/hw/pci-host/raven.c
similarity index 100%
rename from hw/pci-host/prep.c
rename to hw/pci-host/raven.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 36055f14c59..0e8f9cbc2ab 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1302,7 +1302,7 @@ S: Maintained
 F: hw/ppc/prep.c
 F: hw/ppc/prep_systemio.c
 F: hw/ppc/rs6000_mc.c
-F: hw/pci-host/prep.[hc]
+F: hw/pci-host/raven.c
 F: hw/isa/i82378.c
 F: hw/isa/pc87312.c
 F: hw/dma/i82374.c
diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
index 2ccc96f02ce..593d90e5588 100644
--- a/hw/pci-host/Kconfig
+++ b/hw/pci-host/Kconfig
@@ -6,7 +6,7 @@ config XEN_IGD_PASSTHROUGH
     default y
     depends on XEN && PCI_I440FX
 
-config PREP_PCI
+config RAVEN_PCI
     bool
     select PCI
     select OR_IRQ
diff --git a/hw/pci-host/meson.build b/hw/pci-host/meson.build
index 87a896973e7..2460f365471 100644
--- a/hw/pci-host/meson.build
+++ b/hw/pci-host/meson.build
@@ -13,7 +13,7 @@
 pci_ss.add(when: 'CONFIG_SH_PCI', if_true: files('sh_pci.c'))
 
 # PPC devices
-pci_ss.add(when: 'CONFIG_PREP_PCI', if_true: files('prep.c'))
+pci_ss.add(when: 'CONFIG_RAVEN_PCI', if_true: files('raven.c'))
 pci_ss.add(when: 'CONFIG_GRACKLE_PCI', if_true: files('grackle.c'))
 # NewWorld PowerMac
 pci_ss.add(when: 'CONFIG_UNIN_PCI', if_true: files('uninorth.c'))
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index d11dc30509d..ebbe95eb90f 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -73,7 +73,7 @@ config PREP
     imply PCI_DEVICES
     imply TEST_DEVICES
     select CS4231A
-    select PREP_PCI
+    select RAVEN_PCI
     select I82378
     select LSI_SCSI_PCI
     select M48T59
-- 
2.26.3

Re: [PATCH v2 04/11] hw/pci-host: Rename Raven ASIC PCI bridge as raven.c

[David Gibson]

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

On Sat, Apr 17, 2021 at 12:30:21PM +0200, Philippe Mathieu-Daudé wrote:
> The ASIC PCI bridge chipset from Motorola is named 'Raven'.
> This chipset is used in the PowerPC Reference Platform (PReP),
> but not restricted to it. Rename it accordingly.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/pci-host/{prep.c => raven.c} | 0
>  MAINTAINERS                     | 2 +-
>  hw/pci-host/Kconfig             | 2 +-
>  hw/pci-host/meson.build         | 2 +-
>  hw/ppc/Kconfig                  | 2 +-
>  5 files changed, 4 insertions(+), 4 deletions(-)
>  rename hw/pci-host/{prep.c => raven.c} (100%)
> 
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/raven.c
> similarity index 100%
> rename from hw/pci-host/prep.c
> rename to hw/pci-host/raven.c
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 36055f14c59..0e8f9cbc2ab 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1302,7 +1302,7 @@ S: Maintained
>  F: hw/ppc/prep.c
>  F: hw/ppc/prep_systemio.c
>  F: hw/ppc/rs6000_mc.c
> -F: hw/pci-host/prep.[hc]
> +F: hw/pci-host/raven.c
>  F: hw/isa/i82378.c
>  F: hw/isa/pc87312.c
>  F: hw/dma/i82374.c
> diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
> index 2ccc96f02ce..593d90e5588 100644
> --- a/hw/pci-host/Kconfig
> +++ b/hw/pci-host/Kconfig
> @@ -6,7 +6,7 @@ config XEN_IGD_PASSTHROUGH
>      default y
>      depends on XEN && PCI_I440FX
>  
> -config PREP_PCI
> +config RAVEN_PCI
>      bool
>      select PCI
>      select OR_IRQ
> diff --git a/hw/pci-host/meson.build b/hw/pci-host/meson.build
> index 87a896973e7..2460f365471 100644
> --- a/hw/pci-host/meson.build
> +++ b/hw/pci-host/meson.build
> @@ -13,7 +13,7 @@
>  pci_ss.add(when: 'CONFIG_SH_PCI', if_true: files('sh_pci.c'))
>  
>  # PPC devices
> -pci_ss.add(when: 'CONFIG_PREP_PCI', if_true: files('prep.c'))
> +pci_ss.add(when: 'CONFIG_RAVEN_PCI', if_true: files('raven.c'))
>  pci_ss.add(when: 'CONFIG_GRACKLE_PCI', if_true: files('grackle.c'))
>  # NewWorld PowerMac
>  pci_ss.add(when: 'CONFIG_UNIN_PCI', if_true: files('uninorth.c'))
> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
> index d11dc30509d..ebbe95eb90f 100644
> --- a/hw/ppc/Kconfig
> +++ b/hw/ppc/Kconfig
> @@ -73,7 +73,7 @@ config PREP
>      imply PCI_DEVICES
>      imply TEST_DEVICES
>      select CS4231A
> -    select PREP_PCI
> +    select RAVEN_PCI
>      select I82378
>      select LSI_SCSI_PCI
>      select M48T59

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 05/11] hw/pci-host/raven: Add PCI_IO_BASE_ADDR definition

[Philippe Mathieu-Daudé]

Rather than using the magic 0x80000000 number for the PCI I/O BAR
physical address on the main system bus, use a definition.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci-host/raven.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index 0a9162fba97..730f31a8931 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -82,6 +82,8 @@ struct PRePPCIState {
 
 #define BIOS_SIZE (1 * MiB)
 
+#define PCI_IO_BASE_ADDR    0x80000000  /* Physical address on main bus */
+
 static inline uint32_t raven_pci_io_config(hwaddr addr)
 {
     int i;
@@ -159,7 +161,7 @@ static uint64_t raven_io_read(void *opaque, hwaddr addr,
     uint8_t buf[4];
 
     addr = raven_io_address(s, addr);
-    address_space_read(&s->pci_io_as, addr + 0x80000000,
+    address_space_read(&s->pci_io_as, addr + PCI_IO_BASE_ADDR,
                        MEMTXATTRS_UNSPECIFIED, buf, size);
 
     if (size == 1) {
@@ -191,7 +193,7 @@ static void raven_io_write(void *opaque, hwaddr addr,
         g_assert_not_reached();
     }
 
-    address_space_write(&s->pci_io_as, addr + 0x80000000,
+    address_space_write(&s->pci_io_as, addr + PCI_IO_BASE_ADDR,
                         MEMTXATTRS_UNSPECIFIED, buf, size);
 }
 
@@ -294,8 +296,9 @@ static void raven_pcihost_initfn(Object *obj)
     address_space_init(&s->pci_io_as, &s->pci_io, "raven-io");
 
     /* CPU address space */
-    memory_region_add_subregion(address_space_mem, 0x80000000, &s->pci_io);
-    memory_region_add_subregion_overlap(address_space_mem, 0x80000000,
+    memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
+                                &s->pci_io);
+    memory_region_add_subregion_overlap(address_space_mem, PCI_IO_BASE_ADDR,
                                         &s->pci_io_non_contiguous, 1);
     memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
     pci_root_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), NULL,
-- 
2.26.3

Re: [PATCH v2 05/11] hw/pci-host/raven: Add PCI_IO_BASE_ADDR definition

[David Gibson]

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

On Sat, Apr 17, 2021 at 12:30:22PM +0200, Philippe Mathieu-Daudé wrote:
> Rather than using the magic 0x80000000 number for the PCI I/O BAR
> physical address on the main system bus, use a definition.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/pci-host/raven.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
> index 0a9162fba97..730f31a8931 100644
> --- a/hw/pci-host/raven.c
> +++ b/hw/pci-host/raven.c
> @@ -82,6 +82,8 @@ struct PRePPCIState {
>  
>  #define BIOS_SIZE (1 * MiB)
>  
> +#define PCI_IO_BASE_ADDR    0x80000000  /* Physical address on main bus */
> +
>  static inline uint32_t raven_pci_io_config(hwaddr addr)
>  {
>      int i;
> @@ -159,7 +161,7 @@ static uint64_t raven_io_read(void *opaque, hwaddr addr,
>      uint8_t buf[4];
>  
>      addr = raven_io_address(s, addr);
> -    address_space_read(&s->pci_io_as, addr + 0x80000000,
> +    address_space_read(&s->pci_io_as, addr + PCI_IO_BASE_ADDR,
>                         MEMTXATTRS_UNSPECIFIED, buf, size);
>  
>      if (size == 1) {
> @@ -191,7 +193,7 @@ static void raven_io_write(void *opaque, hwaddr addr,
>          g_assert_not_reached();
>      }
>  
> -    address_space_write(&s->pci_io_as, addr + 0x80000000,
> +    address_space_write(&s->pci_io_as, addr + PCI_IO_BASE_ADDR,
>                          MEMTXATTRS_UNSPECIFIED, buf, size);
>  }
>  
> @@ -294,8 +296,9 @@ static void raven_pcihost_initfn(Object *obj)
>      address_space_init(&s->pci_io_as, &s->pci_io, "raven-io");
>  
>      /* CPU address space */
> -    memory_region_add_subregion(address_space_mem, 0x80000000, &s->pci_io);
> -    memory_region_add_subregion_overlap(address_space_mem, 0x80000000,
> +    memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
> +                                &s->pci_io);
> +    memory_region_add_subregion_overlap(address_space_mem, PCI_IO_BASE_ADDR,
>                                          &s->pci_io_non_contiguous, 1);
>      memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
>      pci_root_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), NULL,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 06/11] hw/pci-host/raven: Assert PCI I/O AddressSpace is based at 0x80000000

[Philippe Mathieu-Daudé]

Commit 1ae1dc5ba24 ("raven: Set a correct PCI I/O memory region")
abused an AddressSpace API weakness which allows set non-zero base
address to AddressSpace root region. We will fix that in the next
commit. First add an assertion to ensure no regression is introduced.
As raven_io_address() is called by both MemoryRegionOps handlers, it
is a good place for such assert call.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci-host/raven.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index 730f31a8931..36652122424 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -141,6 +141,17 @@ static const MemoryRegionOps raven_intack_ops = {
 static inline hwaddr raven_io_address(PREPPCIState *s,
                                       hwaddr addr)
 {
+    /*
+     * We shouldn't access AddressSpace internals. However this assert
+     * is temporarily introduced to prove a subtle inconsistency from
+     * commit 1ae1dc5ba24 ("raven: Set a correct PCI I/O memory region"):
+     * AddressSpace root region must be zero-based, but the Raven use is not.
+     *
+     * Assert the root region is based on physical address 0x80000000
+     * until the issue is fixed.
+     */
+    assert(s->pci_io_as.root->addr == PCI_IO_BASE_ADDR);
+
     if (s->contiguous_map == 0) {
         /* 64 KB contiguous space for IOs */
         addr &= 0xFFFF;
-- 
2.26.3

Re: [PATCH v2 06/11] hw/pci-host/raven: Assert PCI I/O AddressSpace is based at 0x80000000

[David Gibson]

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

On Sat, Apr 17, 2021 at 12:30:23PM +0200, Philippe Mathieu-Daudé wrote:
> Commit 1ae1dc5ba24 ("raven: Set a correct PCI I/O memory region")
> abused an AddressSpace API weakness which allows set non-zero base
> address to AddressSpace root region. We will fix that in the next
> commit. First add an assertion to ensure no regression is introduced.
> As raven_io_address() is called by both MemoryRegionOps handlers, it
> is a good place for such assert call.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/pci-host/raven.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
> index 730f31a8931..36652122424 100644
> --- a/hw/pci-host/raven.c
> +++ b/hw/pci-host/raven.c
> @@ -141,6 +141,17 @@ static const MemoryRegionOps raven_intack_ops = {
>  static inline hwaddr raven_io_address(PREPPCIState *s,
>                                        hwaddr addr)
>  {
> +    /*
> +     * We shouldn't access AddressSpace internals. However this assert
> +     * is temporarily introduced to prove a subtle inconsistency from
> +     * commit 1ae1dc5ba24 ("raven: Set a correct PCI I/O memory region"):
> +     * AddressSpace root region must be zero-based, but the Raven use is not.
> +     *
> +     * Assert the root region is based on physical address 0x80000000
> +     * until the issue is fixed.
> +     */
> +    assert(s->pci_io_as.root->addr == PCI_IO_BASE_ADDR);
> +
>      if (s->contiguous_map == 0) {
>          /* 64 KB contiguous space for IOs */
>          addr &= 0xFFFF;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 07/11] hw/pci-host/raven: Use MR alias for AS root, not sysbus mapped MR

[Philippe Mathieu-Daudé]

The PCI I/O region is exposed as AddressSpace. AddressSpaces must
not be sysbus-mapped (to be zero-based), therefore map the region
using an alias.

As the new AdressSpace root memory region is now zero-based, we do not
need anymore to add the PCI I/O base address to the MemoryRegionOps
handlers. Update the temporary assertion and its comment.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci-host/raven.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index 36652122424..7bbb7ef0a29 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -67,6 +67,7 @@ struct PRePPCIState {
     PCIBus pci_bus;
     AddressSpace pci_io_as;
     MemoryRegion pci_io;
+    MemoryRegion pci_io_alias;
     MemoryRegion pci_io_non_contiguous;
     MemoryRegion pci_memory;
     MemoryRegion pci_intack;
@@ -143,14 +144,13 @@ static inline hwaddr raven_io_address(PREPPCIState *s,
 {
     /*
      * We shouldn't access AddressSpace internals. However this assert
-     * is temporarily introduced to prove a subtle inconsistency from
-     * commit 1ae1dc5ba24 ("raven: Set a correct PCI I/O memory region"):
-     * AddressSpace root region must be zero-based, but the Raven use is not.
+     * is temporarily used to prove a subtle inconsistency from commit
+     * 1ae1dc5ba24 ("raven: Set a correct PCI I/O memory region") which
+     * expected the PCI I/O root region base address to be 0x80000000.
      *
-     * Assert the root region is based on physical address 0x80000000
-     * until the issue is fixed.
+     * We now use an alias memory region as root, which is zero-based.
      */
-    assert(s->pci_io_as.root->addr == PCI_IO_BASE_ADDR);
+    assert(s->pci_io_as.root->addr == 0);
 
     if (s->contiguous_map == 0) {
         /* 64 KB contiguous space for IOs */
@@ -172,8 +172,7 @@ static uint64_t raven_io_read(void *opaque, hwaddr addr,
     uint8_t buf[4];
 
     addr = raven_io_address(s, addr);
-    address_space_read(&s->pci_io_as, addr + PCI_IO_BASE_ADDR,
-                       MEMTXATTRS_UNSPECIFIED, buf, size);
+    address_space_read(&s->pci_io_as, addr, MEMTXATTRS_UNSPECIFIED, buf, size);
 
     if (size == 1) {
         return buf[0];
@@ -204,8 +203,7 @@ static void raven_io_write(void *opaque, hwaddr addr,
         g_assert_not_reached();
     }
 
-    address_space_write(&s->pci_io_as, addr + PCI_IO_BASE_ADDR,
-                        MEMTXATTRS_UNSPECIFIED, buf, size);
+    address_space_write(&s->pci_io_as, addr, MEMTXATTRS_UNSPECIFIED, buf, size);
 }
 
 static const MemoryRegionOps raven_io_ops = {
@@ -301,6 +299,8 @@ static void raven_pcihost_initfn(Object *obj)
     DeviceState *pci_dev;
 
     memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
+    memory_region_init_alias(&s->pci_io_alias, obj, "pci-io",
+                             &s->pci_io, 0, memory_region_size(&s->pci_io));
     memory_region_init_io(&s->pci_io_non_contiguous, obj, &raven_io_ops, s,
                           "pci-io-non-contiguous", 0x00800000);
     memory_region_init(&s->pci_memory, obj, "pci-memory", 0x3f000000);
@@ -308,7 +308,7 @@ static void raven_pcihost_initfn(Object *obj)
 
     /* CPU address space */
     memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
-                                &s->pci_io);
+                                &s->pci_io_alias);
     memory_region_add_subregion_overlap(address_space_mem, PCI_IO_BASE_ADDR,
                                         &s->pci_io_non_contiguous, 1);
     memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
-- 
2.26.3

[RFC PATCH v2 08/11] hw/pci-host/raven: Remove pointless alias mapping onto system bus

[Philippe Mathieu-Daudé]

The 'pci-io' alias (mapped with priority 0) ends up masked by
the 'pci-io-non-contiguous' I/O region (mapped with priority 1).

The flatview rendering is ('pci-io-non-contiguous' is written
'pci-io-nc'):

(base addr) [               MemoryRegion               ]        [   FlatView   ]

           +----------------------------+---------------+
           |         priority 0         |   priority 1  |
           +----------------------------+---------------+

8000.0000  +-            +--------------+---------------+      +---------------+
                         |   "pci-io"   |  "pci-io-nc"  |      |  "pci-io-nc"  |
                         |              |               |      |               |
                         | / - isadev   |               |      |               |
                         | | - isadev   |               |      |               |
                         | | - isadev   |               +----> |               |
                         | \ - isadev   |               |      |               |
                         |              |               |      |               |
                         |              |               |      |               |
                         |              |               |      |               |
8080.0000  +-------------+              +---------------+      +---------------+
           |   "pciio"   |              |                      |    "pciio"    |
           |             |              |                      |               |
           |             |              |                      |               |
           |             +  -  -  -  -  |--------------------> |               |
           |             |              |                      |               |
           |             |              |                      |               |
           |             |              |                      |               |
80c0.0000  +-------------+              |                      +---------------+
                         |              |                       +-------------+
                         |              |                       |    "???"    |
                         |              |                       |             |
                         |              |        ???????----->  | unassigned? |
                         |              |                       |             |
                         |              |                       |             |
                         |              |                       |             |
bf80.0000  +-            +--------------+                       +-------------+

The before/after this commit memory tree diff is:

  (qemu) info mtree
  memory-region: system
    0000000000000000-ffffffffffffffff (prio 0, i/o): system
      0000000000000000-0000000001ffffff (prio 0, ram): simm.0
      0000000002000000-0000000003ffffff (prio 1, ram): simm.1
      0000000004000000-0000000005ffffff (prio 2, ram): simm.2
      0000000006000000-0000000007ffffff (prio 3, ram): simm.3
      0000000080000000-00000000807fffff (prio 1, i/o): pci-io-non-contiguous
-     0000000080000000-00000000bf7fffff (prio 0, i/o): alias pci-io @pci-io 0000000000000000-000000003f7fffff
      0000000080800000-0000000080bfffff (prio 0, i/o): pciio
      00000000bfffeff0-00000000bfffeff3 (prio 0, i/o): ppc-parity
      00000000bffffff0-00000000bffffff0 (prio 0, i/o): pci-intack
      00000000c0000000-00000000feffffff (prio 0, i/o): pci-memory
        00000000c00a0000-00000000c00bffff (prio 1, i/o): vga-lowmem
      00000000f0000510-00000000f0000511 (prio 0, i/o): fwcfg.ctl
      00000000f0000512-00000000f0000512 (prio 0, i/o): fwcfg.data
      00000000fff00000-00000000ffffffff (prio 0, rom): bios

We can see it in the monitor, before/after this patch, the
flatview rendering is left unchanged (note the unassigned
80c0.0000-bf7f.ffff range):

  (qemu) info mtree  -f
  FlatView #1
   AS "memory", root: system
   AS "cpu-memory-0", root: system
   Root memory region: system
    0000000000000000-0000000001ffffff (prio 0, ram): simm.0
    0000000002000000-0000000003ffffff (prio 1, ram): simm.1
    0000000004000000-0000000005ffffff (prio 2, ram): simm.2
    0000000006000000-0000000007ffffff (prio 3, ram): simm.3
    0000000080000000-00000000807fffff (prio 1, i/o): pci-io-non-contiguous
    0000000080800000-0000000080bfffff (prio 0, i/o): pciio
    00000000bfffeff0-00000000bfffeff3 (prio 0, i/o): ppc-parity
    00000000bffffff0-00000000bffffff0 (prio 0, i/o): pci-intack
    00000000c00a0000-00000000c00bffff (prio 1, i/o): vga-lowmem
    00000000f0000510-00000000f0000511 (prio 0, i/o): fwcfg.ctl
    00000000f0000512-00000000f0000512 (prio 0, i/o): fwcfg.data
    00000000fff00000-00000000ffffffff (prio 0, rom): bios

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
RFC because I can't justify the unassigned 80c0.0000-bf7f.ffff range.
---
 hw/pci-host/raven.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index 7bbb7ef0a29..f9c92b83770 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -67,7 +67,6 @@ struct PRePPCIState {
     PCIBus pci_bus;
     AddressSpace pci_io_as;
     MemoryRegion pci_io;
-    MemoryRegion pci_io_alias;
     MemoryRegion pci_io_non_contiguous;
     MemoryRegion pci_memory;
     MemoryRegion pci_intack;
@@ -299,8 +298,6 @@ static void raven_pcihost_initfn(Object *obj)
     DeviceState *pci_dev;
 
     memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
-    memory_region_init_alias(&s->pci_io_alias, obj, "pci-io",
-                             &s->pci_io, 0, memory_region_size(&s->pci_io));
     memory_region_init_io(&s->pci_io_non_contiguous, obj, &raven_io_ops, s,
                           "pci-io-non-contiguous", 0x00800000);
     memory_region_init(&s->pci_memory, obj, "pci-memory", 0x3f000000);
@@ -308,9 +305,7 @@ static void raven_pcihost_initfn(Object *obj)
 
     /* CPU address space */
     memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
-                                &s->pci_io_alias);
-    memory_region_add_subregion_overlap(address_space_mem, PCI_IO_BASE_ADDR,
-                                        &s->pci_io_non_contiguous, 1);
+                                &s->pci_io_non_contiguous);
     memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
     pci_root_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), NULL,
                              &s->pci_memory, &s->pci_io, 0, TYPE_PCI_BUS);
-- 
2.26.3

[PATCH v2 09/11] hw/pci-host/prep: Do not directly map bus-master region onto main bus

[Philippe Mathieu-Daudé]

The raven bus-master memory region is exposed as an AddressSpace.
AddressSpaces root MemoryRegion must not be mapped into other
MemoryRegion, therefore map the region using its alias.

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

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index f9c92b83770..d8c1aaa11f5 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -307,8 +307,6 @@ static void raven_pcihost_initfn(Object *obj)
     memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
                                 &s->pci_io_non_contiguous);
     memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
-    pci_root_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), NULL,
-                             &s->pci_memory, &s->pci_io, 0, TYPE_PCI_BUS);
 
     /* Bus master address space */
     memory_region_init(&s->bm, obj, "bm-raven", 4 * GiB);
@@ -320,6 +318,10 @@ static void raven_pcihost_initfn(Object *obj)
     memory_region_add_subregion(&s->bm, 0         , &s->bm_pci_memory_alias);
     memory_region_add_subregion(&s->bm, 0x80000000, &s->bm_ram_alias);
     address_space_init(&s->bm_as, &s->bm, "raven-bm");
+
+    pci_root_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), NULL,
+                             &s->bm_pci_memory_alias, &s->pci_io, 0,
+                             TYPE_PCI_BUS);
     pci_setup_iommu(&s->pci_bus, raven_pcihost_set_iommu, s);
 
     h->bus = &s->pci_bus;
-- 
2.26.3

[PATCH v2 10/11] memory: Make sure root MR won't be added as subregion

[Philippe Mathieu-Daudé]

From: Peter Xu <peterx@redhat.com>

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 5728a681b27..83ac86525b2 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 d4493ef9e43..409bcaec7f5 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -2443,6 +2443,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);
@@ -2820,6 +2821,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.3

[PATCH v2 11/11] hw/pci-host/raven: Remove temporary assertion 'root MR is zero-based'

[Philippe Mathieu-Daudé]

Previous commit added a check in memory_region_add_subregion_common()
to ensure AS root MR can't be added as subregion (changing the MR
base address doing so). We can now remove the temporary assert in
the raven model.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci-host/raven.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index d8c1aaa11f5..3f8508bd467 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -141,16 +141,6 @@ static const MemoryRegionOps raven_intack_ops = {
 static inline hwaddr raven_io_address(PREPPCIState *s,
                                       hwaddr addr)
 {
-    /*
-     * We shouldn't access AddressSpace internals. However this assert
-     * is temporarily used to prove a subtle inconsistency from commit
-     * 1ae1dc5ba24 ("raven: Set a correct PCI I/O memory region") which
-     * expected the PCI I/O root region base address to be 0x80000000.
-     *
-     * We now use an alias memory region as root, which is zero-based.
-     */
-    assert(s->pci_io_as.root->addr == 0);
-
     if (s->contiguous_map == 0) {
         /* 64 KB contiguous space for IOs */
         addr &= 0xFFFF;
-- 
2.26.3

Re: [PATCH v2 00/11] memory: Forbid mapping AddressSpace root MemoryRegion

[Cédric Le Goater]

Hello,

On 4/17/21 12:30 PM, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> This series is the result of a long thread with Peter:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg788366.html
> and IRC chats...
> 
> AddressSpace are physical address view and shouldn't be using
> non-zero base address. The correct way to map a MR used as AS
> root is to use a MR alias.
> 
> Fix the current incorrect uses, then forbid further use.
> 
> Since v1:
> - Split the Raven patch in multiple changes, easier to follow/review
>   (https://www.mail-archive.com/qemu-devel@nongnu.org/msg791116.html)
> 
> Note, the Aspeed patches are already queued in Cédric tree. I had
> to cherry-pick them from his tree to have the series pass CI.

So I will wait for this patchset to be merged before sending the 
aspeed queue. Are there any blocking aspects to it ? 

Thanks,

C.  

Re: [PATCH v2 00/11] memory: Forbid mapping AddressSpace root MemoryRegion

[Philippe Mathieu-Daudé]

On 4/19/21 9:17 AM, Cédric Le Goater wrote:
> On 4/17/21 12:30 PM, Philippe Mathieu-Daudé wrote:
>> This series is the result of a long thread with Peter:
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg788366.html
>> and IRC chats...
>>
>> AddressSpace are physical address view and shouldn't be using
>> non-zero base address. The correct way to map a MR used as AS
>> root is to use a MR alias.
>>
>> Fix the current incorrect uses, then forbid further use.

>>
>> Note, the Aspeed patches are already queued in Cédric tree. I had
>> to cherry-pick them from his tree to have the series pass CI.
> 
> So I will wait for this patchset to be merged before sending the 
> aspeed queue. Are there any blocking aspects to it ? 

I think it is easier for both of us the other way around ;) Your
aspeed queue being merged first. Nothing from mine blocks yours,
and mine depends of a maintainer willing to take it, which has yet
to happen :)

Regards,

Phil.

Re: [PATCH v2 00/11] memory: Forbid mapping AddressSpace root MemoryRegion

[Peter Xu]

On Sat, Apr 17, 2021 at 12:30:17PM +0200, Philippe Mathieu-Daudé wrote:
> AddressSpace are physical address view and shouldn't be using
> non-zero base address. The correct way to map a MR used as AS
> root is to use a MR alias.

Today when I rethink this, I figured another way (maybe easier?) to fix the
issue.

The major problem so far we had is that mr->addr can be anything for a root mr
if it's added as subregion of another mr.

E.g. in current implementation of mtree_print_mr() MR.addr is constantly used
as an offset value:

    cur_start = base + mr->addr;

However afaict mr->addr is defined as "relative offset of this mr to its
container", as in memory_region_add_subregion_common().  Say, mr->addr is
undefined from that pov if mr->container==NULL, as this MR belongs to nobody.
And if it's defined, it's only meaning is in its container's context (or say,
address space) only.

That said, when we do mtree_print_mr(), another solution could be as simple as,
not referencing mr->addr if we _know_ we're working on the root mr, as this is
definitely _not_ in the context of the mr's container, even if it has one
container after all:

---8<---
diff --git a/softmmu/memory.c b/softmmu/memory.c
index d4493ef9e43..d71fb8ecc89 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -2940,7 +2940,7 @@ static void mtree_print_mr(const MemoryRegion *mr, unsigned int level,
         return;
     }
 
-    cur_start = base + mr->addr;
+    cur_start = base + (level == 1) ? 0 : mr->addr;
     cur_end = cur_start + MR_SIZE(mr->size);
 
     /*
---8<---

Phil, do you think it'll work too to fix the strange offset value dumped in
"info mtree"?

I don't know (even if it works, perhaps I've missed something) which is better,
as current series seems cleaner, then any mr will either belong to a AS or a MR
(never both!), but just raise it up.

Thanks,

--
Peter Xu