Re: [PATCH v4 06/13] xen/arm64: Move setup_fixmap() to create_page_tables()

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> On Aug 9, 2023, at 20:28, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Henry,
> 
> Title: NIT: It is more a fold rather than move.

You are correct, using “fold” here is more appropriate.

> 
> On 01/08/2023 04:44, Henry Wang wrote:
>> For the
>> future MPU support work, the early UART mapping could then be moved
>> in prepare_early_mappings().
> 
> I would drop this sentence as this is more related to the future 
> implementation of MPU rather than this patch itself.

Sure, v5 will drop this.

> 
>> No functional change intended.
>> [1] 
>> https://lore.kernel.org/xen-devel/78862bb8-fd7f-5a51-a7ae-3c5b5998ed80@xxxxxxx/
>> Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
>> ---
>> v4:
>> - Rework "[v3,12/52] xen/mmu: extract early uart mapping from setup_fixmap"
>> ---
>>  xen/arch/arm/arm64/head.S     |  1 -
>>  xen/arch/arm/arm64/mmu/head.S | 50 ++++++++++++-----------------------
>>  2 files changed, 17 insertions(+), 34 deletions(-)
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index e4f579a48e..56f68a8e37 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -275,7 +275,6 @@ real_start_efi:
>>          b     enable_boot_cpu_mm
>>    primary_switched:
>> -        bl    setup_fixmap
>>  #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 b7c3dd423a..6bd94c3a45 100644
>> --- a/xen/arch/arm/arm64/mmu/head.S
>> +++ b/xen/arch/arm/arm64/mmu/head.S
> 
> The second paragraph in create_page_tables() now needs to be dropped.

Thanks for finding this! Yes this should be dropped.

> 
>> @@ -231,6 +231,23 @@ link_from_second_id:
>>          create_table_entry boot_second_id, boot_third_id, x19, 2, x0, x1, x2
>>  link_from_third_id:
>>          create_mapping_entry boot_third_id, x19, x19, x0, x1, x2
>> +
>> +#ifdef CONFIG_EARLY_PRINTK
>> +        /* Add UART to the fixmap table */
>> +        ldr   x0, =EARLY_UART_VIRTUAL_ADDRESS
>> +        /* x23: Early UART base physical address */
>> +        create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3, type=PT_DEV_L3
>> +#endif
>> +        /* Map fixmap into boot_second */
>> +        ldr   x0, =FIXMAP_ADDR(0)
>> +        create_table_entry boot_second, xen_fixmap, x0, 2, x1, x2, x3
>> +        /* Ensure any page table updates made above have occurred. */
>> +        dsb   nshst
>> +        /*
>> +         * The fixmap area will be used soon after. So ensure no hardware
>> +         * translation happens before the dsb completes.
>> +         */
>> +        isb
> 
> The DSB/ISB is not necessary anymore as you are not modifying live 
> page-tables. So I would prefer if this is removed.

Sure.

Kind regards,
Henry

> 
> Cheers,
> 
> -- 
> Julien Grall