Hi Julien
On 2023/7/5 05:24, Julien Grall wrote:
> Hi Penny,
>
> On 26/06/2023 04:33, Penny Zheng wrote:
>> From: Wei Chen <wei.chen@arm.com>
>>
>> At the moment, on MMU system, enable_mmu() will return to an
>> address in the 1:1 mapping, then each path is responsible to
>> switch to virtual runtime mapping. Then remove_identity_mapping()
>> is called to remove all 1:1 mapping.
>
> The identity mapping is only removed for the boot CPU. You mention it
> correctly below but here it lead to think it may be called on the
> secondary CPU. So I would add 'on the boot CPU'.
>
>>
>> Since remove_identity_mapping() is not necessary on Non-MMU system,
>> and we also avoid creating empty function for Non-MMU system, trying
>> to keep only one codeflow in arm64/head.S, we move path switch and
>> remove_identity_mapping() in enable_mmu() on MMU system.
>>
>> As the remove_identity_mapping should only be called for the boot
>> CPU only, so we introduce enable_boot_mmu for boot CPU and
>> enable_runtime_mmu for secondary CPUs in this patch.
>
> NIT: Add () after enable_runtime_mmu to be consistent with the rest of
> commit message. Same for the title.
>
sure.
> Also, I would prefer if we name the functions properly from the start.
> So we don't have to rename them when they are moved in patch 7.
>
ok. change them to enable_runtime_mm() and enable_boot_mm() at the beginning
>>
>> Signed-off-by: Wei Chen <wei.chen@arm.com>
>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>> ---
>> v3:
>> - new patch
>> ---
>> xen/arch/arm/arm64/head.S | 87 +++++++++++++++++++++++++++++++--------
>> 1 file changed, 70 insertions(+), 17 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 10a07db428..4dfbe0bc6f 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -314,21 +314,12 @@ real_start_efi:
>> bl check_cpu_mode
>> bl cpu_init
>> - bl create_page_tables
>> - load_paddr x0, boot_pgtable
>> - bl enable_mmu
>> /* We are still in the 1:1 mapping. Jump to the runtime
>> Virtual Address. */
>
> This comment is not accurate anymore given that the MMU is off.
>
I'll remove.
>> - ldr x0, =primary_switched
>> - br x0
>> + ldr lr, =primary_switched
>> + b enable_boot_mmu
>> +
>> primary_switched:
>> - /*
>> - * The 1:1 map may clash with other parts of the Xen virtual
>> memory
>> - * layout. As it is not used anymore, remove it completely to
>> - * avoid having to worry about replacing existing mapping
>> - * afterwards.
>> - */
>> - bl remove_identity_mapping
>> bl setup_fixmap
>> #ifdef CONFIG_EARLY_PRINTK
>> /* Use a virtual address to access the UART. */
>> @@ -373,13 +364,11 @@ GLOBAL(init_secondary)
>> #endif
>> bl check_cpu_mode
>> bl cpu_init
>> - load_paddr x0, init_ttbr
>> - ldr x0, [x0]
>> - bl enable_mmu
>> /* We are still in the 1:1 mapping. Jump to the runtime
>> Virtual Address. */
Note: Remove this too.
>> - ldr x0, =secondary_switched
>> - br x0
>> + ldr lr, =secondary_switched
>> + b enable_runtime_mmu
>> +
>> secondary_switched:
>> #ifdef CONFIG_EARLY_PRINTK
>> /* Use a virtual address to access the UART. */
>> @@ -694,6 +683,70 @@ enable_mmu:
>> ret
>> ENDPROC(enable_mmu)
>> +/*
>> + * Turn on the Data Cache and the MMU. The function will return
>> + * to the virtual address provided in LR (e.g. the runtime mapping).
>
> The description here is exactly the same as below. However, there is a
> different between the two functions. One is to deal with the secondary
> CPUs whilst the second is for the boot CPUs.
>
I'll add-on: This function deals with the secondary CPUs.
>> + *
>> + * Inputs:
>> + * lr : Virtual address to return to.
>> + *
>> + * Clobbers x0 - x5
>> + */
>> +enable_runtime_mmu:
>
> I find "runtime" confusing in this case. How about
> "enable_secondary_cpu_mm"?
>
Sure, will do.
>> + mov x5, lr
>> +
>> + load_paddr x0, init_ttbr
>> + ldr x0, [x0]
>> +
>> + bl enable_mmu
>> + mov lr, x5
>> +
>> + /* return to secondary_switched */
>> + ret
>> +ENDPROC(enable_runtime_mmu)
>> +
>> +/*
>> + * Turn on the Data Cache and the MMU. The function will return
>> + * to the virtual address provided in LR (e.g. the runtime mapping).
>
> Similar remark as for the comment above.
I'll add-on: This function deals with the boot CPUs.
>
>> + *
>> + * Inputs:
>> + * lr : Virtual address to return to.
>> + *
>> + * Clobbers x0 - x5
>> + */
>> +enable_boot_mmu:
>
> Based on my comment above, I would sugesst to call it "enable_boot_cpu_mm"
sure,
>
>> + mov x5, lr
>> +
>> + bl create_page_tables
>> + load_paddr x0, boot_pgtable
>> +
>> + bl enable_mmu
>> + mov lr, x5
>> +
>> + /*
>> + * The MMU is turned on and we are in the 1:1 mapping. Switch
>> + * to the runtime mapping.
>> + */
>> + ldr x0, =1f
>> + br x0
>> +1:
>> + /*
>> + * The 1:1 map may clash with other parts of the Xen virtual
>> memory
>> + * layout. As it is not used anymore, remove it completely to
>> + * avoid having to worry about replacing existing mapping
>> + * afterwards. Function will return to primary_switched.
>> + */
>> + b remove_identity_mapping
>> +
>> + /*
>> + * Here might not be reached, as "ret" in
>> remove_identity_mapping
>> + * will use the return address in LR in advance. But keep ret
>> here
>> + * might be more safe if "ret" in remove_identity_mapping is
>> removed
>> + * in future.
>> + */
>> + ret
>
> Given this path is meant to be unreachable, I would prefer if we call
> "fail".
sure,
>
>> +ENDPROC(enable_boot_mmu)
>> +
>> /*
>> * Remove the 1:1 map from the page-tables. It is not easy to keep
>> track
>> * where the 1:1 map was mapped, so we will look for the top-level
>> entry
>
> Cheers,
>