Re: [PATCH 3/3] [DO NOT COMMIT] xen/arm: Create a trampoline for secondary boot CPUs

From: Carlo Nonato <carlo.nonato@minervasys.tech>
To: Julien Grall <julien@xen.org>
Cc: xen-devel@lists.xenproject.org, Julien Grall <jgrall@amazon.com>,
	 Stefano Stabellini <sstabellini@kernel.org>,
	Bertrand Marquis <bertrand.marquis@arm.com>,
	 Michal Orzel <michal.orzel@amd.com>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH 3/3] [DO NOT COMMIT] xen/arm: Create a trampoline for secondary boot CPUs
Date: Tue, 16 Jan 2024 15:24:15 +0100	[thread overview]
Message-ID: <CAG+AhRWrN=Y4ZT57GjYMoX=7LPe0_3Xg_xaSnA4DY6CDHWPK0Q@mail.gmail.com> (raw)
In-Reply-To: <20240116115509.77545-4-julien@xen.org>

Hi Julien,

On Tue, Jan 16, 2024 at 12:55 PM Julien Grall <julien@xen.org> wrote:
>
> From: Julien Grall <jgrall@amazon.com>
>
> In order to confirm the early boot code is self-contained, allocate a
> separate trampoline region for secondary to boot from it.
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
>  xen/arch/arm/arm64/mmu/mm.c |  7 +++++++
>  xen/arch/arm/mmu/smpboot.c  |  4 +++-
>  xen/arch/arm/psci.c         |  5 ++++-
>  xen/arch/arm/smpboot.c      | 15 ++++++++++++++-
>  4 files changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/mmu/mm.c b/xen/arch/arm/arm64/mmu/mm.c
> index d2651c948698..3c4988dc75d1 100644
> --- a/xen/arch/arm/arm64/mmu/mm.c
> +++ b/xen/arch/arm/arm64/mmu/mm.c
> @@ -110,11 +110,18 @@ void __init arch_setup_page_tables(void)
>      prepare_runtime_identity_mapping();
>  }
>
> +extern mfn_t trampoline_start;
> +
>  void update_identity_mapping(bool enable)
>  {
>      paddr_t id_addr = virt_to_maddr(_start);
>      int rc;
>
> +    if ( !mfn_eq(trampoline_start, INVALID_MFN) )
> +    {
> +        id_addr = mfn_to_maddr(trampoline_start);
> +    }
> +
>      if ( enable )
>          rc = map_pages_to_xen(id_addr, maddr_to_mfn(id_addr), 1,
>                                PAGE_HYPERVISOR_RX);
> diff --git a/xen/arch/arm/mmu/smpboot.c b/xen/arch/arm/mmu/smpboot.c
> index f1cf9252710c..d768dfe065a5 100644
> --- a/xen/arch/arm/mmu/smpboot.c
> +++ b/xen/arch/arm/mmu/smpboot.c
> @@ -72,13 +72,15 @@ static void clear_boot_pagetables(void)
>      clear_table(boot_third);
>  }
>
> +extern mfn_t trampoline_start;
> +
>  static void set_init_ttbr(lpae_t *root)

Isn't this function not present in the patch series?

>  {
>      /*
>       * 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
>       */
> -    void *ptr = map_domain_page(virt_to_mfn(&init_ttbr));
> +    void *ptr = map_domain_page(trampoline_start);
>
>      ptr += PAGE_OFFSET(&init_ttbr);
>
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> index 695d2fa1f1b5..a00574d559d6 100644
> --- a/xen/arch/arm/psci.c
> +++ b/xen/arch/arm/psci.c
> @@ -36,11 +36,14 @@ static uint32_t psci_cpu_on_nr;
>
>  #define PSCI_RET(res)   ((int32_t)(res).a0)
>
> +extern mfn_t trampoline_start;
> +
>  int call_psci_cpu_on(int cpu)
>  {
>      struct arm_smccc_res res;
>
> -    arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary),
> +    arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu),
> +                  mfn_to_maddr(trampoline_start) + PAGE_OFFSET(init_secondary),
>                    &res);
>
>      return PSCI_RET(res);
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 8d508a1bb258..ef84b73ebd46 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -293,10 +293,13 @@ unsigned int __init smp_get_max_cpus(void)
>      return cpus;
>  }
>
> +mfn_t trampoline_start = INVALID_MFN_INITIALIZER;
> +
>  void __init
>  smp_prepare_cpus(void)
>  {
>      int rc;
> +    void *trampoline;
>
>      cpumask_copy(&cpu_present_map, &cpu_possible_map);
>
> @@ -304,6 +307,16 @@ smp_prepare_cpus(void)
>      if ( rc )
>          panic("Unable to allocate CPU sibling/core maps\n");
>
> +    /* Create a trampoline to confirm early boot code is self-contained */
> +    trampoline = alloc_xenheap_page();
> +    BUG_ON(!trampoline);
> +
> +    memcpy(trampoline, _start, PAGE_SIZE);
> +    clean_dcache_va_range(trampoline, PAGE_SIZE);
> +    invalidate_icache();
> +
> +    printk("Trampoline 0x%lx\n", virt_to_maddr(trampoline));
> +    trampoline_start = virt_to_mfn(trampoline);
>  }
>
>  /* Boot the current CPU */
> @@ -439,7 +452,7 @@ 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));
> +    void *ptr = map_domain_page(trampoline_start);
>
>      ptr += PAGE_OFFSET(&smp_up_cpu);
>
> --
> 2.40.1
>

Thank you very much for your help.