Hi Julien
On 2023/7/5 06:25, Julien Grall wrote:
> Hi Penny,
>
> Title: You want to clarify that this change is arm64 only. So:
>
> xen/arm64: mmu: ...
>
> On 26/06/2023 04:34, Penny Zheng wrote:
>> Original setup_fixmap is actually doing two seperate tasks, one is
>> enabling
>> the early UART when earlyprintk on, and the other is to set up the fixmap
>> even when earlyprintk is not configured.
>>
>> To be more dedicated and precise, the old function shall be split into
>> two
>> functions, setup_early_uart and new setup_fixmap.
> While some of the split before would be warrant even without the MPU
> support. This one is not because there is limited point to have 3 lines
> function. So I think you want to justify based on what you plan to do
> with the MPU code.
>
> That said, I don't think we need to introduce setup_fixmap. See below.
>
>
>>
>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>> Signed-off-by: Wei Chen <wei.chen@arm.com>
>> ---
>> v3:
>> - new patch
>> ---
>> xen/arch/arm/arm64/head.S | 3 +++
>> xen/arch/arm/arm64/mmu/head.S | 24 +++++++++++++++++-------
>> 2 files changed, 20 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index e63886b037..55a4cfe69e 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -258,7 +258,10 @@ real_start_efi:
>> b enable_boot_mm
>> primary_switched:
>> + bl setup_early_uart
>> +#ifdef CONFIG_HAS_FIXMAP
>> bl setup_fixmap
>> +#endif
>> #ifdef CONFIG_EARLY_PRINTK
>> /* Use a virtual address to access the UART. */
>> ldr x23, =EARLY_UART_VIRTUAL_ADDRESS
>> diff --git a/xen/arch/arm/arm64/mmu/head.S
>> b/xen/arch/arm/arm64/mmu/head.S
>> index 2b209fc3ce..295596aca1 100644
>> --- a/xen/arch/arm/arm64/mmu/head.S
>> +++ b/xen/arch/arm/arm64/mmu/head.S
>> @@ -367,24 +367,34 @@ identity_mapping_removed:
>> ENDPROC(remove_identity_mapping)
>> /*
>> - * Map the UART in the fixmap (when earlyprintk is used) and hook the
>> - * fixmap table in the page tables.
>> - *
>> - * The fixmap cannot be mapped in create_page_tables because it may
>> - * clash with the 1:1 mapping.
>
> Since commit 9d267c049d92 ("xen/arm64: Rework the memory layout"), there
> is no chance that the fixmap will clash with the 1:1 mapping. So rather
> than introducing a new function, I would move the creation of the fixmap
> in create_pagetables().
>
Understood. I'll move the creation of the fixmap in create_pagetables().
> This would avoid the #ifdef CONFIG_HAS_FIXMAP in head.S.
>
>> + * Map the UART in the fixmap (when earlyprintk is used)
>> *
>> * Inputs:
>> - * x20: Physical offset
>> * x23: Early UART base physical address
>> *
>> * Clobbers x0 - x3
>> */
>> -ENTRY(setup_fixmap)
>> +ENTRY(setup_early_uart)
>> #ifdef CONFIG_EARLY_PRINTK
>> /* Add UART to the fixmap table */
>> ldr x0, =EARLY_UART_VIRTUAL_ADDRESS
>> create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3,
>> type=PT_DEV_L3
>> + /* Ensure any page table updates made above have occurred. */
>> + dsb nshst
>> +
>> + ret
>
> The 'ret' needs to be outside of the '#ifdef' block. But, in this case,
> I would prefer if we don't call setup_early_uart() when
> !CONFIG_EARLY_PRINTK in head.S
>
okay. I'll move the #ifdef to the caller in head.S.
>> #endif
>> +ENDPROC(setup_early_uart)
>> +
>> +/*
>> + * Map the fixmap table in the page tables.
>> + *
>> + * The fixmap cannot be mapped in create_page_tables because it may
>> + * clash with the 1:1 mapping.
>> + *
>> + * Clobbers x0 - x3
>> + */
>> +ENTRY(setup_fixmap)
>> /* Map fixmap into boot_second */
>> ldr x0, =FIXMAP_ADDR(0)
>> create_table_entry boot_second, xen_fixmap, x0, 2, x1, x2, x3
>
> Cheeers,
>