On Sun, Sep 15, 2013 at 05:55:54PM +0100, Peter Maydell wrote: > On 15 September 2013 16:24, Michael S. Tsirkin <m...@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.mayd...@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.
