Re: [Qemu-devel] [PATCH RFC v2 2/2] hw/pci: handle unassigned pci addresses

[Peter Maydell]

On 15 September 2013 13:17, Michael S. Tsirkin <address@hidden> wrote:
> On Sun, Sep 15, 2013 at 12:23:41PM +0100, Peter Maydell wrote:
>>. If it's a "pure container" then it
>> doesn't respond for areas that none of its subregions
>> cover (it can't, it has no idea what it should do).
>
> Interesting. This really is completely undocumented
> though.
>
> documentation merely says:
>         specifies a priority that allows the core to decide which of two 
> regions at
>         the same address are visible (highest wins)
>
> which makes one think the only thing affecting
> visibility is the priority.

Yes, we could definitely stand to improve the documentation.

> I wonder whether there's a way to describe this
> in terms that dont expose the implementation of
> render_memory_region, somehow.
>
> Maybe like follows:
> when multiple regions cover the same address only one region is going to
> "win" and get invoked for an access.
> The winner can be determined as follows:
> - "pure container" regions created with memory_region_init(..)
>    are ignored
> - if multiple non-container regions cover an address, the winner is
>   determined using a priority vector, built of priority field values
>   from address space down to our region (i.e. region priority, followed by
>   a subregion priority, followed by a sub subregion priority etc).
>
> These priority vectors are compared as follows:
>
> - if vectors are identical, which wins is undefined
> - otherwise if one vector is a sub-vector of another,
>   which wins is undefined
> - otherwise the first vector in the lexicographical
>   order wins

This just seems to me to be documenting a theoretical
implementation. If we're lucky it has the same semantics
as what we actually do; if we're unlucky it doesn't in
some corner case and is just confusing. And it would
certainly be confusing if you look at the code and it does
absolutely nothing like the documentation's algorithm.

I think we could simply add an extra bullet point in the
'Visibility' section of docs/memory.txt saying:
 - if a search within a container or alias subregion does not find
   a match, we continue with the next subregion in priority order.

plus a note at the bottom saying:
"Notice that these rules mean that if a container subregion
does not handle an address in its range (ie it has "holes"
where it has not mapped its own subregions) then lower
priority siblings of the container will be checked to see if they
should be used for accesses within those "holes".

We could also do with explicitly mentioning in the section
about priority that priority is container local, since this seems
to be the number one confusion among people looking at
memory region priority.

>> Mostly this doesn't come up because you don't need
>> to play games with overlapping memory regions and
>> containers very often: the common case is "nothing
>> overlaps at all". But the facilities are there if you need
>> them.

> Dynamic regions like PI BARs are actually very common,
> IMO this means overlapping case is actually very common.

Yes, that's true, but even then it's usually just "overlaps
of subregions within a single container" and there isn't
a need to worry about within-container versus outside-container
interactions.

-- PMM