Re: [Qemu-devel] [PATCH] docs/memory.txt: Clarify and expand priority/overlap documentation

[Peter Maydell]

On 15 September 2013 16:24, Michael S. Tsirkin <address@hidden> wrote:
> On Sun, Sep 15, 2013 at 03:51:53PM +0100, Peter Maydell wrote:
>> The documentation of how overlapping memory regions behave and how
>> the priority system works was rather brief, and confusion about
>> priorities seems to be quite common for developers trying to understand
>> how the memory region system works, so expand and clarify it.
>> This includes a worked example with overlaps, documentation of the
>> behaviour when an overlapped container has "holes", and mention
>> that it's valid for a region to have both MMIO callbacks and
>> subregions (and how this interacts with priorities when it does).
>>
>> Signed-off-by: Peter Maydell <address@hidden>
>
> Great, thanks a lot!
> Minor comments below:
>
>> ---
>>  docs/memory.txt | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/memory.txt b/docs/memory.txt
>> index feb9fe9..bd0ef6e 100644
>> --- a/docs/memory.txt
>> +++ b/docs/memory.txt
>> @@ -45,6 +45,10 @@ MemoryRegion):
>>    can overlay a subregion of RAM with MMIO or ROM, or a PCI controller
>>    that does not prevent card from claiming overlapping BARs.
>>
>> +  It is valid for regions which are not "pure containers"
>
> I would add "that is, MMIO, RAM or ROM"

Actually maybe it would be better to instead have this new paragraph
go at the bottom of the 'types of region' section rather than inside
the 'container' section. Containers with MMIO etc are a bit odd because
I think conceptually it's easier to think of them as a kind of container but
API-wise you do it by just creating one of the other types of region and
then using the subregion APIs on it. So I put the explanation in the
part describing containers, but it looks a bit odd. If we moved it we would
have:

It is valid to add subregions to a region which is not a pure container
(that is, to an MMIO, RAM or ROM region). This means that the region
will act like a container, except that any addresses within the container's
region which are not claimed by any subregion are handled by the
container itself (ie by its MMIO callbacks or RAM backing). However
it is generally possible to achieve the same effect with a pure container
one of whose subregions is a low priority "background" region covering
the whole address range; this is often clearer and is preferred.
Subregions cannot be added to an alias region.

>> to have subregions;
>> +  this means that any addresses within the container's region which are
>> +  not claimed by a subregion
>
> maybe stress "by any subregion"

Agreed.

>> are handled by the container's MMIO callbacks.
>
> RAM doesn't have MMIO callbacks (at least at the API level),
> maybe say something like "cause an access to the container
> itself (e.g. invoke container's MMIO callbacks or
> modify container's RAM)" is better?

I'm kind of unsure about container RAM/ROM, which is why I didn't
specifically mention it: it's not forbidden by assertions, and it will
have a reasonably straightforward effect by analogy with containers
with MMIO callbacks, but it's hard to see why you'd want it. (We only
use the container-with-IO for a particular effect with the system IO
space's region.) But I've tweaked the wording in my suggested new
para above along these lines.

>> +
>>  - alias: a subsection of another region.  Aliases allow a region to be
>>    split apart into discontiguous regions.  Examples of uses are memory banks
>>    used when the guest address space is smaller than the amount of RAM
>> @@ -81,6 +85,45 @@ allows the region to overlap any other region in the same 
>> container, and
>>  specifies a priority that allows the core to decide which of two regions at
>>  the same address are visible (highest wins).
>>
>> +If the higher priority region in an overlap is a container or alias, then
>> +the lower priority region will appear in any "holes" that the higher 
>> priority
>> +region has left by not mapping subregions
> Maybe add
> "(or recursively - holes that some of the subregions
> left - if some of the subregions are containers or aliases)"
>> to that area of its address range.

Yes -- though I've put it as an extra sentence rather than inserting it into
the middle of an already fairly long sentence:

(This applies recursively -- if the subregions are themselves containers or
aliases that leave holes then the lower priority region will appear in these
holes too.)

>>  Visibility
>>  ----------
>>  The memory core uses the following rules to select a memory region when the
>> @@ -93,8 +136,11 @@ guest accesses an address:
>>    - if the subregion is a leaf (RAM or MMIO), the search terminates
>
> Maybe add
> "And the leaf is selected"

Not sure what you mean by "selected" here.

>>    - if the subregion is a container, the same algorithm is used within the
>>      subregion (after the address is adjusted by the subregion offset)
>> -  - if the subregion is an alias, the search is continues at the alias 
>> target
>> +  - if the subregion is an alias, the search is continued at the alias 
>> target
>>      (after the address is adjusted by the subregion offset and alias offset)
>> +  - if a recursive search within a container or alias subregion does not
>> +    find a match (because of a "hole" in the container's coverage of its
>> +    address range), we continue with the next subregion in priority order
>>
>
> This makes it look like the one way for a search to terminate
> is with RAM or MMIO.
> There are two other cases:
> - non pure container -> container can be selected
> - no match is found -> nothing is selected

How about:
 - if a recursive search within a container or alias subregion does not
    find a match (because of a "hole" in the container's coverage of its
    address range), then if this is a container with its own MMIO or RAM
    backing the search terminates with the container itself. Otherwise
    we continue with the next subregion in priority order

and then one level of bullet point nesting out (ie lining up with
"all direct subregions...")
 - if none of the subregions match then the search terminates with
   no match found

?

-- PMM