Re: [Xen-devel] [PATCH v3] xen/arm: Park CPUs with a MIDR different from the boot CPU.

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 15 Feb 2018, Julien Grall wrote:
> Xen does not properly support big.LITTLE platform. All vCPUs of a guest
> will always have the MIDR of the boot CPU (see arch_domain_create).
> At best the guest may see unreliable performance (vCPU switching between
> big and LITTLE), at worst the guest will become unreliable or insecure.
> 
> This is becoming more apparent with branch predictor hardening in Linux
> because they target a specific kind of CPUs and may not work on other
> CPUs.
> 
> For the time being, park any CPUs with a MDIR different from the boot
> CPU. This will be revisited in the future once Xen gains understanding
> of big.LITTLE.
> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2016-12/msg00826.html
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> Reviewed-by: Oleksandr Tyshchenkko <oleksandr_tyshchenko@xxxxxxxx>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Hi Julien,

This patch is fine. As discussed, I have a couple of patches that make
big.LITTLE a bit more usable, provided that the right cpu affinity is
specified for all domains.

I am going to take this patch as is, and add it at the beginning of my
little series. I'll update the warning and the docs in a separate patch.


> ---
> 
> We probably want to backport this as part of XSA-254. Using big.LITTLE
> on Xen has never been supported but we didn't make it clearly. This is
> becoming more apparent with code targeting specific CPUs.
> 
> Oleksandr, FIY, I've kept your reviewed-by despite changing the command line
> option.
> 
>     Changes in v3:
>         - Rename hmp_unsafe to hmp-unsafe.
>         - Add Jan's acked-by
>         - Add Oleksandr's reviewed-by
> 
>     Changes in v2:
>         - Add a command line option to override the default behavior.
> ---
>  docs/misc/xen-command-line.markdown | 10 ++++++++++
>  xen/arch/arm/smpboot.c              | 26 ++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/docs/misc/xen-command-line.markdown 
> b/docs/misc/xen-command-line.markdown
> index 79feba6bcd..7bd009f858 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1000,6 +1000,16 @@ supported only when compiled with XSM on x86.
>  
>  Control Xens use of the APEI Hardware Error Source Table, should one be 
> found.
>  
> +### hmp-unsafe (arm)
> +> `= <boolean>`
> +
> +> Default : `false`
> +
> +Say yes at your own risk if you want to enable heterogenous computing
> +(such as big.LITTLE). This may result to an unstable and insecure
> +platform. When the option is disabled (default), CPUs that are not
> +identical to the boot CPU will be parked and not used by Xen.
> +
>  ### hpetbroadcast
>  > `= <boolean>`
>  
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 1255185a9c..7ea4e41866 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -27,6 +27,7 @@
>  #include <xen/smp.h>
>  #include <xen/softirq.h>
>  #include <xen/timer.h>
> +#include <xen/warning.h>
>  #include <xen/irq.h>
>  #include <xen/console.h>
>  #include <asm/cpuerrata.h>
> @@ -69,6 +70,13 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, 
> cpu_sibling_mask);
>  /* representing HT and core siblings of each logical CPU */
>  DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_core_mask);
>  
> +/*
> + * By default non-boot CPUs not identical to the boot CPU will be
> + * parked.
> + */
> +static bool __read_mostly opt_hmp_unsafe = false;
> +boolean_param("hmp-unsafe", opt_hmp_unsafe);
> +
>  static void setup_cpu_sibling_map(int cpu)
>  {
>      if ( !zalloc_cpumask_var(&per_cpu(cpu_sibling_mask, cpu)) ||
> @@ -255,6 +263,9 @@ void __init smp_init_cpus(void)
>      else
>          acpi_smp_init_cpus();
>  
> +    if ( opt_hmp_unsafe )
> +        warning_add("WARNING: HMP COMPUTING HAS BEEN ENABLED.\n"
> +                    "It has implications on the security and stability of 
> the system.\n");
>  }
>  
>  int __init
> @@ -292,6 +303,21 @@ void start_secondary(unsigned long boot_phys_offset,
>  
>      init_traps();
>  
> +    /*
> +     * Currently Xen assumes the platform has only one kind of CPUs.
> +     * This assumption does not hold on big.LITTLE platform and may
> +     * result to instability and insecure platform. Better to park them
> +     * for now.
> +     */
> +    if ( !opt_hmp_unsafe &&
> +         current_cpu_data.midr.bits != boot_cpu_data.midr.bits )
> +    {
> +        printk(XENLOG_ERR "CPU%u MIDR (0x%x) does not match boot CPU MIDR 
> (0x%x).\n",
> +               smp_processor_id(), current_cpu_data.midr.bits,
> +               boot_cpu_data.midr.bits);
> +        stop_cpu();
> +    }
> +
>      mmu_init_secondary_cpu();
>  
>      gic_init_secondary_cpu();
> -- 
> 2.11.0
> 

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