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

[Avi Kivity]

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.

> 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.