Re: [Xen-devel] [PATCH v2] xen: arm: improve handling of system with non-contiguous RAM regions

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-12-04 at 17:02 +0000, George Dunlap wrote:
> On Mon, Dec 2, 2013 at 2:39 PM, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:
> > arm32 currently only makes use of memory which is contiguous with the first
> > bank. On the Midway platform this means that we only use 4GB of the 8GB
> > available.
> >
> > Change things to make use of non-contiguous memory regions with the
> > restriction that we require that at least half of the total span of the RAM
> > addresses contain RAM. The frametable is currently not sparse and so this
> > restriction avoids problems with allocating enormous amounts of memory for 
> > the
> > frametable to cover holes in the address space and exhausting the actual 
> > RAM.
> >
> > 50% is arguably too restrictive. 4GB of RAM requires 32MB of frametable on
> > arm32 and 56M on arm64, so we could probably cope with a lower ratio of 
> > actual
> > RAM. However half is nice and conservative.
> >
> > arm64 currently uses all banks without regard for the size of the 
> > frametable,
> > which I have observed causing problems on models. Implement that same
> > restriction as arm32 there.
> >
> > Long term we should look at moving to a pfn compression based scheme similar
> > to x86, which removes the holes from the frametable.
> >
> > There were some bogus/outdated comments scattered around this code which I
> > have removed.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > Tested-by: Julien Grall <julien.grall@xxxxxxxxxx>
> > Acked-by: Julien Grall <julien.grall@xxxxxxxxxx>
> > Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> > ---
> > v2: Rebase over "avoid truncation in mfn to paddr conversions"
> >
> > Freeze:
> >
> > The benefit of this series is that we can use the full 8GB of RAM on the
> > midway systems, rather than being limited to just the first 4GB (less I/O
> > holes). I expect it to be common that server class 32-bit arm systems will
> > have a hole in memory (between a bank <4GB and one above).
> >
> > The risk is that we regress on some other supported platform but AFAIK the
> > vexpress and sunxi only have a single memory bank and the arndale has
> > contiguous memory regions.
> 
> If there were a bug in this patch, what would be the likely impact?
> Would the host not boot?  Would it only be able to access a part of
> its memory?

These are the two most likely outcomes of a bug. I can't say which one
is more likely for sure.

> Or would it just allocate too much memory for a frametable?

This is unlikely since the restriction placed on the frametable size
relative to RAM size is deliberately pretty harsh to mitigate the risk
here.

> The complexity of the patch looks middle to middle-high, just from the
> number of lines -- would you agree with that, or is the resulting code
> actually fairly simple and straightforward?

There's quite a few big comments ;-). (1/4 of the additions are
comments)

The need to reindent the arm32 "/* Add non-xenheap memory */" loop
inside a new outer loop also adds quite a bit to the overall look of
complexity, plus I've created lots of temporary variables to try and
make the core logic as clear as possible.

The real meat is the 1/2 dozen lines after the "If the new bank is
contiguous" comment in the arm32 case and the two lines after "We allow
non-contiguous regions " in the arm64 case. I think those pretty
unambiguously implement what is described in the commit log / comments.

> Does the fact that vexpress, sunxi, and arndale have single bank or
> contiguous memory regions mean that they will skip the complicated
> sections of code?  Or will they go through the complicated code and
> possibly trip over some bugs in spite of the fact that their layout is
> fairly simple?

There is some difference between the current code and the "simple fall
straight through" case after this patch but I don't think it is major.

FWIW Julien has tested on Arndale (a multiple contiguous bank
configuration) and it is OK.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel