Re: [PATCH v1 repost 2/4] arm/smpboot: Move smp_up_cpu to a new section .data.idmap

[Julien Grall <julien@xxxxxxx>]

On 17/01/2024 08:44, 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, smp_up_cpu is used by secondary CPUs to wait there turn for
> s/there/their ?
Yes. I will fix it on commit.

> > booting 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.
> >
> > Move smp_up_cpu to the recently create section .data.idmap. The idmap is
> s/create/created
>
> > currently part of the text section and therefore will be mapped read-onl
> s/onl/only
>
> > executable. This means that we need to temporarily remap
> > smp_up_cpu in order to update it.
> >
> > Introduce a new function set_smp_up_cpu() for this purpose so the code
> > is not duplicated between when opening and closing the gate.
> >
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
>
> > ---
> >   xen/arch/arm/smpboot.c | 36 +++++++++++++++++++++++++++++++-----
> >   1 file changed, 31 insertions(+), 5 deletions(-)
> >
> > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> > index 7110bc11fc05..8d508a1bb258 100644
> > --- a/xen/arch/arm/smpboot.c
> > +++ b/xen/arch/arm/smpboot.c
> > @@ -29,6 +29,10 @@
> >   #include <asm/psci.h>
> >   #include <asm/acpi.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))
> > +
> >   cpumask_t cpu_online_map;
> >   cpumask_t cpu_present_map;
> >   cpumask_t cpu_possible_map;
> > @@ -56,7 +60,7 @@ struct init_info init_data =
> >   };
> >
> >   /* Shared state for coordinating CPU bringup */
> > -unsigned long smp_up_cpu = MPIDR_INVALID;
> > +unsigned long __section(".data.idmap") smp_up_cpu = MPIDR_INVALID;
> >   /* Shared state for coordinating CPU teardown */
> >   static bool cpu_is_dead;
> >
> > @@ -429,6 +433,28 @@ void stop_cpu(void)
> >           wfi();
> >   }
> >
> > +static void set_smp_up_cpu(unsigned long mpidr)
> > +{
> > +    /*
> > +     * smp_up_cpu is part of the identity mapping which is read-only. So
> > +     * We need to re-map the region so it can be updated.
> > +     */
> > +    void *ptr = map_domain_page(virt_to_mfn(&smp_up_cpu));
> > +
> > +    ptr += PAGE_OFFSET(&smp_up_cpu);
> > +
> > +    *(unsigned long *)ptr = mpidr;
> > +
> > +    /*
> > +     * init_ttbr will be accessed with the MMU off, so ensure the update
> smp_up_cpu instead of init_ttbr
Doh. I just copied/pasted the code from init_ttbr. I will update it.

BTW, I thought about trying to consolidate the code between 
set_init_ttbr() and set_smp_up_cpu() but it didn't seem to be worth it.
Cheers,

--
Julien Grall