Re: [Qemu-devel] [PATCH RFC] memory: drop _overlap variant

[Michael S. Tsirkin]

On Thu, Feb 14, 2013 at 12:56:02PM +0000, Peter Maydell wrote:
> On 14 February 2013 12:45, Michael S. Tsirkin <address@hidden> wrote:
> > overlap flag in the region is currently unused, most devices have no
> > idea whether their region overlaps with anything, so drop it,
> > assume that all regions can overlap and always require priority.
> 
> Devices themselves shouldn't care, for the most part -- they just
> provide a memory region and it's their parent that has to map it
> and know whether it overlaps or not. Similarly, parents should
> generally be in control of the container they're mapping the
> memory region into, and know whether it will be an overlapping
> map or not.
> 
> > It's also not clear how should devices allocate priorities.
> 
> Up to the parent which controls the region being mapped into.

We could just assume same priority as parent but what happens if it
has to be different? There are also aliases so a region
can have multiple parents. Presumably it will have to have
different priorities depending on what the parent does?
Here's a list of instances using priority != 0.

hw/armv7m_nvic.c:                                MEMORY_PRIO_LOW);
hw/cirrus_vga.c:                                    MEMORY_PRIO_LOW);
hw/cirrus_vga.c:                                    MEMORY_PRIO_LOW);
hw/cirrus_vga.c:                                &s->low_mem_container, 
MEMORY_PRIO_LOW);
hw/kvm/pci-assign.c: &r_dev->mmio, MEMORY_PRIO_LOW);
hw/kvmvapic.c:    memory_region_add_subregion(as, rom_paddr, &s->rom, 
MEMORY_PRIO_HIGH);
hw/lpc_ich9.c:                                        MEMORY_PRIO_LOW);
hw/onenand.c:                                &s->mapped_ram, MEMORY_PRIO_LOW);
hw/pam.c:                                    MEMORY_PRIO_LOW);
hw/pc.c:                                MEMORY_PRIO_LOW);
hw/pc_sysfw.c:                                isa_bios, MEMORY_PRIO_LOW);
hw/pc_sysfw.c:                                isa_bios, MEMORY_PRIO_LOW);
hw/pci/pci.c:                                        MEMORY_PRIO_LOW);
hw/pci/pci_bridge.c:    memory_region_add_subregion(parent_space, base, alias, 
MEMORY_PRIO_LOW);
hw/piix_pci.c:                                MEMORY_PRIO_LOW);
hw/piix_pci.c:                                &d->rcr_mem, MEMORY_PRIO_LOW);
hw/q35.c:                                &mch->smram_region, MEMORY_PRIO_LOW);
hw/vga-isa.c:                                MEMORY_PRIO_LOW);
hw/vga.c:                                    MEMORY_PRIO_MEDIUM);
hw/vga.c:                                vga_io_memory, MEMORY_PRIO_LOW);
hw/xen_pt_msi.c:                                MEMORY_PRIO_MEDIUM); /* 
Priority: pci default + 1

Making priority relative to parent but not the same just seems like a recipe 
for disaster.

> I definitely don't like making the priority argument mandatory:
> this is just introducing pointless boilerplate for the common
> case where nothing overlaps and you know nothing overlaps.

Non overlapping is not a common case at all.  E.g. with normal PCI
devices you have no way to know nothing overlaps - addresses are guest
programmable.

See also recent discussion about 64 bit BARs.

We could add a wrapper for MEMORY_PRIO_LOWEST - will that address
your concern?

> Maybe we should take the printf() about subregion collisions
> in memory_region_add_subregion_common() out of the #if 0
> that it currently sits in?
> 
> -- PMM

This is just a debugging tool, it won't fix anything.

-- 
MST