[PATCH 0/5] memory: Forbid mapping AddressSpace root MemoryRegion

[PATCH 0/5] 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

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.

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

Philippe Mathieu-Daudé (4):
  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/prep: Remove unuseful memory region mapping
  hw/pci-host/prep: Do not directly map bus-master region onto main bus

 include/exec/memory.h       |  1 +
 include/hw/ssi/aspeed_smc.h |  1 +
 hw/arm/aspeed.c             |  6 +++++-
 hw/pci-host/prep.c          | 17 ++++++++---------
 hw/ssi/aspeed_smc.c         |  4 +++-
 softmmu/memory.c            |  2 ++
 6 files changed, 20 insertions(+), 11 deletions(-)

-- 
2.26.2

[PATCH 1/5] 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.2

Re: [PATCH 1/5] hw/arm/aspeed: Do not directly map ram container onto main address bus

[Cédric Le Goater]

On 3/12/21 7:28 PM, Philippe Mathieu-Daudé wrote:
> 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);
> 

RAM is now initialized before the SoC. So we should be able to use 
machine->ram instead of the bmc->ram_container MR and simplify the
Aspeed SMC model below the SoC.

Thanks,

C.

[PATCH 2/5] 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>
---
 include/hw/ssi/aspeed_smc.h | 1 +
 hw/ssi/aspeed_smc.c         | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

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 16addee4dc8..aa26578bdac 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -1386,7 +1386,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.2

Re: [PATCH 2/5] hw/arm/aspeed: Do not sysbus-map mmio flash region directly, use alias

[Philippe Mathieu-Daudé]

Incorrect subject prefix, should be "hw/ssi/aspeed_smc"

On 3/12/21 7:28 PM, Philippe Mathieu-Daudé wrote:
> 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>
> ---
>  include/hw/ssi/aspeed_smc.h | 1 +
>  hw/ssi/aspeed_smc.c         | 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> 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 16addee4dc8..aa26578bdac 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -1386,7 +1386,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);

Re: [PATCH 2/5] hw/arm/aspeed: Do not sysbus-map mmio flash region directly, use alias

[Cédric Le Goater]

On 3/13/21 1:05 PM, Philippe Mathieu-Daudé wrote:
> Incorrect subject prefix, should be "hw/ssi/aspeed_smc"

Is this just good practice or something that was agreed upon ? 
We should update checkpatch if so.

Thanks,

C. 


> 
> On 3/12/21 7:28 PM, Philippe Mathieu-Daudé wrote:
>> 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>
>> ---
>>  include/hw/ssi/aspeed_smc.h | 1 +
>>  hw/ssi/aspeed_smc.c         | 4 +++-
>>  2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> 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 16addee4dc8..aa26578bdac 100644
>> --- a/hw/ssi/aspeed_smc.c
>> +++ b/hw/ssi/aspeed_smc.c
>> @@ -1386,7 +1386,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);

Re: [PATCH 2/5] hw/arm/aspeed: Do not sysbus-map mmio flash region directly, use alias

[Philippe Mathieu-Daudé]

On 3/17/21 7:46 PM, Cédric Le Goater wrote:
> On 3/13/21 1:05 PM, Philippe Mathieu-Daudé wrote:
>> Incorrect subject prefix, should be "hw/ssi/aspeed_smc"
> 
> Is this just good practice or something that was agreed upon ? 

Not sure, maybe "good practice"? Anyway here I only meant to
correct my own patch without respining the series just for this.

> We should update checkpatch if so.

Certainly a waste of time =)

> 
> Thanks,
> 
> C. 

Re: [PATCH 2/5] hw/arm/aspeed: Do not sysbus-map mmio flash region directly, use alias

[Cédric Le Goater]

On 3/12/21 7:28 PM, Philippe Mathieu-Daudé wrote:
> 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>

That does the trick but you need an extra change in the model. 

The fixes are in my aspeed-6.0 branch on GH and they survive the last
patch of your series :

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

I will upstream for 6.1.

Thanks,

C. 

> ---
>  include/hw/ssi/aspeed_smc.h | 1 +
>  hw/ssi/aspeed_smc.c         | 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> 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 16addee4dc8..aa26578bdac 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -1386,7 +1386,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);
>  
> 

Re: [PATCH 2/5] hw/arm/aspeed: Do not sysbus-map mmio flash region directly, use alias

[Philippe Mathieu-Daudé]

On 3/17/21 7:30 PM, Cédric Le Goater wrote:
> On 3/12/21 7:28 PM, Philippe Mathieu-Daudé wrote:
>> 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>
> 
> That does the trick but you need an extra change in the model. 
> 
> The fixes are in my aspeed-6.0 branch on GH and they survive the last
> patch of your series :
> 
>   [PATCH 5/5] memory: Make sure root MR won't be added as subregion

I wondered about changing DMA_FLASH_ADDR() wasn't sure the tests
would use the flash.

> I will upstream for 6.1.

Thanks!

Phil.

Re: [PATCH 2/5] hw/arm/aspeed: Do not sysbus-map mmio flash region directly, use alias

[Cédric Le Goater]

On 3/17/21 8:00 PM, Philippe Mathieu-Daudé wrote:
> On 3/17/21 7:30 PM, Cédric Le Goater wrote:
>> On 3/12/21 7:28 PM, Philippe Mathieu-Daudé wrote:
>>> 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>
>>
>> That does the trick but you need an extra change in the model. 
>>
>> The fixes are in my aspeed-6.0 branch on GH and they survive the last
>> patch of your series :
>>
>>   [PATCH 5/5] memory: Make sure root MR won't be added as subregion
> 
> I wondered about changing DMA_FLASH_ADDR() wasn't sure the tests
> would use the flash.

The acceptance tests (not merged yet) download firmware images
in which u-boot does DMA accesses to calibrate the reads on the
flash device. 

C.
  
> 
>> I will upstream for 6.1.
> 
> Thanks!
> 
> Phil.
> 

[PATCH 3/5] hw/pci-host/prep: Remove unuseful memory region mapping

[Philippe Mathieu-Daudé]

The pci_io_non_contiguous region is mapped on top of pci_io
with higher priority, but simply dispatch into this region
address space. Simplify by directly registering the former
region in place, and adapt the address space dispatch offsets.

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

diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
index 0a9162fba97..00a28c2d18c 100644
--- a/hw/pci-host/prep.c
+++ b/hw/pci-host/prep.c
@@ -159,8 +159,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,
-                       MEMTXATTRS_UNSPECIFIED, buf, size);
+    address_space_read(&s->pci_io_as, addr, MEMTXATTRS_UNSPECIFIED, buf, size);
 
     if (size == 1) {
         return buf[0];
@@ -191,8 +190,7 @@ static void raven_io_write(void *opaque, hwaddr addr,
         g_assert_not_reached();
     }
 
-    address_space_write(&s->pci_io_as, addr + 0x80000000,
-                        MEMTXATTRS_UNSPECIFIED, buf, size);
+    address_space_write(&s->pci_io_as, addr, MEMTXATTRS_UNSPECIFIED, buf, size);
 }
 
 static const MemoryRegionOps raven_io_ops = {
@@ -294,9 +292,8 @@ 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,
-                                        &s->pci_io_non_contiguous, 1);
+    memory_region_add_subregion(address_space_mem, 0x80000000,
+                                &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.2

Re: [PATCH 3/5] hw/pci-host/prep: Remove unuseful memory region mapping

[Peter Xu]

On Fri, Mar 12, 2021 at 07:28:49PM +0100, Philippe Mathieu-Daudé wrote:
> The pci_io_non_contiguous region is mapped on top of pci_io
> with higher priority, but simply dispatch into this region
> address space. Simplify by directly registering the former
> region in place, and adapt the address space dispatch offsets.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/pci-host/prep.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> index 0a9162fba97..00a28c2d18c 100644
> --- a/hw/pci-host/prep.c
> +++ b/hw/pci-host/prep.c
> @@ -159,8 +159,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,
> -                       MEMTXATTRS_UNSPECIFIED, buf, size);
> +    address_space_read(&s->pci_io_as, addr, MEMTXATTRS_UNSPECIFIED, buf, size);
>  
>      if (size == 1) {
>          return buf[0];
> @@ -191,8 +190,7 @@ static void raven_io_write(void *opaque, hwaddr addr,
>          g_assert_not_reached();
>      }
>  
> -    address_space_write(&s->pci_io_as, addr + 0x80000000,
> -                        MEMTXATTRS_UNSPECIFIED, buf, size);
> +    address_space_write(&s->pci_io_as, addr, MEMTXATTRS_UNSPECIFIED, buf, size);

This changes access to s->pci_io_as, but below didn't change s->pci_io_as
layout at all (below is about address_space_mem).  Is this intended?

>  }
>  
>  static const MemoryRegionOps raven_io_ops = {
> @@ -294,9 +292,8 @@ 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,
> -                                        &s->pci_io_non_contiguous, 1);
> +    memory_region_add_subregion(address_space_mem, 0x80000000,
> +                                &s->pci_io_non_contiguous);

