Re: [PATCH v2 05/10] xen/arm: Setup virtual paging for non-boot CPUs on hotplug/resume

[Julien Grall <julien.grall@arm.com>]

Hi Mirela,

On 20/04/18 13:25, Mirela Simonovic wrote:
> In existing code the paging for non-boot CPUs is setup only on boot. The
> setup is triggered from start_xen() after all CPUs are brought online.
> In other words, the initialization of VTCR_EL2 register is done out of the
> cpu_up/start_secondary() control flow. However, the cpu_up flow is also used
> to hotplug non-boot CPUs on resume from suspend to RAM state, in which case
> the virtual paging will not be configured.
> With this patch the setting of paging is triggered from start_secondary()
> function if the current system state is not boot. This way, the paging
> for secondary CPUs will be setup in non-boot scenarios as well, while the
> setup in boot scenario remains unchanged.
> It is assumed here that after the system completed the boot, CPUs that
> execute start_secondary() were booted as well when the Xen itself was
> booted. According to this assumption non-boot CPUs will always be compliant
> with the VTCR_EL2 value that was selected by Xen on boot.
> Currently, these in no mechanism to trigger hotplugging of a CPU. This
> will be added with the suspend to RAM support for ARM, where the hotplug
> of non-boot CPUs will be triggered via enable_nonboot_cpus() call.
> 
> Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
> 
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
> Changes in v2:
> -Fix commit message
> -Save configured VTCR_EL2 value into static variable that will be used
>   by non-boot CPUs on hotplug
> -Add setup_virt_paging_secondary() and invoke it from start_secondary()
>   if that CPU has to setup virtual paging (if the system state is not boot)
> ---
>   xen/arch/arm/p2m.c        | 13 ++++++++++++-
>   xen/arch/arm/smpboot.c    |  3 +++
>   xen/include/asm-arm/p2m.h |  3 +++
>   3 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index d43c3aa896..9bb62c13cd 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1451,13 +1451,21 @@ err:
>       return page;
>   }
>   
> -static void __init setup_virt_paging_one(void *data)
> +static void setup_virt_paging_one(void *data)
>   {
>       unsigned long val = (unsigned long)data;
>       WRITE_SYSREG32(val, VTCR_EL2);
>       isb();
>   }
>   
> +/* VTCR value to be configured by all CPUs. Set only once by the boot CPU */
> +static unsigned long __read_mostly vtcr_value;

VTCR is a register, so the type should be represented in term of bits 
(i.e uint*_t).

> +
> +void setup_virt_paging_secondary(void)
> +{
> +    setup_virt_paging_one((void *)vtcr_value);

That's fairly ugly. Is there any way to rework the interface? For 
instance, because you have a static variable which contain the VTCR, you 
could just use the variable in setup_virt_paging one.

> +}
> +
>   void __init setup_virt_paging(void)
>   {
>       /* Setup Stage 2 address translation */
> @@ -1540,6 +1548,9 @@ void __init setup_virt_paging(void)
>       BUG_ON( P2M_ROOT_LEVEL == 0 && P2M_ROOT_ORDER > 0 );
>       setup_virt_paging_one((void *)val);
>       smp_call_function(setup_virt_paging_one, (void *)val, 1);
> +
> +    /* Save configured value (to be used later for secondary CPUs hotplug) */
> +    vtcr_value = val;
>   }
>   
>   /*
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 38b665a6d2..abc642804f 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -358,6 +358,9 @@ void start_secondary(unsigned long boot_phys_offset,
>       local_irq_enable();
>       local_abort_enable();
>   
> +    if ( system_state != SYS_STATE_boot )
> +        setup_virt_paging_secondary();
I think this code needs some documentation. So people understand why you 
only call setup_virt_paging_secondary() after boot. But is there any 
reason to not use a notifier (see notify_cpu_starting)? This would avoid 
yet another export.

> +
>       check_local_cpu_errata();
>   
>       printk(XENLOG_DEBUG "CPU %u booted.\n", smp_processor_id());
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 8823707c17..85b66a1196 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -153,6 +153,9 @@ void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
>   /* Second stage paging setup, to be called on all CPUs */
>   void setup_virt_paging(void);
>   
> +/* To be called by secondary CPU on hotplug */
> +void setup_virt_paging_secondary(void);
> +
>   /* Init the datastructures for later use by the p2m code */
>   int p2m_init(struct domain *d);
>   
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel