diff mbox series

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

Message ID 20220309112048.17377-3-julien@xen.org (mailing list archive)
State New, archived
Headers show
Series xen/arm: Don't switch TTBR while the MMU is on | expand

Commit Message

Julien Grall March 9, 2022, 11:20 a.m. UTC
From: Julien Grall <jgrall@amazon.com>

Xen is currently not fully compliant with the 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.

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.

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
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@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.
        - 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(-)

Comments

Julien Grall March 17, 2022, 8:46 p.m. UTC | #1
Hi,

On 09/03/2022 11:20, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Xen is currently not fully compliant with the 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.
> 
> 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.
> 
> 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
> 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@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.
>          - 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
>           bne   1f
>           ret
>   1:
> diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
> index 5db28a8dbd56..b2f31a914103 100644
> --- a/xen/arch/arm/include/asm/config.h
> +++ b/xen/arch/arm/include/asm/config.h
> @@ -107,8 +107,20 @@
>    *  Unused
>    */
>   
> +#ifdef CONFIG_ARM_32
> +
>   #define COMMON_VIRT_START       _AT(vaddr_t, 0)
>   
> +#else
> +
> +#define SLOT0_ENTRY_BITS  39
> +#define SLOT0(slot) (_AT(vaddr_t,slot) << SLOT0_ENTRY_BITS)
> +#define SLOT0_ENTRY_SIZE  SLOT0(1)
> +
> +#define COMMON_VIRT_START       SLOT(1)

