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

[Michael S. Tsirkin]

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 <address@hidden> wrote:
> > 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?
> 
> 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