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

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

Hi

On Wed, Feb 14, 2018 at 4:47 PM, Oleksandr Tyshchenko
<olekstysh@xxxxxxxxx> wrote:
> Hi, Julien.
>
> On Wed, Feb 14, 2018 at 4:17 PM, Julien Grall <julien.grall@xxxxxxx> 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>
>>
>> ---
>>
>> 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.
>>
>>     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..cf5997b8db 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..5c05cadb0a 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;
I think, you can remove the initialization of variable.

>> +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
>
> Thanks for leaving an option to enable hmp back. We understand the
> possible risks, but we are playing with big and LITTLE cores for test
> purposes
> and without such option we will have to revert the patch to allow
> LITTLE cores to up and running.
>
> --
> Regards,
>
> Oleksandr Tyshchenko

Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>

-- 
Regards,

Oleksandr Tyshchenko

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