Re: [PATCH v1 repost 1/4] arm/mmu: Move init_ttbr to a new section .data.idmap

[Julien Grall <julien@xxxxxxx>]

On 17/01/2024 08:30, Michal Orzel wrote:
> Hi Julien,
Hi Michal,

> On 16/01/2024 15:37, Julien Grall wrote:
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > With the upcoming work to color Xen, the binary will not be anymore
> > physically contiguous. This will be a problem during boot as the
> > assembly code will need to work out where each piece of Xen reside.
> >
> > An easy way to solve the issue is to have all code/data accessed
> > by the secondary CPUs while the MMU is off within a single page.
> >
> > Right now, init_ttbr is used by secondary CPUs to find there page-tables
> > before the MMU is on. Yet it is currently in .data which is unlikely
> > to be within the same page as the rest of the idmap.
> >
> > Create a new section .data.idmap that will be used for variables
> > accessed by the early boot code. The first one is init_ttbr.
> >
> > The idmap is currently part of the text section and therefore will
> > be mapped read-only executable. This means that we need to temporarily
> > remap init_ttbr in order to update it.
> >
> > Introduce a new function set_init_ttbr() for this purpose so the code
> > is not duplicated between arm64 and arm32.
> >
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
>
> with ...
>
> > ---
> >   xen/arch/arm/mmu/smpboot.c | 34 +++++++++++++++++++++++++++++-----
> >   xen/arch/arm/xen.lds.S     |  1 +
> >   2 files changed, 30 insertions(+), 5 deletions(-)
> >
> > diff --git a/xen/arch/arm/mmu/smpboot.c b/xen/arch/arm/mmu/smpboot.c
> > index b6fc0aae07f1..f1cf9252710c 100644
> > --- a/xen/arch/arm/mmu/smpboot.c
> > +++ b/xen/arch/arm/mmu/smpboot.c
> > @@ -9,6 +9,10 @@
> >
> >   #include <asm/setup.h>
> >
> > +/* Override macros from asm/page.h to make them work with mfn_t */
> > +#undef virt_to_mfn
> > +#define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
> > +
> >   /*
> >    * Static start-of-day pagetables that we use before the allocators
> >    * are up. These are used by all CPUs during bringup before switching
> > @@ -44,7 +48,7 @@ DEFINE_BOOT_PAGE_TABLE(boot_second);
> >   DEFINE_BOOT_PAGE_TABLES(boot_third, XEN_NR_ENTRIES(2));
> >
> >   /* Non-boot CPUs use this to find the correct pagetables. */
> > -uint64_t init_ttbr;
> > +uint64_t __section(".data.idmap") init_ttbr;
> Do we need to keep the declaration in mmu/mm.h? This variable is only used in 
> this file
> and in assembly, so maybe better to drop declaration and use asmlinkage instead?
I don't see the problem of keeping the declaration in mmu/mm.h. In any 
case, this seems to be unrelated to this patch.
> >   /* Clear a translation table and clean & invalidate the cache */
> >   static void clear_table(void *table)
> > @@ -68,6 +72,27 @@ static void clear_boot_pagetables(void)
> >       clear_table(boot_third);
> >   }
> >
> > +static void set_init_ttbr(lpae_t *root)
> > +{
> > +    /*
> > +     * init_ttbr is part of the identity mapping which is read-only. So
> > +     * We need to re-map the region so it can be updated
> Would you mind fixing s/So We/So we/ and add a full stop after last sentence?
I can do that.

Cheers,

--
Julien Grall