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

[Peter Maydell]

On 14 February 2013 13:09, Michael S. Tsirkin <address@hidden> wrote:
> On Thu, Feb 14, 2013 at 12:56:02PM +0000, Peter Maydell wrote:
>> Up to the parent which controls the region being mapped into.
>
> We could just assume same priority as parent

Er, no. I mean the code in control of the parent MR sets the
priority, when it calls memory_region_add_subregion_overlap().

> but what happens if it
> has to be different? There are also aliases so a region
> can have multiple parents.

The alias has its own priority.

> 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);

So this one I know about, and it's a good example of what
I'm talking about. This function sets up a container memory
region ("nvic"), and it is in total control of what is
mapped into that container. Specifically, it puts in a
"nvic_sysregs" background region which covers the whole
0x1000 size of the container (at an implicit priority of
zero). It then layers over that an alias of the GIC
registers ("nvic-gic") at a specific address and with
a priority of 1 so it appears above the background region.
Nobody else ever puts anything in this container, so
the only thing we care about is that the priority of
the nvic-gic region is higher than that of the nvic_sysregs
region; and it's clear from the code that we do that.
Priority is a local question whose meaning is only relevant
within a particular container region, not system-wide, and
having system-wide MEMORY_PRIO_ defines obscures that IMHO.

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

That means PCI is a special case :-) If the guest can
program overlap then presumably PCI specifies semantics
for what happens then, and there need to be PCI specific
wrappers that enforce those semantics and they can call
the relevant _overlap functions when mapping things.
In any case this isn't a concern for the PCI *device*,
which can just provide its memory regions. It's a problem
the PCI *host adaptor* has to deal with when it's figuring
out how to map those regions into the container which
corresponds to its area of the address space.

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

Well, I'm entirely happy with the memory API we have at
the moment, and I'm trying to figure out why you want to
change it...

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

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

It might tell us what bits of code are currently erroneously
mapping regions that overlap without using the _overlap()
function. Then we could fix them.

-- PMM