On Thu, Feb 14, 2013 at 04:14:39PM +0200, Avi Kivity wrote: > On Thu, Feb 14, 2013 at 3:09 PM, Michael S. Tsirkin <m...@redhat.com> wrote: > > On Thu, Feb 14, 2013 at 12:56:02PM +0000, Peter Maydell wrote: > >> On 14 February 2013 12:45, Michael S. Tsirkin <m...@redhat.com> 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? > > Priority is only considered relative to siblings. The parent's > priority is only considered wrt the parent's siblings, not its > children.
But some parents are system created and shared by many devices so children for such have no idea who their siblings are. Please take a look at the typical map in this mail: '[BUG] Guest OS hangs on boot when 64bit BAR present' system overlap 0 pri 0 [0x0 - 0x7fffffffffffffff] kvmvapic-rom overlap 1 pri 1000 [0xca000 - 0xcd000] pc.ram overlap 0 pri 0 [0xca000 - 0xcd000] ++ pc.ram [0xca000 - 0xcd000] is added to view .................... smram-region overlap 1 pri 1 [0xa0000 - 0xc0000] pci overlap 0 pri 0 [0xa0000 - 0xc0000] cirrus-lowmem-container overlap 1 pri 1 [0xa0000 - 0xc0000] cirrus-low-memory overlap 0 pri 0 [0xa0000 - 0xc0000] ++cirrus-low-memory [0xa0000 - 0xc0000] is added to view kvm-ioapic overlap 0 pri 0 [0xfec00000 - 0xfec01000] ++kvm-ioapic [0xfec00000 - 0xfec01000] is added to view pci-hole64 overlap 0 pri 0 [0x100000000 - 0x4000000100000000] pci overlap 0 pri 0 [0x100000000 - 0x4000000100000000] pci-hole overlap 0 pri 0 [0x7d000000 - 0x100000000] pci overlap 0 pri 0 [0x7d000000 - 0x100000000] ivshmem-bar2-container overlap 1 pri 1 [0xfe000000 - 0x100000000] ivshmem.bar2 overlap 0 pri 0 [0xfe000000 - 0x100000000] ++ivshmem.bar2 [0xfe000000 - 0xfec00000] is added to view ++ivshmem.bar2 [0xfec01000 - 0x100000000] is added to view As you see, ioapic at 0xfec00000 overlaps pci-hole. ioapic is guest programmable in theory - should use _overlap? pci-hole is not but can overlap with ioapic. So also _overlap? Let's imagine someone writes a guest programmable device for ARM. Now we should update all ARM devices from regular to _overlap? > > 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? > > The alias region has its own priority > > > 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. > > Non overlapping is mostly useful for embedded platforms. Maybe it should have a longer name like _nonoverlap then? Current API makes people assume _overlap is only for special cases and default should be non overlap. -- MST