[PATCH 0/8] Simplify memory_region_add_subregion_overlap(..., priority=0)

[PATCH 0/8] Simplify memory_region_add_subregion_overlap(..., priority=0)

[Philippe Mathieu-Daudé]

Hi,

In this series we use coccinelle to replace:
- memory_region_add_subregion_overlap(..., priority=0)
+ memory_region_add_subregion(...)

Rationale is the code is easier to read, and reviewers don't
have to worry about overlapping because it isn't used.

Last patch is a minor cleanup in variable names.

I expect each subsystem maintainer to take the subsystem patches.

Regards,

Phil.

Philippe Mathieu-Daudé (8):
  hw/arm/nrf51_soc: Use memory_region_add_subregion() when priority is 0
  hw/arm/raspi: Use memory_region_add_subregion() when priority is 0
  hw/arm/xlnx-versal: Use memory_region_add_subregion() when priority is
    0
  hw/i386/intel_iommu: Use memory_region_add_subregion when priority is
    0
  hw/mips/boston: Use memory_region_add_subregion() when priority is 0
  hw/vfio/pci: Use memory_region_add_subregion() when priority is 0
  target/i386: Use memory_region_add_subregion() when priority is 0
  target/i386/cpu: Use 'mr' for MemoryRegion variables

 target/i386/cpu.h            |  2 +-
 hw/arm/bcm2835_peripherals.c |  4 ++--
 hw/arm/nrf51_soc.c           | 14 +++++++-------
 hw/arm/raspi.c               |  2 +-
 hw/arm/xlnx-versal-virt.c    |  3 +--
 hw/arm/xlnx-versal.c         |  4 ++--
 hw/i386/intel_iommu.c        | 11 ++++-------
 hw/mips/boston.c             | 14 +++++++-------
 hw/vfio/pci.c                |  3 +--
 target/i386/cpu.c            | 18 +++++++++---------
 target/i386/kvm.c            |  2 +-
 11 files changed, 36 insertions(+), 41 deletions(-)

-- 
2.21.0

[PATCH 1/8] hw/arm/nrf51_soc: Use memory_region_add_subregion() when priority is 0

[Philippe Mathieu-Daudé]

It is pointless to overlap a memory subregion with priority 0.
Use the simpler memory_region_add_subregion() function.

This patch was produced with the following spatch script:

    @@
    expression region;
    expression offset;
    expression subregion;
    @@
    -memory_region_add_subregion_overlap(region, offset, subregion, 0)
    +memory_region_add_subregion(region, offset, subregion)

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/arm/nrf51_soc.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 74029169d0..ade06b225f 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -94,7 +94,7 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
         return;
     }
     mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->uart), 0);
-    memory_region_add_subregion_overlap(&s->container, NRF51_UART_BASE, mr, 0);
+    memory_region_add_subregion(&s->container, NRF51_UART_BASE, mr);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart), 0,
                        qdev_get_gpio_in(DEVICE(&s->cpu),
                        BASE_TO_IRQ(NRF51_UART_BASE)));
@@ -107,7 +107,7 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
     }
 
     mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->rng), 0);
-    memory_region_add_subregion_overlap(&s->container, NRF51_RNG_BASE, mr, 0);
+    memory_region_add_subregion(&s->container, NRF51_RNG_BASE, mr);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->rng), 0,
                        qdev_get_gpio_in(DEVICE(&s->cpu),
                        BASE_TO_IRQ(NRF51_RNG_BASE)));
@@ -127,13 +127,13 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
     }
 
     mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->nvm), 0);
-    memory_region_add_subregion_overlap(&s->container, NRF51_NVMC_BASE, mr, 0);
+    memory_region_add_subregion(&s->container, NRF51_NVMC_BASE, mr);
     mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->nvm), 1);
-    memory_region_add_subregion_overlap(&s->container, NRF51_FICR_BASE, mr, 0);
+    memory_region_add_subregion(&s->container, NRF51_FICR_BASE, mr);
     mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->nvm), 2);
