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

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Sep 15, 2013 at 05:55:54PM +0100, Peter Maydell wrote:
> On 15 September 2013 16:24, Michael S. Tsirkin <mst@redhat.com> 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 <peter.maydell@linaro.org>
> >
> > 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.

Well search terminates but what is the result?
It says "to select a memory region"
so for the result I said "is selected".

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

Looks good.