This should have been SLOT0(). I got it right in my tree but merge the 
change to the wrong patch :(.

Cheers,
Bertrand Marquis March 25, 2022, 1:17 p.m. UTC | #2
Hi Julien,

> On 9 Mar 2022, at 12:20, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Xen is currently not fully compliant with the Arm because it will
I think you wanted to say “arm arm” her.

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

I think should be rephrased.

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

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

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

>         bne   1f
>         ret
> 1:
> diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
> index 5db28a8dbd56..b2f31a914103 100644
> --- a/xen/arch/arm/include/asm/config.h
> +++ b/xen/arch/arm/include/asm/config.h
> @@ -107,8 +107,20 @@
>  *  Unused
>  */
> 
> +#ifdef CONFIG_ARM_32
> +
> #define COMMON_VIRT_START       _AT(vaddr_t, 0)
> 
> +#else
> +
> +#define SLOT0_ENTRY_BITS  39
> +#define SLOT0(slot) (_AT(vaddr_t,slot) << SLOT0_ENTRY_BITS)
> +#define SLOT0_ENTRY_SIZE  SLOT0(1)
> +
> +#define COMMON_VIRT_START       SLOT(1)
> +
> +#endif
> +
> #define XEN_VIRT_START          (COMMON_VIRT_START + MB(2))
> #define XEN_SLOT_SIZE           MB(2)
> #define XEN_VIRT_END            (XEN_VIRT_START + XEN_SLOT_SIZE)
> @@ -161,14 +173,10 @@
> 
> #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_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 ?

>     BUILD_BUG_ON(first_table_offset(XEN_VIRT_START));
> +#ifdef CONFIG_ARM_32
>     BUILD_BUG_ON(second_linear_offset(XEN_VIRT_START) >= XEN_PT_LPAE_ENTRIES);
> +#endif
> #ifdef CONFIG_DOMAIN_PAGE
>     BUILD_BUG_ON(DOMHEAP_VIRT_START & ~FIRST_MASK);
> #endif
> @@ -611,10 +610,11 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
>     phys_offset = boot_phys_offset;
> 
> #ifdef CONFIG_ARM_64
> -    p = (void *) xen_pgtable;
> -    p[0] = pte_of_xenaddr((uintptr_t)xen_first);
> -    p[0].pt.table = 1;
> -    p[0].pt.xn = 0;
> +    pte = pte_of_xenaddr((uintptr_t)xen_first);
> +    pte.pt.table = 1;
> +    pte.pt.xn = 0;
> +    xen_pgtable[zeroeth_table_offset(XEN_VIRT_START)] = pte;
> +
>     p = (void *) xen_first;
> #else
>     p = (void *) cpu0_pgtable;
> -- 
> 2.32.0
> 

Cheers
Bertrand
Julien Grall March 25, 2022, 1:35 p.m. UTC | #3
On 25/03/2022 13:17, Bertrand Marquis wrote:
> Hi Julien,

Hi,

>> On 9 Mar 2022, at 12:20, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <jgrall@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.

> 
> I think should be rephrased.

I am not entirely sure how to rephrase it. Do you have a proposal?

> 
>>
>> 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"?

> 
>> 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@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?

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

Cheers,
Bertrand Marquis March 25, 2022, 2:05 p.m. UTC | #4
Hi Julien,

> On 25 Mar 2022, at 14:35, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 25/03/2022 13:17, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi,
> 
>>> On 9 Mar 2022, at 12:20, Julien Grall <julien@xen.org> wrote:
>>> 
>>> From: Julien Grall <jgrall@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 <jgrall@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
Julien Grall March 25, 2022, 2:36 p.m. UTC | #5
Hi,

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

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

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 May 21, 2022, 3:49 p.m. UTC | #6
On 25/03/2022 14:36, Julien Grall wrote:
>>>>> 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@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.
> 
> I have only encountered one board with the memory over 512GB. I can't 
> remember whether it is AMD Seattle or X-Gene.

So I found the answer. This was AMD Seattle where the memory start at 
512GB. So I will map Xen starting at 2TB (so there is a bit of slack).

Cheers,
diff mbox series

Patch

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
         bne   1f
         ret
 1:
diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
index 5db28a8dbd56..b2f31a914103 100644
--- a/xen/arch/arm/include/asm/config.h
+++ b/xen/arch/arm/include/asm/config.h
@@ -107,8 +107,20 @@ 
  *  Unused
  */
 
+#ifdef CONFIG_ARM_32
+
 #define COMMON_VIRT_START       _AT(vaddr_t, 0)
 
+#else
+
+#define SLOT0_ENTRY_BITS  39
+#define SLOT0(slot) (_AT(vaddr_t,slot) << SLOT0_ENTRY_BITS)
+#define SLOT0_ENTRY_SIZE  SLOT0(1)
+
+#define COMMON_VIRT_START       SLOT(1)
+
+#endif
+
 #define XEN_VIRT_START          (COMMON_VIRT_START + MB(2))
 #define XEN_SLOT_SIZE           MB(2)
 #define XEN_VIRT_END            (XEN_VIRT_START + XEN_SLOT_SIZE)
@@ -161,14 +173,10 @@ 
 
 #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_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
     BUILD_BUG_ON(first_table_offset(XEN_VIRT_START));
+#ifdef CONFIG_ARM_32
     BUILD_BUG_ON(second_linear_offset(XEN_VIRT_START) >= XEN_PT_LPAE_ENTRIES);
+#endif
 #ifdef CONFIG_DOMAIN_PAGE
     BUILD_BUG_ON(DOMHEAP_VIRT_START & ~FIRST_MASK);
 #endif
@@ -611,10 +610,11 @@  void __init setup_pagetables(unsigned long boot_phys_offset)
     phys_offset = boot_phys_offset;
 
 #ifdef CONFIG_ARM_64
-    p = (void *) xen_pgtable;
-    p[0] = pte_of_xenaddr((uintptr_t)xen_first);
-    p[0].pt.table = 1;
-    p[0].pt.xn = 0;
+    pte = pte_of_xenaddr((uintptr_t)xen_first);
+    pte.pt.table = 1;
+    pte.pt.xn = 0;
+    xen_pgtable[zeroeth_table_offset(XEN_VIRT_START)] = pte;
+
     p = (void *) xen_first;
 #else
     p = (void *) cpu0_pgtable;