Hi Julien, > On 25 Mar 2022, at 14:35, Julien Grall <jul...@xen.org> wrote: > > > > On 25/03/2022 13:17, Bertrand Marquis wrote: >> Hi Julien, > > Hi, > >>> On 9 Mar 2022, at 12:20, Julien Grall <jul...@xen.org> wrote: >>> >>> From: Julien Grall <jgr...@amazon.com> >>> >>> 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 <jgr...@amazon.com> >>> >>> --- >>> >>> 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
