| Message ID | 20180214141759.7573-1-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Xen-devel,v2] xen/arm: Park CPUs with a MIDR different from the boot CPU. | expand |
Hi, Julien. On Wed, Feb 14, 2018 at 4:17 PM, Julien Grall <julien.grall@arm.com> 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@arm.com> > > --- > > 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; > +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@lists.xenproject.org > 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.
Hi On Wed, Feb 14, 2018 at 4:47 PM, Oleksandr Tyshchenko <olekstysh@gmail.com> wrote: > Hi, Julien. > > On Wed, Feb 14, 2018 at 4:17 PM, Julien Grall <julien.grall@arm.com> 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@arm.com> >> >> --- >> >> 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@lists.xenproject.org >> 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@epam.com>
>>> On 14.02.18 at 15:17, <julien.grall@arm.com> wrote: > --- 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) Could I talk you into using hmp-unsafe instead? As said elsewhere, dashes (as being easier to type) should be preferred over underscores (which make option strings needlessly resemble identifier names). Other than that the doc addition is Acked-by: Jan Beulich <jbeulich@suse.com> if that's needed in the first place. Jan
Hi Jan, On 15/02/18 09:03, Jan Beulich wrote: >>>> On 14.02.18 at 15:17, <julien.grall@arm.com> wrote: >> --- 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) > > Could I talk you into using hmp-unsafe instead? As said elsewhere, > dashes (as being easier to type) should be preferred over underscores > (which make option strings needlessly resemble identifier names). I don't mind on the naming. I will update it. > Other > than that the doc addition is > Acked-by: Jan Beulich <jbeulich@suse.com> > if that's needed in the first place. I have CCed you because docs falls under "THE REST". Not sure what is the policy here thought. Anyway, thank you for the acked. Cheers, > > Jan >
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; +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();
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@arm.com> --- 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(+)