[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] docs/memory.txt: Clarify and expand priority/ov
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH] docs/memory.txt: Clarify and expand priority/overlap documentation |
Date: |
Sun, 15 Sep 2013 20:07:34 +0300 |
On Sun, Sep 15, 2013 at 05:55:54PM +0100, Peter Maydell wrote:
> 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.
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.