Re: [PATCH 09/12] Revert "xen/arm: mm: Initialize page-tables earlier"

[Julien Grall <julien@xxxxxxx>]

Hi Carlo,

On 26/08/2022 13:51, Carlo Nonato wrote:
> From: Luca Miccio <lucmiccio@xxxxxxxxx>
>
> This reverts commit 3a5d341681af650825bbe3bee9be5d187da35080.
Usually, this indicates that this was a clean revert. IOW, there was no 
clash or modification necessary. Looking at the diff below, this doesn't 
look to be the case because you are also reverting f8c818848fa6 
"xen/arm: mm: Re-implement early_fdt_map() using map_pages_to_xen()" 
and introduce a new version of create_boot_mappings().
So I think the commit message/title should be reworded to explain this 
is not a clean revert and what extra changes were made.
But see below about re-introducing create_boot_mapping().

> The cache coloring support will be configurable within the Xen command line,
> but it will be initialized before the page-tables; this is necessary
> for coloring the hypervisor itself beacuse we will create a specific
> mapping for it that could be configured using some command line options.
> In order to parse all the needed information from the device tree, we
> need to revert the above commit and restore the previous order for
> page-tables initialization.
>
> Signed-off-by: Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
> Signed-off-by: Marco Solieri <marco.solieri@xxxxxxxxxxxxxxx>
> ---
>   xen/arch/arm/mm.c    | 33 ++++++++++++++++++++-------------
>   xen/arch/arm/setup.c |  4 ++--
>   2 files changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index b42cddb1b4..1afa02b4af 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -368,6 +368,17 @@ static inline lpae_t pte_of_xenaddr(vaddr_t va)
>       return mfn_to_xen_entry(maddr_to_mfn(ma), MT_NORMAL);
>   }
>   
> +static void __init create_boot_mappings(unsigned long virt_offset,
> +                                        mfn_t base_mfn)
> +{
> +    lpae_t pte;
> +
> +    pte = mfn_to_xen_entry(base_mfn, MT_NORMAL);
> +    write_pte(&boot_second[second_table_offset(virt_offset)], pte);
> +    flush_xen_tlb_local();
> +}
Please don't introduce a new function that create mappings. All mappings 
should be done using map_pages_to_xen(). Looking at the implementation, 
it should be usable with the following diff:
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index c81c706c8b23..78afb8eb0ec1 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1104,7 +1104,7 @@ static int xen_pt_update(unsigned long virt,
      *
      * XXX: Add a check.
      */
-    const mfn_t root = virt_to_mfn(THIS_CPU_PGTABLE);
+    const mfn_t root = maddr_to_mfn(READ_SYSREG64(TTBR0_EL2));

     /*
      * The hardware was configured to forbid mapping both writeable and

With that there is no change required in early_fdt_map() and ...

>   
> +    /* ... DTB */
> +    pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START)];
> +    xen_second[second_table_offset(BOOT_FDT_VIRT_START)] = pte;
> +    pte = boot_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)];
> +    xen_second[second_table_offset(BOOT_FDT_VIRT_START + SZ_2M)] = pte;
> +
... rather than copying the 2 entries, you could call early_fdt_map() 
after the switch. The advantage is it will avoid to hardoded more 
page-table entries.
As part of my switch_ttbr() rework, I am planning to re-introduce 
relocation (at least for testing). So I will include the changes I 
mention above in my series.
Cheers,

--
Julien Grall