I don't know any of this code at all... but it seems the two memory regions are
not identical in size:

    memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
    memory_region_init_io(&s->pci_io_non_contiguous, obj, &raven_io_ops, s,
                          "pci-io-non-contiguous", 0x00800000);

Then it seems the memory access dispatching to (0x00800000, 0x3f800000) would
change too, from s->pci_io to nothing.  Raise this up too since I don't know
either whether it's intended..

>      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.2
> 

-- 
Peter Xu

Re: [PATCH 3/5] hw/pci-host/prep: Remove unuseful memory region mapping

[David Gibson]

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

On Fri, Mar 12, 2021 at 03:38:21PM -0500, Peter Xu wrote:
> On Fri, Mar 12, 2021 at 07:28:49PM +0100, Philippe Mathieu-Daudé wrote:
> > The pci_io_non_contiguous region is mapped on top of pci_io
> > with higher priority, but simply dispatch into this region
> > address space. Simplify by directly registering the former
> > region in place, and adapt the address space dispatch offsets.
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >  hw/pci-host/prep.c | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> > index 0a9162fba97..00a28c2d18c 100644
> > --- a/hw/pci-host/prep.c
> > +++ b/hw/pci-host/prep.c
> > @@ -159,8 +159,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,
> > -                       MEMTXATTRS_UNSPECIFIED, buf, size);
> > +    address_space_read(&s->pci_io_as, addr, MEMTXATTRS_UNSPECIFIED, buf, size);
> >  
> >      if (size == 1) {
> >          return buf[0];
> > @@ -191,8 +190,7 @@ static void raven_io_write(void *opaque, hwaddr addr,
> >          g_assert_not_reached();
> >      }
> >  
> > -    address_space_write(&s->pci_io_as, addr + 0x80000000,
> > -                        MEMTXATTRS_UNSPECIFIED, buf, size);
> > +    address_space_write(&s->pci_io_as, addr, MEMTXATTRS_UNSPECIFIED, buf, size);
> 
> This changes access to s->pci_io_as, but below didn't change s->pci_io_as
> layout at all (below is about address_space_mem).  Is this intended?
> 
> >  }
> >  
> >  static const MemoryRegionOps raven_io_ops = {
> > @@ -294,9 +292,8 @@ 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,
> > -                                        &s->pci_io_non_contiguous, 1);
> > +    memory_region_add_subregion(address_space_mem, 0x80000000,
> > +                                &s->pci_io_non_contiguous);
> 
> I don't know any of this code at all... but it seems the two memory regions are
> not identical in size:
> 
>     memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
>     memory_region_init_io(&s->pci_io_non_contiguous, obj, &raven_io_ops, s,
>                           "pci-io-non-contiguous", 0x00800000);
> 
> Then it seems the memory access dispatching to (0x00800000, 0x3f800000) would
> change too, from s->pci_io to nothing.  Raise this up too since I don't know
> either whether it's intended..

Right, it seems like this removes the mapping of s->pci_io entirely.

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

-- 
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 --]

Re: [PATCH 3/5] hw/pci-host/prep: Remove unuseful memory region mapping

[Philippe Mathieu-Daudé]

