Re: [PATCH early-RFC 2/5] xen/arm64: Rework the memory layout

[Julien Grall <julien@xxxxxxx>]

Hi,

On 25/03/2022 14:05, Bertrand Marquis wrote:
> > On 25 Mar 2022, at 14:35, Julien Grall <julien@xxxxxxx> wrote:
> >
> >
> >
> > On 25/03/2022 13:17, Bertrand Marquis wrote:
> > > Hi Julien,
> > Hi,
> >
> > > > On 9 Mar 2022, at 12:20, Julien Grall <julien@xxxxxxx> wrote:
> > > >
> > > > From: Julien Grall <jgrall@xxxxxxxxxx>
> > > >
> > > > Xen is currently not fully compliant with the Arm because it will
> > > I think you wanted to say “arm arm” her.
> > Yes. I will update it.
> >
> > > > switch the TTBR with the MMU on.
> > > >
> > > > In order to be compliant, we need to disable the MMU before
> > > > switching the TTBR. The implication is the page-tables should
> > > > contain an identity mapping of the code switching the TTBR.
> > > >
> > > > If we don't rework the memory layout, we would need to find a
> > > > virtual address that matches a physical address and doesn't clash
> > > > with the static virtual regions. This can be a bit tricky.
> > > This sentence is a bit misleading. Even with the rework you need
> > > to do that just by moving the Xen virtual address upper you make
> > > sure that anything physical memory under 512GB can be mapped
> > > 1:1 without clashing with other Xen mappings (unless Xen is loaded
> > > in memory at physical address 512GB which would end in the same issue).
> > So the key difference is with the rework, it is trivial to create the 1:1 
> > mapping as we know it doesn't clash. This is not the case without the rework.
> Agree
>
> > > I think should be rephrased.
> > I am not entirely sure how to rephrase it. Do you have a proposal?
> Turn it into the positive:
> Rework the memory layout to put Xen over 512GB. This makes it trivial to create
> a 1:1 mapping, with the assumption that the physical memory is under 512GB.
I will use this wording in the next version.

> > > > be loaded at (512GB + 2MB). This requires a slight tweak of the boot
> > > > code as XEN_VIRT_START cannot be used as an immediate.
> > > >
> > > > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> > > >
> > > > ---
> > > >
> > > >     TODO:
> > > >         - I vaguely recall that one of the early platform we supported add
> > > >           the memory starting in high memory (> 1TB). I need to check
> > > >           whether the new layout will be fine.
> > > I think we have some Juno with some memory like that, tell me if you need help 
> > > here.
> > Would you be able to check the memory layout and confirm?
> I checked and the Juno we have as the high memory a lot lower than that:
> RAM: 0000000880000000 - 00000009ffffffff
>
> No idea why it was a lot higher in my mind.
I have only encountered one board with the memory over 512GB. I can't 
remember whether it is AMD Seattle or X-Gene.
> > > >         - Update the documentation to reflect the new layout
> > > > ---
> > > > xen/arch/arm/arm64/head.S         |  3 ++-
> > > > xen/arch/arm/include/asm/config.h | 20 ++++++++++++++------
> > > > xen/arch/arm/mm.c                 | 14 +++++++-------
> > > > 3 files changed, 23 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > > > index 66d862fc8137..878649280d73 100644
> > > > --- a/xen/arch/arm/arm64/head.S
> > > > +++ b/xen/arch/arm/arm64/head.S
> > > > @@ -594,7 +594,8 @@ create_page_tables:
> > > >           * need an additional 1:1 mapping, the virtual mapping will
> > > >           * suffice.
> > > >           */
> > > > -        cmp   x19, #XEN_VIRT_START
> > > > +        ldr   x0, =XEN_VIRT_START
> > > > +        cmp   x19, x0
> > > A comment in the code would be good here to prevent someone reverting this.
> > Anyone trying to revert the change will face a compilation error:
> >
> >   CC      arch/arm/arm64/head.o
> > arch/arm/arm64/head.S: Assembler messages:
> > arch/arm/arm64/head.S:597: Error: immediate out of range
> >
> > So I don't think a comment is necessary because this is not specific to a 
> > compiler/assembler.
> Right I should have thought of the compilation error.
TBH, I would have preferred to keep the single instruction. AFAICT, the 
immediate should be either between 0 - 4095. Or a number between 4096 
and 2^24 that is 4KB aligned.
So it would not suit us here.

Cheers,

--
Julien Grall