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

[George Dunlap <george.dunlap@eu.citrix.com>]

On 12/04/2013 05:29 PM, Ian Campbell wrote:
> 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@citrix.com> 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@citrix.com>
>>> Tested-by: Julien Grall <julien.grall@linaro.org>
>>> Acked-by: Julien Grall <julien.grall@linaro.org>
>>> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
>>> ---
>>> 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.

It sounds like what you're saying is that the patch is more mid-low 
complexity?

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

Right.  I'm inclined to classify "Can't access half of the ram" as a 
kind of bug.  It's a bit less serious than "can't boot", but then again, 
even "can't boot" is not a bad one to have as bugs go -- you know right 
away that there's a problem, so it should be fairly easy to find a fix.

So the benefit is fixing the "can't access half the ram" bug, at the 
risk of introducing an easily discoverable and fixable "can't boot" bug 
(or an equivalent "can't access all the ram" bug); and that risk is 
fairly low, if your assessment of the complexity of the patch is accurate.

Unless someone has an objection, I think it should probably go in, then.

I guess that's a, "Release-ack-if-no-one-has-objected-in-a-day-or-so". :-)

  -George