diff mbox series

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

Message ID 1519946795-7191-1-git-send-email-sstabellini@kernel.org
State Superseded
Headers show
Series [Xen-devel,v3,1/6] xen/arm: Park CPUs with a MIDR different from the boot CPU. | expand

Commit Message

Stefano Stabellini March 1, 2018, 11:26 p.m. UTC
From: Julien Grall <julien.grall@arm.com>

From: Julien Grall <julien.grall@arm.com>

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>
Reviewed-by: Oleksandr Tyshchenkko <oleksandr_tyshchenko@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
 docs/misc/xen-command-line.markdown | 10 ++++++++++
 xen/arch/arm/smpboot.c              | 26 ++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

Comments

Julien Grall March 2, 2018, 10:06 a.m. UTC | #1
Hi,

Something is wrong with the threading of this series. Most of the 
patches are threaded below #1 rather than #0.

On 01/03/18 23:26, Stefano Stabellini wrote:
> On big.LITTLE systems not all cores have the same MIDR. Instead of
> storing only one VPIDR per domain, initialize it to the value of the
> MIDR of the pCPU where the vCPU will run.
> 
> This way, assuming that the vCPU has been created with the right pCPU
> affinity, the guest will be able to read the right VPIDR value, matching
> the one of the physical cpu.
> 
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>

Reviewed-by: Julien Grall <julien.grall@arm.com>

Cheers,

> 
> ---
> 
> Changes in v3:
> - improve commit message
> - do not store vpidr in struct vcpu
> 
> Changes in v2:
> - remove warning message
> - make vpidr per vcpu
> ---
>   xen/arch/arm/domain.c        | 8 ++++----
>   xen/arch/arm/vcpreg.c        | 4 ++--
>   xen/include/asm-arm/domain.h | 3 ---
>   3 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 5e76809..545bbf6 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -172,6 +172,8 @@ static void ctxt_switch_from(struct vcpu *p)
>   
>   static void ctxt_switch_to(struct vcpu *n)
>   {
> +    uint32_t vpidr;
> +
>       /* When the idle VCPU is running, Xen will always stay in hypervisor
>        * mode. Therefore we don't need to restore the context of an idle VCPU.
>        */
> @@ -180,7 +182,8 @@ static void ctxt_switch_to(struct vcpu *n)
>   
>       p2m_restore_state(n);
>   
> -    WRITE_SYSREG32(n->domain->arch.vpidr, VPIDR_EL2);
> +    vpidr = READ_SYSREG32(MIDR_EL1);
> +    WRITE_SYSREG32(vpidr, VPIDR_EL2);
>       WRITE_SYSREG(n->arch.vmpidr, VMPIDR_EL2);
>   
>       /* VGIC */
> @@ -595,9 +598,6 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>       if ( (d->shared_info = alloc_xenheap_pages(0, 0)) == NULL )
>           goto fail;
>   
> -    /* Default the virtual ID to match the physical */
> -    d->arch.vpidr = boot_cpu_data.midr.bits;
> -
>       clear_page(d->shared_info);
>       share_xen_page_with_guest(
>           virt_to_page(d->shared_info), d, XENSHARE_writable);
> diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
> index e363183..b04d996 100644
> --- a/xen/arch/arm/vcpreg.c
> +++ b/xen/arch/arm/vcpreg.c
> @@ -230,7 +230,6 @@ void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
>   {
>       const struct hsr_cp32 cp32 = hsr.cp32;
>       int regidx = cp32.reg;
> -    struct domain *d = current->domain;
>   
>       if ( !check_conditional_instr(regs, hsr) )
>       {
> @@ -295,7 +294,8 @@ void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
>            *  - Variant and Revision bits match MDIR
>            */
>           val = (1 << 24) | (5 << 16);
> -        val |= ((d->arch.vpidr >> 20) & 0xf) | (d->arch.vpidr & 0xf);
> +        val |= ((current_cpu_data.midr.bits >> 20) & 0xf) |
> +                (current_cpu_data.midr.bits & 0xf);
>           set_user_reg(regs, regidx, val);
>   
>           break;
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 4fe189b..0dd8c95 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -63,9 +63,6 @@ struct arch_domain
>           RELMEM_done,
>       } relmem;
>   
> -    /* Virtual CPUID */
> -    uint32_t vpidr;
> -
>       struct {
>           uint64_t offset;
>       } phys_timer_base;
>
Julien Grall March 2, 2018, 10:08 a.m. UTC | #2
On 01/03/18 23:26, Stefano Stabellini wrote:
> Introduce a new document about big.LITTLE and update the documentation
> of hmp-unsafe.
> 
> Also update the warning messages to point users to the docs.
> 
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>

Acked-by: Julien Grall <julien.grall@arm.com>

> CC: jbeulich@suse.com
> CC: konrad.wilk@oracle.com
> CC: tim@xen.org
> CC: wei.liu2@citrix.com
> CC: andrew.cooper3@citrix.com
> CC: George.Dunlap@eu.citrix.com
> CC: ian.jackson@eu.citrix.com
> 
> ---
> 
> Changes in v3:
> - split warning messages to be under 72 chars
> 
> Changes in v2:
> - add a separate doc for big.LITTLE
> - improve the warning message
> ---
>   docs/misc/arm/big.LITTLE.txt        | 46 +++++++++++++++++++++++++++++++++++++
>   docs/misc/xen-command-line.markdown |  7 +++++-
>   xen/arch/arm/smpboot.c              | 11 +++++----
>   3 files changed, 59 insertions(+), 5 deletions(-)
>   create mode 100644 docs/misc/arm/big.LITTLE.txt
> 
> diff --git a/docs/misc/arm/big.LITTLE.txt b/docs/misc/arm/big.LITTLE.txt
> new file mode 100644
> index 0000000..b6ea1c9
> --- /dev/null
> +++ b/docs/misc/arm/big.LITTLE.txt
> @@ -0,0 +1,46 @@
> +big.LITTLE is a form of heterogeneous computing that comes with two
> +types of general purpose cpu cores: big cores, more powerful and with a
> +higher power consumption rate, and LITTLE cores, less powerful and
> +cheaper to run. For example, Cortex A53 and Cortex A57 cpus. Typically,
> +big cores are only recommended for burst activity, especially in
> +battery powered environments. Please note that Xen doesn't not use any
> +board specific power management techniques at the moment, it only uses
> +WFI. It is recommended to check the vendor's big.LITTLE and power
> +management documentation before using it in a Xen environment.
> +
> +
> +big and LITTLE cores are fully compatible in terms of instruction sets,
> +but can differ in many subtle ways. For example, their cacheline sizes
> +might differ. For this reason, vcpu migration between big and LITTLE
> +cores can lead to data corruptions.
> +
> +Today, the Xen scheduler does not have support for big.LITTLE,
> +therefore, it might unknowingly move any vcpus between big and LITTLE
> +cores, potentially leading to breakages. To avoid this kind of issues,
> +at boot time Xen disables all cpus that differ from the boot cpu.
> +
> +
> +Expert users can enable all big.LITTLE cores by passing hmp-unsafe=true
> +to the Xen command line [1]. Given the lack of big.LITTLE support in the
> +scheduler, it is only safe if the cpu affinity of all domains is
> +manually specified, so that the scheduler is not allowed to switch a
> +vcpu from big to LITTLE or vice versa.
> +
> +In the case of dom0, dom0_vcpus_pin needs to be added to the Xen command
> +line options [1]. For DomUs, the `cpus' option should be added to all VM
> +config files [2].
> +
> +For example, if the first 4 cpus are big and the last 4 are LITTLE, the
> +following options run all domain vcpus on either big or LITTLE cores
> +(not both):
> +
> +  cpus = "0-3"
> +  cpus = "4-7"
> +
> +The following option runs one domain vcpu as big and one as LITTLE:
> +
> +  cpus = ["0-3", "4-7"]
> +
> +
> +[1] docs/misc/xen-command-line.markdown
> +[2] docs/man/xl.cfg.pod.5
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index 7b80119..4391b75 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -992,7 +992,12 @@ Control Xens use of the APEI Hardware Error Source Table, should one be found.
>   
>   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
> +platform, unless you manually specify the cpu affinity of all domains so
> +that all vcpus are scheduled on the same class of pcpus (big or LITTLE
> +but not both). vcpu migration between big cores and LITTLE cores is not
> +supported. See docs/misc/arm/big.LITTLE.txt for more information.
> +
> +When the hmp-unsafe option is disabled (default), CPUs that are not
>   identical to the boot CPU will be parked and not used by Xen.
>   
>   ### hpetbroadcast
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 122c0b5..04efb33 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -266,7 +266,8 @@ void __init smp_init_cpus(void)
>   
>       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");
> +                    "It has implications on the security and stability of the system,\n"
> +                    "unless the cpu affinity of all domains is specified.\n");
>   }
>   
>   int __init
> @@ -308,13 +309,15 @@ void start_secondary(unsigned long boot_phys_offset,
>       /*
>        * 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.
> +     * result to instability and insecure platform (unless cpu affinity
> +     * is manually specified for all domains). 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",
> +        printk(XENLOG_ERR "CPU%u MIDR (0x%x) does not match boot CPU MIDR (0x%x),\n"
> +               "disable cpu (see big.LITTLE.txt under docs/).\n",
>                  smp_processor_id(), current_cpu_data.midr.bits,
>                  boot_cpu_data.midr.bits);
>           stop_cpu();
>
Julien Grall March 2, 2018, 10:15 a.m. UTC | #3
Hi Stefano,

On 01/03/18 23:26, Stefano Stabellini wrote:
> Even different cpus in big.LITTLE systems are expected to have the same
> cacheline size. Unless the minimum of all cacheline sizes is used across
> all cpu cores, cache coherency protocols can go wrong. Instead, for
> now, just disable any cpu with a different cacheline size.
> 
> This check is not covered by the hmp-unsafe option, because even with
> the correct scheduling and vcpu pinning in place, the system breaks if
> cacheline sizes differ across cores. We don't believe it is a problem
> for most big.LITTLE systems.
> 
> This patch moves the implementation of setup_cache to a static inline,
> still setting cacheline_bytes at the beginning of start_xen as before.
> 
> In start_secondary we check that the cacheline sizes match, otherwise we
> disable the cpu.

I am afraid that this commit message is only valid after "xen/arm: Read 
the cacheline from CTR register".

What you effectively check in that patch is the D-cache level 1 line 
size is equal on every CPU. You could rewrite the commit message to 
reflect that, but then people may wonder why you impose such restriction 
on Xen? So it would really make sense to fix the way to read the 
D-cacheline size first.

> 
> Suggested-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> ---
> Changes in v3:
> - new patch
> 
> ---
> Interestingly I couldn't find a better way in C89 to printk a size_t
> than casting it to unsigned long.

You can use %zu.

> ---
>   xen/arch/arm/setup.c       | 15 +--------------
>   xen/arch/arm/smpboot.c     |  8 ++++++++
>   xen/include/asm-arm/page.h | 12 ++++++++++++
>   3 files changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 032a6a8..b5f4c3a 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -682,19 +682,6 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
>   
>   size_t __read_mostly cacheline_bytes;
>   
> -/* Very early check of the CPU cache properties */
> -void __init setup_cache(void)
> -{
> -    uint32_t ccsid;
> -
> -    /* Read the cache size ID register for the level-0 data cache */
> -    WRITE_SYSREG32(0, CSSELR_EL1);
> -    ccsid = READ_SYSREG32(CCSIDR_EL1);
> -
> -    /* Low 3 bits are log2(cacheline size in words) - 2. */
> -    cacheline_bytes = 1U << (4 + (ccsid & 0x7));
> -}
> -
>   /* C entry point for boot CPU */
>   void __init start_xen(unsigned long boot_phys_offset,
>                         unsigned long fdt_paddr,
> @@ -708,7 +695,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>       struct domain *dom0;
>       struct xen_arch_domainconfig config;
>   
> -    setup_cache();
> +    cacheline_bytes = read_cacheline_size();
>   
>       percpu_init_areas();
>       set_processor_id(0); /* needed early, for smp_processor_id() */
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 04efb33..153572e 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -323,6 +323,14 @@ void start_secondary(unsigned long boot_phys_offset,
>           stop_cpu();
>       }
>   
> +    if ( cacheline_bytes != read_cacheline_size() )
> +    {
> +        printk(XENLOG_ERR "CPU%u cacheline size (%lu) does not match the boot CPU (%lu)\n",
> +               smp_processor_id(), (unsigned long) read_cacheline_size(),
> +               (unsigned long) cacheline_bytes);
> +        stop_cpu();
> +    }
> +
>       mmu_init_secondary_cpu();
>   
>       gic_init_secondary_cpu();
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index d948250..9fbf232 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -138,6 +138,18 @@ extern size_t cacheline_bytes;
>   
>   #define copy_page(dp, sp) memcpy(dp, sp, PAGE_SIZE)
>   
> +static inline size_t read_cacheline_size(void)
> +{
> +    uint32_t ccsid;
> +
> +    /* Read the cache size ID register for the level-0 data cache */
> +    WRITE_SYSREG32(0, CSSELR_EL1);
> +    ccsid = READ_SYSREG32(CCSIDR_EL1);
> +
> +    /* Low 3 bits are log2(cacheline size in words) - 2. */
> +    return (size_t) (1U << (4 + (ccsid & 0x7)));
> +}
> +
>   /* Functions for flushing medium-sized areas.
>    * if 'range' is large enough we might want to use model-specific
>    * full-cache flushes. */
>
Stefano Stabellini March 2, 2018, 6:26 p.m. UTC | #4
On Fri, 2 Mar 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 01/03/18 23:26, Stefano Stabellini wrote:
> > Even different cpus in big.LITTLE systems are expected to have the same
> > cacheline size. Unless the minimum of all cacheline sizes is used across
> > all cpu cores, cache coherency protocols can go wrong. Instead, for
> > now, just disable any cpu with a different cacheline size.
> > 
> > This check is not covered by the hmp-unsafe option, because even with
> > the correct scheduling and vcpu pinning in place, the system breaks if
> > cacheline sizes differ across cores. We don't believe it is a problem
> > for most big.LITTLE systems.
> > 
> > This patch moves the implementation of setup_cache to a static inline,
> > still setting cacheline_bytes at the beginning of start_xen as before.
> > 
> > In start_secondary we check that the cacheline sizes match, otherwise we
> > disable the cpu.
> 
> I am afraid that this commit message is only valid after "xen/arm: Read the
> cacheline from CTR register".

I forgot to update the commit message, I'll fix.


> What you effectively check in that patch is the D-cache level 1 line size is
> equal on every CPU. You could rewrite the commit message to reflect that, but
> then people may wonder why you impose such restriction on Xen? So it would
> really make sense to fix the way to read the D-cacheline size first.

Yes, I understand. I'll reshuffle. For simplicity I'll make that patch
part of this series as first patch, although we both understand that
conceptually they are separate.


> > 
> > Suggested-by: Julien Grall <julien.grall@arm.com>
> > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> > 
> > ---
> > Changes in v3:
> > - new patch
> > 
> > ---
> > Interestingly I couldn't find a better way in C89 to printk a size_t
> > than casting it to unsigned long.
> 
> You can use %zu.

It's C99 only :-(


> > ---
> >   xen/arch/arm/setup.c       | 15 +--------------
> >   xen/arch/arm/smpboot.c     |  8 ++++++++
> >   xen/include/asm-arm/page.h | 12 ++++++++++++
> >   3 files changed, 21 insertions(+), 14 deletions(-)
> > 
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 032a6a8..b5f4c3a 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -682,19 +682,6 @@ static void __init setup_mm(unsigned long dtb_paddr,
> > size_t dtb_size)
> >     size_t __read_mostly cacheline_bytes;
> >   -/* Very early check of the CPU cache properties */
> > -void __init setup_cache(void)
> > -{
> > -    uint32_t ccsid;
> > -
> > -    /* Read the cache size ID register for the level-0 data cache */
> > -    WRITE_SYSREG32(0, CSSELR_EL1);
> > -    ccsid = READ_SYSREG32(CCSIDR_EL1);
> > -
> > -    /* Low 3 bits are log2(cacheline size in words) - 2. */
> > -    cacheline_bytes = 1U << (4 + (ccsid & 0x7));
> > -}
> > -
> >   /* C entry point for boot CPU */
> >   void __init start_xen(unsigned long boot_phys_offset,
> >                         unsigned long fdt_paddr,
> > @@ -708,7 +695,7 @@ void __init start_xen(unsigned long boot_phys_offset,
> >       struct domain *dom0;
> >       struct xen_arch_domainconfig config;
> >   -    setup_cache();
> > +    cacheline_bytes = read_cacheline_size();
> >         percpu_init_areas();
> >       set_processor_id(0); /* needed early, for smp_processor_id() */
> > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> > index 04efb33..153572e 100644
> > --- a/xen/arch/arm/smpboot.c
> > +++ b/xen/arch/arm/smpboot.c
> > @@ -323,6 +323,14 @@ void start_secondary(unsigned long boot_phys_offset,
> >           stop_cpu();
> >       }
> >   +    if ( cacheline_bytes != read_cacheline_size() )
> > +    {
> > +        printk(XENLOG_ERR "CPU%u cacheline size (%lu) does not match the
> > boot CPU (%lu)\n",
> > +               smp_processor_id(), (unsigned long) read_cacheline_size(),
> > +               (unsigned long) cacheline_bytes);
> > +        stop_cpu();
> > +    }
> > +
> >       mmu_init_secondary_cpu();
> >         gic_init_secondary_cpu();
> > diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> > index d948250..9fbf232 100644
> > --- a/xen/include/asm-arm/page.h
> > +++ b/xen/include/asm-arm/page.h
> > @@ -138,6 +138,18 @@ extern size_t cacheline_bytes;
> >     #define copy_page(dp, sp) memcpy(dp, sp, PAGE_SIZE)
> >   +static inline size_t read_cacheline_size(void)
> > +{
> > +    uint32_t ccsid;
> > +
> > +    /* Read the cache size ID register for the level-0 data cache */
> > +    WRITE_SYSREG32(0, CSSELR_EL1);
> > +    ccsid = READ_SYSREG32(CCSIDR_EL1);
> > +
> > +    /* Low 3 bits are log2(cacheline size in words) - 2. */
> > +    return (size_t) (1U << (4 + (ccsid & 0x7)));
> > +}
> > +
> >   /* Functions for flushing medium-sized areas.
> >    * if 'range' is large enough we might want to use model-specific
> >    * full-cache flushes. */
> > 
> 
> -- 
> Julien Grall
>
Andrew Cooper March 2, 2018, 6:31 p.m. UTC | #5
On 02/03/18 18:26, Stefano Stabellini wrote:
>
>>> Suggested-by: Julien Grall <julien.grall@arm.com>
>>> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
>>>
>>> ---
>>> Changes in v3:
>>> - new patch
>>>
>>> ---
>>> Interestingly I couldn't find a better way in C89 to printk a size_t
>>> than casting it to unsigned long.
>> You can use %zu.
> It's C99 only :-(

Sorry for the interjection, but what has C89 got to do with anything? 
Xen uses -std=gnu99, as per the root Config.mk file.

~Andrew
Stefano Stabellini March 2, 2018, 6:35 p.m. UTC | #6
On Fri, 2 Mar 2018, Andrew Cooper wrote:
> On 02/03/18 18:26, Stefano Stabellini wrote:

> >

> >>> Suggested-by: Julien Grall <julien.grall@arm.com>

> >>> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>

> >>>

> >>> ---

> >>> Changes in v3:

> >>> - new patch

> >>>

> >>> ---

> >>> Interestingly I couldn't find a better way in C89 to printk a size_t

> >>> than casting it to unsigned long.

> >> You can use %zu.

> > It's C99 only :-(

> 

> Sorry for the interjection, but what has C89 got to do with anything? 

> Xen uses -std=gnu99, as per the root Config.mk file.


I remember our discussions about C standard versions, but I take that
they only affected public headers then (unless I am misremembering).
Good!
Julien Grall March 2, 2018, 6:40 p.m. UTC | #7
On 02/03/2018 18:35, Stefano Stabellini wrote:
> On Fri, 2 Mar 2018, Andrew Cooper wrote:
>> On 02/03/18 18:26, Stefano Stabellini wrote:
>>>
>>>>> Suggested-by: Julien Grall <julien.grall@arm.com>
>>>>> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
>>>>>
>>>>> ---
>>>>> Changes in v3:
>>>>> - new patch
>>>>>
>>>>> ---
>>>>> Interestingly I couldn't find a better way in C89 to printk a size_t
>>>>> than casting it to unsigned long.
>>>> You can use %zu.
>>> It's C99 only :-(
>>
>> Sorry for the interjection, but what has C89 got to do with anything?
>> Xen uses -std=gnu99, as per the root Config.mk file.
> 
> I remember our discussions about C standard versions, but I take that
> they only affected public headers then (unless I am misremembering).

That's right. Public headers should be C89 compliant. Xen is making good 
use of C99 :).

Cheers,
diff mbox series

Patch

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index f73990f..7b80119 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -985,6 +985,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 1255185..7ea4e41 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();