Simplify memory_region_add_subregion_overlap(..., priority=0)
Subsystems (3)
cover Simplify memory_region_add_subregion_overlap(..., priority=0)
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(-)
Signature · 1 line
2.21.0
PATCH 1/8 hw/arm/nrf51_soc: Use memory_region_add_subregion() when priority is 0
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)--- hw/arm/nrf51_soc.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
hw/arm/nrf51_soc.c +7 -7
@@ -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 );
Signature · 1 line
2.21.0
PATCH 2/8 hw/arm/raspi: Use memory_region_add_subregion() when priority is 0
In reply to 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)--- hw/arm/bcm2835_peripherals.c | 4 ++-- hw/arm/raspi.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
hw/arm/bcm2835_peripherals.c +2 -2
@@ -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 */
hw/arm/raspi.c +1 -1
@@ -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),
Signature · 1 line
2.21.0
PATCH 3/8 hw/arm/xlnx-versal: Use memory_region_add_subregion() when priority is 0
In reply to 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)--- hw/arm/xlnx-versal-virt.c | 3 +-- hw/arm/xlnx-versal.c | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-)
hw/arm/xlnx-versal-virt.c +1 -2
@@ -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 ;
hw/arm/xlnx-versal.c +2 -2
@@ -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)
Signature · 1 line
2.21.0
PATCH 4/8 hw/i386/intel_iommu: Use memory_region_add_subregion when priority is 0
In reply to 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)--- hw/i386/intel_iommu.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
hw/i386/intel_iommu.c +4 -7
@@ -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 );
Signature · 1 line
2.21.0
PATCH 5/8 hw/mips/boston: Use memory_region_add_subregion() when priority is 0
In reply to 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)--- hw/mips/boston.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
hw/mips/boston.c +7 -7
@@ -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 );
Signature · 1 line
2.21.0
PATCH 6/8 hw/vfio/pci: Use memory_region_add_subregion() when priority is 0
In reply to 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)--- hw/vfio/pci.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
hw/vfio/pci.c +1 -2
@@ -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 ();
Signature · 1 line
2.21.0
PATCH 7/8 target/i386: Use memory_region_add_subregion() when priority is 0
In reply to 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)--- target/i386/cpu.c | 2 +- target/i386/kvm.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
target/i386/cpu.c +1 -1
@@ -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 ;
target/i386/kvm.c +1 -1
@@ -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) {
Signature · 1 line
2.21.0
PATCH 8/8 target/i386/cpu: Use 'mr' for MemoryRegion variables
In reply to 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.
--- target/i386/cpu.h | 2 +- target/i386/cpu.c | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-)
target/i386/cpu.h +1 -1
@@ -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;
target/i386/cpu.c +9 -9
@@ -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;
Signature · 1 line
2.21.0
Quoted 9 lines · 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.
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
Quoted 11 lines · Peter Maydell
Quoted 9 lines · 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.
Quoted 10 lines
So our implementation of these two functions makes them have the same behaviour, but the documentation comments in
memory.hdescribe them as different: a subregion added withmemory_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.
Quoted 6 lines
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.)
Quoted 8 lines
We used to have some #if-ed out code in
memory.cwhich was able to detect incorrect overlap, but it got removed in commitb613597819587. 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).
Quoted 2 lines · Philippe Mathieu-Daudé
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
Quoted 13 lines · Peter Maydell
Quoted 2 lines · Philippe Mathieu-Daudé
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.
Quoted 2 lines
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*.
Signature · 1 line
MST
Quoted 4 lines · Peter Maydell
(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.
Signature · 1 line
MST
Quoted 7 lines · Michael S. Tsirkin
Quoted 4 lines · Peter Maydell
(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.)
Quoted 5 lines
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.
Quoted 7 lines
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
Quoted 14 lines · Peter Maydell
Quoted 7 lines · Michael S. Tsirkin
Quoted 4 lines · Peter Maydell
(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.)
Quoted 5 lines
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".
Quoted 8 lines
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?
Quoted 19 lines
Quoted 7 lines
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
Quoted 25 lines · Michael S. Tsirkin
Quoted 14 lines · Peter Maydell
Quoted 7 lines · Michael S. Tsirkin
Quoted 4 lines · Peter Maydell
(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.)
Quoted 5 lines
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".Quoted 8 lines
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.
Quoted 4 lines
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