-    memory_region_add_subregion_overlap(&s->container, NRF51_UICR_BASE, mr, 0);
+    memory_region_add_subregion(&s->container, NRF51_UICR_BASE, mr);
     mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->nvm), 3);
-    memory_region_add_subregion_overlap(&s->container, NRF51_FLASH_BASE, mr, 0);
+    memory_region_add_subregion(&s->container, NRF51_FLASH_BASE, mr);
 
     /* GPIO */
     object_property_set_bool(OBJECT(&s->gpio), true, "realized", &err);
@@ -143,7 +143,7 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
     }
 
     mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->gpio), 0);
-    memory_region_add_subregion_overlap(&s->container, NRF51_GPIO_BASE, mr, 0);
+    memory_region_add_subregion(&s->container, NRF51_GPIO_BASE, mr);
 
     /* Pass all GPIOs to the SOC layer so they are available to the board */
     qdev_pass_gpios(DEVICE(&s->gpio), dev_soc, NULL);
-- 
2.21.0

[PATCH 2/8] hw/arm/raspi: Use memory_region_add_subregion() when priority is 0

[Philippe Mathieu-Daudé]

It is pointless to overlap a memory subregion with priority 0.
Use the simpler memory_region_add_subregion() function.

This patch was produced with the following spatch script:

    @@
    expression region;
    expression offset;
    expression subregion;
    @@
    -memory_region_add_subregion_overlap(region, offset, subregion, 0)
    +memory_region_add_subregion(region, offset, subregion)

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/arm/bcm2835_peripherals.c | 4 ++--
 hw/arm/raspi.c               | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
index 17207ae07e..f792bd6bb1 100644
--- a/hw/arm/bcm2835_peripherals.c
+++ b/hw/arm/bcm2835_peripherals.c
@@ -160,8 +160,8 @@ static void bcm2835_peripherals_realize(DeviceState *dev, Error **errp)
     for (n = 0; n < 4; n++) {
         memory_region_init_alias(&s->ram_alias[n], OBJECT(s),
                                  "bcm2835-gpu-ram-alias[*]", ram, 0, ram_size);
-        memory_region_add_subregion_overlap(&s->gpu_bus_mr, (hwaddr)n << 30,
-                                            &s->ram_alias[n], 0);
+        memory_region_add_subregion(&s->gpu_bus_mr, (hwaddr)n << 30,
+                                    &s->ram_alias[n]);
     }
 
     /* Interrupt Controller */
diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 6a510aafc1..3649b75449 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -187,7 +187,7 @@ static void raspi_init(MachineState *machine, int version)
     memory_region_allocate_system_memory(&s->ram, OBJECT(machine), "ram",
                                          machine->ram_size);
     /* FIXME: Remove when we have custom CPU address space support */
-    memory_region_add_subregion_overlap(get_system_memory(), 0, &s->ram, 0);
+    memory_region_add_subregion(get_system_memory(), 0, &s->ram);
 
     /* Setup the SOC */
     object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s->ram),
-- 
2.21.0

[PATCH 3/8] hw/arm/xlnx-versal: Use memory_region_add_subregion() when priority is 0

[Philippe Mathieu-Daudé]

It is pointless to overlap a memory subregion with priority 0.
Use the simpler memory_region_add_subregion() function.

This patch was produced with the following spatch script:

    @@
    expression region;
    expression offset;
    expression subregion;
    @@
    -memory_region_add_subregion_overlap(region, offset, subregion, 0)
    +memory_region_add_subregion(region, offset, subregion)

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/arm/xlnx-versal-virt.c | 3 +--
 hw/arm/xlnx-versal.c      | 4 ++--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
index 462493c467..901e9ed86c 100644
--- a/hw/arm/xlnx-versal-virt.c
+++ b/hw/arm/xlnx-versal-virt.c
@@ -437,8 +437,7 @@ static void versal_virt_init(MachineState *machine)
 
     /* Make the APU cpu address space visible to virtio and other
      * modules unaware of muliple address-spaces.  */
