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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

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

Something in this area, telling what we do and not what we don't

> 
>>> 
>>> On arm64, the memory layout  has plenty of unused space. In most of
>>> the case we expect Xen to be loaded in low memory.
>>> 
>>> The memory layout is reshuffled to keep the 0th slot free. Xen will now
>> 0th slot of first level of page table.
> 
> We want to keep the first 512GB free. So did you intend to write "zero level"?

Yes sorry.

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

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


>>> -#define SLOT0_ENTRY_BITS  39
>>> -#define SLOT0(slot) (_AT(vaddr_t,slot) << SLOT0_ENTRY_BITS)
>>> -#define SLOT0_ENTRY_SIZE  SLOT0(1)
>>> -
>>> -#define VMAP_VIRT_START  GB(1)
>>> +#define VMAP_VIRT_START  (SLOT0(1) + GB(1))
>>> #define VMAP_VIRT_END    (VMAP_VIRT_START + GB(1))
>>> 
>>> -#define FRAMETABLE_VIRT_START  GB(32)
>>> +#define FRAMETABLE_VIRT_START  (SLOT0(1) + GB(32))
>>> #define FRAMETABLE_SIZE        GB(32)
>>> #define FRAMETABLE_NR          (FRAMETABLE_SIZE / sizeof(*frame_table))
>>> #define FRAMETABLE_VIRT_END    (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1)
>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>>> index 6b7c41d827ca..75ed9a3ce249 100644
>>> --- a/xen/arch/arm/mm.c
>>> +++ b/xen/arch/arm/mm.c
>>> @@ -187,11 +187,10 @@ static void __init __maybe_unused 
>>> build_assertions(void)
>>>     BUILD_BUG_ON(DIRECTMAP_VIRT_START & ~FIRST_MASK);
>>> #endif
>>>     /* Page table structure constraints */
>>> -#ifdef CONFIG_ARM_64
>>> -    BUILD_BUG_ON(zeroeth_table_offset(XEN_VIRT_START));
>>> -#endif
>> Don’t you want to enforce the opposite now ? Check that it is not on slot 0 ?
> 
> I can. But this is not going to guarantee us the first 512GB is going to be 
> free.

It could still make sure that XEN_VIRT_START is not in the slot 0.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall