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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Julien,

On Sat, Sep 10, 2022 at 4:29 PM Julien Grall <julien@xxxxxxx> wrote:
>
> 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.

Thanks for the diff and the suggestions. I just tested it and it works
properly! Nice.

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

Thanks.

- Carlo Nonato