-    memory_region_add_subregion_overlap(get_system_memory(),
-                                        0, &s->soc.fpd.apu.mr, 0);
+    memory_region_add_subregion(get_system_memory(), 0, &s->soc.fpd.apu.mr);
 
     s->binfo.ram_size = machine->ram_size;
     s->binfo.loader_start = 0x0;
diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
index 8b3d8d85b8..538d907f8a 100644
--- a/hw/arm/xlnx-versal.c
+++ b/hw/arm/xlnx-versal.c
@@ -281,8 +281,8 @@ static void versal_realize(DeviceState *dev, Error **errp)
     memory_region_init_ram(&s->lpd.mr_ocm, OBJECT(s), "ocm",
                            MM_OCM_SIZE, &error_fatal);
 
-    memory_region_add_subregion_overlap(&s->mr_ps, MM_OCM, &s->lpd.mr_ocm, 0);
-    memory_region_add_subregion_overlap(&s->fpd.apu.mr, 0, &s->mr_ps, 0);
+    memory_region_add_subregion(&s->mr_ps, MM_OCM, &s->lpd.mr_ocm);
+    memory_region_add_subregion(&s->fpd.apu.mr, 0, &s->mr_ps);
 }
 
 static void versal_init(Object *obj)
-- 
2.21.0

[PATCH 4/8] hw/i386/intel_iommu: Use memory_region_add_subregion when priority is 0

[Philippe Mathieu-Daudé]

It is pointless to overlap a memory subregion with priority 0.
Use the simpler memory_region_add_subregion() function.

This patch was produced with the following spatch script:

    @@
    expression region;
    expression offset;
    expression subregion;
    @@
    -memory_region_add_subregion_overlap(region, offset, subregion, 0)
    +memory_region_add_subregion(region, offset, subregion)

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/intel_iommu.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 43c94b993b..afa7e07b05 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3363,11 +3363,9 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
          * switch between DMAR & noDMAR by enable/disable
          * corresponding sub-containers
          */
-        memory_region_add_subregion_overlap(&vtd_dev_as->root, 0,
-                                            MEMORY_REGION(&vtd_dev_as->iommu),
-                                            0);
-        memory_region_add_subregion_overlap(&vtd_dev_as->root, 0,
-                                            &vtd_dev_as->nodmar, 0);
+        memory_region_add_subregion(&vtd_dev_as->root, 0,
+                                    MEMORY_REGION(&vtd_dev_as->iommu));
+        memory_region_add_subregion(&vtd_dev_as->root, 0, &vtd_dev_as->nodmar);
 
         vtd_switch_address_space(vtd_dev_as);
     }
@@ -3764,8 +3762,7 @@ static void vtd_realize(DeviceState *dev, Error **errp)
     memory_region_init_alias(&s->mr_sys_alias, OBJECT(s),
                              "vtd-sys-alias", get_system_memory(), 0,
                              memory_region_size(get_system_memory()));
-    memory_region_add_subregion_overlap(&s->mr_nodmar, 0,
-                                        &s->mr_sys_alias, 0);
+    memory_region_add_subregion(&s->mr_nodmar, 0, &s->mr_sys_alias);
     memory_region_add_subregion_overlap(&s->mr_nodmar,
                                         VTD_INTERRUPT_ADDR_FIRST,
                                         &s->mr_ir, 1);
-- 
2.21.0

[PATCH 5/8] hw/mips/boston: Use memory_region_add_subregion() when priority is 0

[Philippe Mathieu-Daudé]

It is pointless to overlap a memory subregion with priority 0.
Use the simpler memory_region_add_subregion() function.

