Re: [PATCH v5 2/5] xen/arm64: Rework the memory layout

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 30/01/2023 19:36, Julien Grall wrote:
> 
> 
> On 30/01/2023 09:08, Michal Orzel wrote:
>> Hi Julien,
> 
> Hi Michal,
> 
>> On 27/01/2023 20:55, Julien Grall wrote:
>>>
>>>
>>> From: Julien Grall <jgrall@xxxxxxxxxx>
>>>
>>> Xen is currently not fully compliant with the Arm Arm because it will
>>> 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.
>>>
>>> In most of the case we expect Xen to be loaded in low memory. I am aware
>>> of one platform (i.e AMD Seattle) where the memory start above 512GB.
>>> To give us some slack, consider that Xen may be loaded in the first 2TB
>>> of the physical address space.
>>>
>>> The memory layout is reshuffled to keep the first four slots of the zeroeth
>>> level free. Xen will now be loaded at (2TB + 2MB). This requires a slight
>>> tweak of the boot code because XEN_VIRT_START cannot be used as an
>>> immediate.
>>>
>>> This reshuffle will make trivial to create a 1:1 mapping when Xen is
>>> loaded below 2TB.
>>>
>>> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
>>> Tested-by: Luca Fancellu <luca.fancellu@xxxxxxx>
>>> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
>>>
>>> ---
>>>      Changes in v5:
>>>          - We are reserving 4 slots rather than 2.
>>>          - Fix the addresses in the layout comment.
>>>          - Fix the size of the region in the layout comment
>>>          - Add Luca's tested-by (the reviewed-by was not added
>>>            because of the changes requested by Michal
>>>          - Add Michal's reviewed-by
>>>
>>>      Changes in v4:
>>>          - Correct the documentation
>>>          - The start address is 2TB, so slot0 is 4 not 2.
>>>
>>>      Changes in v2:
>>>          - Reword the commit message
>>>          - Load Xen at 2TB + 2MB
>>>          - Update the documentation to reflect the new layout
>>> ---
>>>   xen/arch/arm/arm64/head.S         |  3 ++-
>>>   xen/arch/arm/include/asm/config.h | 34 +++++++++++++++++++++----------
>>>   xen/arch/arm/mm.c                 | 11 +++++-----
>>>   3 files changed, 31 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>>> index 4a3f87117c83..663f5813b12e 100644
>>> --- a/xen/arch/arm/arm64/head.S
>>> +++ b/xen/arch/arm/arm64/head.S
>>> @@ -607,7 +607,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
>>>           bne   1f
>>>           ret
>>>   1:
>>> diff --git a/xen/arch/arm/include/asm/config.h 
>>> b/xen/arch/arm/include/asm/config.h
>>> index 5df0e4c4959b..e388462c23d1 100644
>>> --- a/xen/arch/arm/include/asm/config.h
>>> +++ b/xen/arch/arm/include/asm/config.h
>>> @@ -72,16 +72,13 @@
>>>   #include <xen/page-size.h>
>>>
>>>   /*
>>> - * Common ARM32 and ARM64 layout:
>>> + * ARM32 layout:
>>>    *   0  -   2M   Unmapped
>>>    *   2M -   4M   Xen text, data, bss
>>>    *   4M -   6M   Fixmap: special-purpose 4K mapping slots
>>>    *   6M -  10M   Early boot mapping of FDT
>>>    *   10M - 12M   Livepatch vmap (if compiled in)
>>>    *
>>> - * ARM32 layout:
>>> - *   0  -  12M   <COMMON>
>>> - *
>>>    *  32M - 128M   Frametable: 32 bytes per page for 12GB of RAM
>>>    * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
>>>    *                    space
>>> @@ -90,14 +87,23 @@
>>>    *   2G -   4G   Domheap: on-demand-mapped
>>>    *
>>>    * ARM64 layout:
>>> - * 0x0000000000000000 - 0x0000007fffffffff (512GB, L0 slot [0])
>>> - *   0  -  12M   <COMMON>
>>> + * 0x0000000000000000 - 0x000001ffffffffff (2TB, L0 slots [0..3])
>>> + *
>>> + *  Reserved to identity map Xen
>>> + *
>>> + * 0x0000020000000000 - 0x0000027fffffffff (512GB, L0 slot [4]
>>> + *  (Relative offsets)
>>> + *   0  -   2M   Unmapped
>>> + *   2M -   4M   Xen text, data, bss
>>> + *   4M -   6M   Fixmap: special-purpose 4K mapping slots
>>> + *   6M -  10M   Early boot mapping of FDT
>>> + *  10M -  12M   Livepatch vmap (if compiled in)
>>>    *
>>>    *   1G -   2G   VMAP: ioremap and early_ioremap
>>>    *
>>>    *  32G -  64G   Frametable: 56 bytes per page for 2TB of RAM
>>>    *
>>> - * 0x0000008000000000 - 0x00007fffffffffff (127.5TB, L0 slots [1..255])
>>> + * 0x0000028000000000 - 0x00007fffffffffff (125TB, L0 slots [5..255])
>>>    *  Unused
>>>    *
>>>    * 0x0000800000000000 - 0x000084ffffffffff (5TB, L0 slots [256..265])
>>> @@ -107,7 +113,17 @@
>>>    *  Unused
>>>    */
>>>
>>> +#ifdef CONFIG_ARM_32
>>>   #define XEN_VIRT_START          _AT(vaddr_t, MB(2))
>>> +#else
>>> +
>>> +#define SLOT0_ENTRY_BITS  39
>>> +#define SLOT0(slot) (_AT(vaddr_t,slot) << SLOT0_ENTRY_BITS)
>>> +#define SLOT0_ENTRY_SIZE  SLOT0(1)
>>> +
>>> +#define XEN_VIRT_START          (SLOT0(4) + _AT(vaddr_t, MB(2)))
>>> +#endif
>>> +
>>>   #define XEN_VIRT_SIZE           _AT(vaddr_t, MB(2))
>>>
>>>   #define FIXMAP_VIRT_START       (XEN_VIRT_START + XEN_VIRT_SIZE)
>>> @@ -163,10 +179,6 @@
>>>
>>>   #else /* ARM_64 */
>>>
>>> -#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_SIZE   GB(1)
>>>
>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>>> index f758cad545fa..0b0edf28d57a 100644
>>> --- a/xen/arch/arm/mm.c
>>> +++ b/xen/arch/arm/mm.c
>>> @@ -153,7 +153,7 @@ static void __init __maybe_unused build_assertions(void)
>>>   #endif
>>>       /* Page table structure constraints */
>>>   #ifdef CONFIG_ARM_64
>>> -    BUILD_BUG_ON(zeroeth_table_offset(XEN_VIRT_START));
>>> +    BUILD_BUG_ON(zeroeth_table_offset(XEN_VIRT_START) < 2);
>> I haven't spotted this before but this should be < 4.
> 
> Good catch! I am planning to handle it on commit unless there is more to
> fix in this patch.
No more remarks from my side.

~Michal