On 3/22/21 5:09 AM, David Gibson wrote:
> On Fri, Mar 12, 2021 at 03:38:21PM -0500, Peter Xu wrote:
>> On Fri, Mar 12, 2021 at 07:28:49PM +0100, Philippe Mathieu-Daudé wrote:
>>> The pci_io_non_contiguous region is mapped on top of pci_io
>>> with higher priority, but simply dispatch into this region
>>> address space. Simplify by directly registering the former
>>> region in place, and adapt the address space dispatch offsets.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  hw/pci-host/prep.c | 11 ++++-------
>>>  1 file changed, 4 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
>>> index 0a9162fba97..00a28c2d18c 100644
>>> --- a/hw/pci-host/prep.c
>>> +++ b/hw/pci-host/prep.c
>>> @@ -159,8 +159,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,
>>> -                       MEMTXATTRS_UNSPECIFIED, buf, size);
>>> +    address_space_read(&s->pci_io_as, addr, MEMTXATTRS_UNSPECIFIED, buf, size);
>>>  
>>>      if (size == 1) {
>>>          return buf[0];
>>> @@ -191,8 +190,7 @@ static void raven_io_write(void *opaque, hwaddr addr,
>>>          g_assert_not_reached();
>>>      }
>>>  
>>> -    address_space_write(&s->pci_io_as, addr + 0x80000000,
>>> -                        MEMTXATTRS_UNSPECIFIED, buf, size);
>>> +    address_space_write(&s->pci_io_as, addr, MEMTXATTRS_UNSPECIFIED, buf, size);
>>
>> This changes access to s->pci_io_as, but below didn't change s->pci_io_as
>> layout at all (below is about address_space_mem).  Is this intended?
>>
>>>  }
>>>  
>>>  static const MemoryRegionOps raven_io_ops = {
>>> @@ -294,9 +292,8 @@ 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,
>>> -                                        &s->pci_io_non_contiguous, 1);
>>> +    memory_region_add_subregion(address_space_mem, 0x80000000,
>>> +                                &s->pci_io_non_contiguous);
>>
>> I don't know any of this code at all... but it seems the two memory regions are
>> not identical in size:
>>
>>     memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
>>     memory_region_init_io(&s->pci_io_non_contiguous, obj, &raven_io_ops, s,
>>                           "pci-io-non-contiguous", 0x00800000);
>>
>> Then it seems the memory access dispatching to (0x00800000, 0x3f800000) would
>> change too, from s->pci_io to nothing.  Raise this up too since I don't know
>> either whether it's intended..
> 
> Right, it seems like this removes the mapping of s->pci_io entirely.

Yes, this is on purpose. The dispatching is done via raven_io_ops:

    address_space_init(&s->pci_io_as, &s->pci_io, "raven-io");

raven_io_ops uses raven_io_address() which returns an address in
"pci-io" range if < 0x800000, else it returns io_mem_unassigned
because the address is outside the pci_io_as AddressSpace.

Note, the flatview is unchanged with this patch.

My take here is 1/ I squashed too much changes and 2/ I didn't
documented enough. I'll respin improved.

Thanks for your reviews!

Phil.

[PATCH 4/5] 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/prep.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
index 00a28c2d18c..6eaf9242bd8 100644
--- a/hw/pci-host/prep.c
+++ b/hw/pci-host/prep.c
@@ -295,8 +295,6 @@ static void raven_pcihost_initfn(Object *obj)
     memory_region_add_subregion(address_space_mem, 0x80000000,
                                 &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);
@@ -308,6 +306,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.2

Re: [PATCH 4/5] hw/pci-host/prep: Do not directly map bus-master region onto main bus

[David Gibson]

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

On Fri, Mar 12, 2021 at 07:28:50PM +0100, Philippe Mathieu-Daudé wrote:
> 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>

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

> ---
>  hw/pci-host/prep.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> index 00a28c2d18c..6eaf9242bd8 100644
> --- a/hw/pci-host/prep.c
> +++ b/hw/pci-host/prep.c
> @@ -295,8 +295,6 @@ static void raven_pcihost_initfn(Object *obj)
>      memory_region_add_subregion(address_space_mem, 0x80000000,
>                                  &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);
> @@ -308,6 +306,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;

-- 
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 5/5] 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 54ccf1a5f09..8137ad3a9f6 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 9db47b7db6b..ce322ff3d6e 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 5/5] memory: Make sure root MR won't be added as subregion

[Philippe Mathieu-Daudé]

Hi Peter,

On 3/12/21 7:28 PM, Philippe Mathieu-Daudé wrote:
> 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 54ccf1a5f09..8137ad3a9f6 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 9db47b7db6b..ce322ff3d6e 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);

Not all containers have to be backed by AddressSpace.

This check is too strict IMO.

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