This patch was produced with the following spatch script:

    @@
    expression region;
    expression offset;
    expression subregion;
    @@
    -memory_region_add_subregion_overlap(region, offset, subregion, 0)
    +memory_region_add_subregion(region, offset, subregion)

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/mips/boston.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index ca7d813a52..a27258b4d1 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -412,10 +412,10 @@ xilinx_pcie_init(MemoryRegion *sys_mem, uint32_t bus_nr,
     qdev_init_nofail(dev);
 
     cfg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
-    memory_region_add_subregion_overlap(sys_mem, cfg_base, cfg, 0);
+    memory_region_add_subregion(sys_mem, cfg_base, cfg);
 
     mmio = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
-    memory_region_add_subregion_overlap(sys_mem, 0, mmio, 0);
+    memory_region_add_subregion(sys_mem, 0, mmio);
 
     qdev_connect_gpio_out_named(dev, "interrupt_out", 0, irq);
 
@@ -471,17 +471,17 @@ static void boston_mach_init(MachineState *machine)
 
     flash =  g_new(MemoryRegion, 1);
     memory_region_init_rom(flash, NULL, "boston.flash", 128 * MiB, &err);
-    memory_region_add_subregion_overlap(sys_mem, 0x18000000, flash, 0);
+    memory_region_add_subregion(sys_mem, 0x18000000, flash);
 
     ddr = g_new(MemoryRegion, 1);
     memory_region_allocate_system_memory(ddr, NULL, "boston.ddr",
                                          machine->ram_size);
-    memory_region_add_subregion_overlap(sys_mem, 0x80000000, ddr, 0);
+    memory_region_add_subregion(sys_mem, 0x80000000, ddr);
 
     ddr_low_alias = g_new(MemoryRegion, 1);
     memory_region_init_alias(ddr_low_alias, NULL, "boston_low.ddr",
                              ddr, 0, MIN(machine->ram_size, (256 * MiB)));
-    memory_region_add_subregion_overlap(sys_mem, 0, ddr_low_alias, 0);
+    memory_region_add_subregion(sys_mem, 0, ddr_low_alias);
 
     xilinx_pcie_init(sys_mem, 0,
                      0x10000000, 32 * MiB,
@@ -501,7 +501,7 @@ static void boston_mach_init(MachineState *machine)
     platreg = g_new(MemoryRegion, 1);
     memory_region_init_io(platreg, NULL, &boston_platreg_ops, s,
                           "boston-platregs", 0x1000);
-    memory_region_add_subregion_overlap(sys_mem, 0x17ffd000, platreg, 0);
+    memory_region_add_subregion(sys_mem, 0x17ffd000, platreg);
 
     s->uart = serial_mm_init(sys_mem, 0x17ffe000, 2,
                              get_cps_irq(&s->cps, 3), 10000000,
@@ -509,7 +509,7 @@ static void boston_mach_init(MachineState *machine)
 
     lcd = g_new(MemoryRegion, 1);
     memory_region_init_io(lcd, NULL, &boston_lcd_ops, s, "boston-lcd", 0x8);
-    memory_region_add_subregion_overlap(sys_mem, 0x17fff000, lcd, 0);
+    memory_region_add_subregion(sys_mem, 0x17fff000, lcd);
 
     chr = qemu_chr_new("lcd", "vc:320x240", NULL);
     qemu_chr_fe_init(&s->lcd_display, chr, NULL);
-- 
2.21.0

[PATCH 6/8] hw/vfio/pci: Use memory_region_add_subregion() when priority is 0

[Philippe Mathieu-Daudé]

It is pointless to overlap a memory subregion with priority 0.
Use the simpler memory_region_add_subregion() function.

This patch was produced with the following spatch script:

    @@
    expression region;
    expression offset;
    expression subregion;
    @@
    -memory_region_add_subregion_overlap(region, offset, subregion, 0)
    +memory_region_add_subregion(region, offset, subregion)

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/vfio/pci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 2d40b396f2..74b1eb7ddc 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1095,8 +1095,7 @@ static void vfio_sub_page_bar_update_mapping(PCIDevice *pdev, int bar)
     memory_region_set_size(mmap_mr, size);
     if (size != vdev->bars[bar].size && memory_region_is_mapped(base_mr)) {
         memory_region_del_subregion(r->address_space, base_mr);
-        memory_region_add_subregion_overlap(r->address_space,
-                                            bar_addr, base_mr, 0);
+        memory_region_add_subregion(r->address_space, bar_addr, base_mr);
     }
 
     memory_region_transaction_commit();
-- 
2.21.0

[PATCH 7/8] target/i386: Use memory_region_add_subregion() when priority is 0

[Philippe Mathieu-Daudé]

It is pointless to overlap a memory subregion with priority 0.
Use the simpler memory_region_add_subregion() function.

This patch was produced with the following spatch script:

    @@
    expression region;
    expression offset;
    expression subregion;
    @@
    -memory_region_add_subregion_overlap(region, offset, subregion, 0)
    +memory_region_add_subregion(region, offset, subregion)

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/i386/cpu.c | 2 +-
 target/i386/kvm.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 69f518a21a..6131c62f9d 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6483,7 +6483,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
          */
         memory_region_init_alias(cpu->cpu_as_mem, OBJECT(cpu), "memory",
                                  get_system_memory(), 0, ~0ull);
-        memory_region_add_subregion_overlap(cpu->cpu_as_root, 0, cpu->cpu_as_mem, 0);
+        memory_region_add_subregion(cpu->cpu_as_root, 0, cpu->cpu_as_mem);
         memory_region_set_enabled(cpu->cpu_as_mem, true);
 
         cs->num_ases = 2;
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 1d10046a6c..4e1ba9d474 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -2081,7 +2081,7 @@ static void register_smram_listener(Notifier *n, void *unused)
      */
     memory_region_init_alias(&smram_as_mem, OBJECT(kvm_state), "mem-smram",
                              get_system_memory(), 0, ~0ull);
-    memory_region_add_subregion_overlap(&smram_as_root, 0, &smram_as_mem, 0);
+    memory_region_add_subregion(&smram_as_root, 0, &smram_as_mem);
     memory_region_set_enabled(&smram_as_mem, true);
 
     if (smram) {
-- 
2.21.0

[PATCH 8/8] target/i386/cpu: Use 'mr' for MemoryRegion variables

[Philippe Mathieu-Daudé]

The codebase use 'as' in variable names for AddressSpace objects,
and 'mr' for MemoryRegion objects. Since these variables are
MemoryRegion objects, rename them as 'mr' to avoid confusion with
AddressSpace objects.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/i386/cpu.h |  2 +-
 target/i386/cpu.c | 18 +++++++++---------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index cde2a16b94..1e5ded6e84 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1713,7 +1713,7 @@ struct X86CPU {
     /* in order to simplify APIC support, we leave this pointer to the
        user */
     struct DeviceState *apic_state;
-    struct MemoryRegion *cpu_as_root, *cpu_as_mem, *smram;
+    MemoryRegion *cpu_mr_root, *cpu_mr_mem, *smram;
     Notifier machine_done;
 
     struct kvm_msrs *kvm_msr_buf;
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 6131c62f9d..b5d22740b8 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5983,7 +5983,7 @@ static void x86_cpu_machine_done(Notifier *n, void *unused)
         memory_region_init_alias(cpu->smram, OBJECT(cpu), "smram",
                                  smram, 0, 1ull << 32);
         memory_region_set_enabled(cpu->smram, true);
-        memory_region_add_subregion_overlap(cpu->cpu_as_root, 0, cpu->smram, 1);
+        memory_region_add_subregion_overlap(cpu->cpu_mr_root, 0, cpu->smram, 1);
     }
 }
 #else
@@ -6471,24 +6471,24 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
 #ifndef CONFIG_USER_ONLY
     if (tcg_enabled()) {
-        cpu->cpu_as_mem = g_new(MemoryRegion, 1);
-        cpu->cpu_as_root = g_new(MemoryRegion, 1);
+        cpu->cpu_mr_mem = g_new(MemoryRegion, 1);
+        cpu->cpu_mr_root = g_new(MemoryRegion, 1);
 
         /* Outer container... */
-        memory_region_init(cpu->cpu_as_root, OBJECT(cpu), "memory", ~0ull);
-        memory_region_set_enabled(cpu->cpu_as_root, true);
+        memory_region_init(cpu->cpu_mr_root, OBJECT(cpu), "memory", ~0ull);
+        memory_region_set_enabled(cpu->cpu_mr_root, true);
 
         /* ... with two regions inside: normal system memory with low
          * priority, and...
          */
-        memory_region_init_alias(cpu->cpu_as_mem, OBJECT(cpu), "memory",
+        memory_region_init_alias(cpu->cpu_mr_mem, OBJECT(cpu), "memory",
                                  get_system_memory(), 0, ~0ull);
-        memory_region_add_subregion(cpu->cpu_as_root, 0, cpu->cpu_as_mem);
-        memory_region_set_enabled(cpu->cpu_as_mem, true);
+        memory_region_add_subregion(cpu->cpu_mr_root, 0, cpu->cpu_mr_mem);
+        memory_region_set_enabled(cpu->cpu_mr_mem, true);
 
         cs->num_ases = 2;
         cpu_address_space_init(cs, 0, "cpu-memory", cs->memory);
-        cpu_address_space_init(cs, 1, "cpu-smm", cpu->cpu_as_root);
+        cpu_address_space_init(cs, 1, "cpu-smm", cpu->cpu_mr_root);
 
         /* ... SMRAM with higher priority, linked from /machine/smram.  */
         cpu->machine_done.notify = x86_cpu_machine_done;
-- 
2.21.0

Re: [PATCH 0/8] Simplify memory_region_add_subregion_overlap(..., priority=0)

[Peter Maydell]

On Sat, 14 Dec 2019 at 15:56, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Hi,
>
> In this series we use coccinelle to replace:
> - memory_region_add_subregion_overlap(..., priority=0)
> + memory_region_add_subregion(...)
>
> Rationale is the code is easier to read, and reviewers don't
> have to worry about overlapping because it isn't used.

So our implementation of these two functions makes them
have the same behaviour, but the documentation comments
in memory.h describe them as different: a subregion added
with memory_region_add_subregion() is not supposed to
overlap any other subregion unless that other subregion
was explicitly marked as overlapping. My intention with the
API design here was that using the _overlap() version is
a statement of intent -- this region is *expected* to be
overlapping with some other regions, which hopefully
have a priority set so they go at the right order wrt this one.
Use of the non-overlap function says "I don't expect this
to overlap". (It doesn't actually assert that it doesn't
overlap because we have some legacy uses, notably
in the x86 PC machines, which do overlap without using
the right function, which we've never tried to tidy up.)

We used to have some #if-ed out code in memory.c which
was able to detect incorrect overlap, but it got removed
in commit b613597819587. I thought then and still do
that rather than removing code and API distinctions that
allow us to tell if the board code has done something
wrong (unintentional overlap, especially unintentional
overlap at the same priority value) it would be better to
fix the board bugs so we could enable the warnings/asserts...

thanks
-- PMM

Re: [PATCH 0/8] Simplify memory_region_add_subregion_overlap(..., priority=0)

[Philippe Mathieu-Daudé]

On 12/14/19 5:28 PM, Peter Maydell wrote:
> On Sat, 14 Dec 2019 at 15:56, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> Hi,
>>
>> In this series we use coccinelle to replace:
>> - memory_region_add_subregion_overlap(..., priority=0)
>> + memory_region_add_subregion(...)
>>
>> Rationale is the code is easier to read, and reviewers don't
>> have to worry about overlapping because it isn't used.
> 
> So our implementation of these two functions makes them
> have the same behaviour, but the documentation comments
> in memory.h describe them as different: a subregion added
> with memory_region_add_subregion() is not supposed to
> overlap any other subregion unless that other subregion
> was explicitly marked as overlapping. My intention with the
> API design here was that using the _overlap() version is
> a statement of intent -- this region is *expected* to be
> overlapping with some other regions, which hopefully
> have a priority set so they go at the right order wrt this one.

I didn't notice the documentation differences, now it is clear.

> Use of the non-overlap function says "I don't expect this
> to overlap". (It doesn't actually assert that it doesn't
> overlap because we have some legacy uses, notably
> in the x86 PC machines, which do overlap without using
> the right function, which we've never tried to tidy up.)
> 
> We used to have some #if-ed out code in memory.c which
> was able to detect incorrect overlap, but it got removed
> in commit b613597819587. I thought then and still do
> that rather than removing code and API distinctions that
> allow us to tell if the board code has done something
> wrong (unintentional overlap, especially unintentional
> overlap at the same priority value) it would be better to
> fix the board bugs so we could enable the warnings/asserts...

Maybe we can a warning if priority=0, to force board designers to use 
explicit priority (explicit overlap).

Re: [PATCH 0/8] Simplify memory_region_add_subregion_overlap(..., priority=0)

[Peter Maydell]

On Sat, 14 Dec 2019 at 18:17, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> Maybe we can a warning if priority=0, to force board designers to use
> explicit priority (explicit overlap).

Priority 0 is fine, it's just one of the possible positive and
negative values. I think what ideally we would complain about
is where we see an overlap and both the regions involved
have the same priority value, because in that case which
one the guest sees is implicitly dependent on (I think) which
order the subregions were added, which is fragile if we move
code around. I'm not sure how easy that is to test for or how
much of our existing code violates it, though.

thanks
-- PMM

Re: [PATCH 0/8] Simplify memory_region_add_subregion_overlap(..., priority=0)

[Michael S. Tsirkin]

On Sat, Dec 14, 2019 at 08:01:46PM +0000, Peter Maydell wrote:
> On Sat, 14 Dec 2019 at 18:17, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > Maybe we can a warning if priority=0, to force board designers to use
> > explicit priority (explicit overlap).
> 
> Priority 0 is fine, it's just one of the possible positive and
> negative values. I think what ideally we would complain about
> is where we see an overlap and both the regions involved
> have the same priority value, because in that case which
> one the guest sees is implicitly dependent on (I think) which
> order the subregions were added, which is fragile if we move
> code around. I'm not sure how easy that is to test for or how
> much of our existing code violates it, though.
> 
> thanks
> -- PMM

Problem is it's not uncommon for guests to create such
configs, and then just never access them.
So the thing to do would be to complain *on access*.

-- 
MST

Re: [PATCH 0/8] Simplify memory_region_add_subregion_overlap(..., priority=0)

[Michael S. Tsirkin]

On Sat, Dec 14, 2019 at 04:28:08PM +0000, Peter Maydell wrote:
> (It doesn't actually assert that it doesn't
> overlap because we have some legacy uses, notably
> in the x86 PC machines, which do overlap without using
> the right function, which we've never tried to tidy up.)

It's not exactly legacy uses.

To be more exact, the way the non overlap versions
are *used* is to mean "I don't care what happens when they overlap"
as opposed to "will never overlap".

There are lots of regions where guest can make things overlapping
but doesn't, e.g. PCI BARs can be programmed to overlap
almost anything.

What happens on real hardware if you then access one of
the BARs is undefined, but programming itself is harmless.
That's why we can't assert.

-- 
MST

Re: [PATCH 0/8] Simplify memory_region_add_subregion_overlap(..., priority=0)

[Peter Maydell]

On Sun, 15 Dec 2019 at 09:51, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sat, Dec 14, 2019 at 04:28:08PM +0000, Peter Maydell wrote:
> > (It doesn't actually assert that it doesn't
> > overlap because we have some legacy uses, notably
> > in the x86 PC machines, which do overlap without using
> > the right function, which we've never tried to tidy up.)
>
> It's not exactly legacy uses.
>
> To be more exact, the way the non overlap versions
> are *used* is to mean "I don't care what happens when they overlap"
> as opposed to "will never overlap".

Almost all of the use of the non-overlap versions is
for "these are never going to overlap" -- devices or ram at
fixed addresses in the address space that can't
ever be mapped over by anything else. If you want
"can overlap but I don't care which one wins" then
that would be more clearly expressed by using the _overlap()
version but just giving everything that can overlap there
the same priority.

> There are lots of regions where guest can make things overlapping
> but doesn't, e.g. PCI BARs can be programmed to overlap
> almost anything.
>
> What happens on real hardware if you then access one of
> the BARs is undefined, but programming itself is harmless.
> That's why we can't assert.

Yeah, good point, for the special case where it's the
guest that's determining the addresses where something's
mapped we might want to allow the behaviour to fall out
of the implementation. (You could instead specify set of
priorities that makes the undefined-behaviour something
specific, rather than just an emergent property of
the implementation QEMU happens to have, but it seems
a bit hard to justify.)

thanks
-- PMM

Re: [PATCH 0/8] Simplify memory_region_add_subregion_overlap(..., priority=0)

[Michael S. Tsirkin]

On Sun, Dec 15, 2019 at 03:27:12PM +0000, Peter Maydell wrote:
> On Sun, 15 Dec 2019 at 09:51, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Sat, Dec 14, 2019 at 04:28:08PM +0000, Peter Maydell wrote:
> > > (It doesn't actually assert that it doesn't
> > > overlap because we have some legacy uses, notably
> > > in the x86 PC machines, which do overlap without using
> > > the right function, which we've never tried to tidy up.)
> >
> > It's not exactly legacy uses.
> >
> > To be more exact, the way the non overlap versions
> > are *used* is to mean "I don't care what happens when they overlap"
> > as opposed to "will never overlap".
> 
> Almost all of the use of the non-overlap versions is
> for "these are never going to overlap" -- devices or ram at
> fixed addresses in the address space that can't
> ever be mapped over by anything else. If you want
> "can overlap but I don't care which one wins" then
> that would be more clearly expressed by using the _overlap()
> version but just giving everything that can overlap there
> the same priority.

Problem is device doesn't always know whether something can overlap it.
Imagine device A at a fixed address.
Guest can program device B to overlap the fixed address.
How is device A supposed to know this can happen?



> > There are lots of regions where guest can make things overlapping
> > but doesn't, e.g. PCI BARs can be programmed to overlap
> > almost anything.
> >
> > What happens on real hardware if you then access one of
> > the BARs is undefined, but programming itself is harmless.
> > That's why we can't assert.
> 
> Yeah, good point, for the special case where it's the
> guest that's determining the addresses where something's
> mapped we might want to allow the behaviour to fall out
> of the implementation. (You could instead specify set of
> priorities that makes the undefined-behaviour something
> specific, rather than just an emergent property of
> the implementation QEMU happens to have, but it seems
> a bit hard to justify.)
> 
> thanks
> -- PMM

Re: [PATCH 0/8] Simplify memory_region_add_subregion_overlap(..., priority=0)

[Peter Maydell]

On Mon, 16 Dec 2019 at 11:40, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sun, Dec 15, 2019 at 03:27:12PM +0000, Peter Maydell wrote:
> > On Sun, 15 Dec 2019 at 09:51, Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Sat, Dec 14, 2019 at 04:28:08PM +0000, Peter Maydell wrote:
> > > > (It doesn't actually assert that it doesn't
> > > > overlap because we have some legacy uses, notably
> > > > in the x86 PC machines, which do overlap without using
> > > > the right function, which we've never tried to tidy up.)
> > >
> > > It's not exactly legacy uses.
> > >
> > > To be more exact, the way the non overlap versions
> > > are *used* is to mean "I don't care what happens when they overlap"
> > > as opposed to "will never overlap".
> >
> > Almost all of the use of the non-overlap versions is
> > for "these are never going to overlap" -- devices or ram at
> > fixed addresses in the address space that can't
> > ever be mapped over by anything else. If you want
> > "can overlap but I don't care which one wins" then
> > that would be more clearly expressed by using the _overlap()
> > version but just giving everything that can overlap there
> > the same priority.
>
> Problem is device doesn't always know whether something can overlap it.
> Imagine device A at a fixed address.
> Guest can program device B to overlap the fixed address.
> How is device A supposed to know this can happen?

That's why (the original intention was) only one of the
regions needs to be marked 'overlap OK', not both.

thanks
-- PMM