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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 13 Feb 2018, Julien Grall wrote:
> On 02/09/2018 07:12 PM, Julien Grall wrote:
> > Hi,
> > 
> > On 02/09/2018 07:10 PM, Stefano Stabellini wrote:
> > > On Fri, 9 Feb 2018, Julien Grall wrote:
> > > > On 02/09/2018 07:02 PM, Stefano Stabellini wrote:
> > > > > On Fri, 9 Feb 2018, Julien Grall wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On 02/08/2018 11:49 PM, Stefano Stabellini wrote:
> > > > > > > On Thu, 1 Feb 2018, Julien Grall wrote:
> > > > > > > > On 1 February 2018 at 19:37, Stefano Stabellini
> > > > > > > > <sstabellini@xxxxxxxxxx>
> > > > > > > > wrote:
> > > > > > > > > On Tue, 30 Jan 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>
> > > > > > > > > > 
> > > > > > > > > > ---
> > > > > > > > > > 
> > > > > > > > > > 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.
> > > > > > > > > > ---
> > > > > > > > > >     xen/arch/arm/smpboot.c | 15 +++++++++++++++
> > > > > > > > > >     1 file changed, 15 insertions(+)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> > > > > > > > > > index 1255185a9c..2c2815f9ee 100644
> > > > > > > > > > --- a/xen/arch/arm/smpboot.c
> > > > > > > > > > +++ b/xen/arch/arm/smpboot.c
> > > > > > > > > > @@ -292,6 +292,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 unstability. Better to park them for now.
> > > > > > > > > > +     *
> > > > > > > > > > +     * TODO: Add big.LITTLE support.
> > > > > > > > > > +     */
> > > > > > > > > > +    if ( 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();
> > > > > > > > > > +    }
> > > > > > > > > 
> > > > > > > > > I understand that this patch is the right thing to do from a
> > > > > > > > > correctness
> > > > > > > > > perspective, especially in regards to the SP2 mitigation.
> > > > > > > > > 
> > > > > > > > > At the same time I would also like to give the option for
> > > > > > > > > people
> > > > > > > > > that
> > > > > > > > > want to use big.LITTLE with cpupools / cpu pinning to do so if
> > > > > > > > > they
> > > > > > > > > really want to, but I am not sure what to suggest.
> > > > > > > > > 
> > > > > > > > > Could we introduce a command line to proceed anyway? But then
> > > > > > > > > the
> > > > > > > > > system
> > > > > > > > > would be susceptible to SP2 in the cpus different from the
> > > > > > > > > boot cpu.
> > > > > > > > > Could we make the SP2 mitigation work on big.LITTLE or is it
> > > > > > > > > too
> > > > > > > > > much
> > > > > > > > > trouble? Do you have any other ideas or thoughts about this?
> > > > > > > > 
> > > > > > > > This patch is here to prevent to spread instability/insecurity
> > > > > > > > or give
> > > > > > > > the feeling we do support big.LITTLE.
> > > > > > > > 
> > > > > > > > Even outside of SP2, there are possibility for instability
> > > > > > > > because CPU
> > > > > > > > errata
> > > > > > > > would not be applied correctly in the guest or because Xen is
> > > > > > > > not able
> > > > > > > > to
> > > > > > > > know that non CPUs may have a different cacheline size...
> > > > > > > > 
> > > > > > > > I want to end this idea that Xen may support big.LITTLE.
> > > > > > > > 
> > > > > > > > The first thing to modify is the vpdir (virtual MIDR), at the
> > > > > > > > moment
> > > > > > > > we
> > > > > > > > always
> > > > > > > > use the boot MIDR. What would you choose now? The MIDR of the
> > > > > > > > CPU
> > > > > > > > where
> > > > > > > > the hypercall happen?
> > > > > > > > 
> > > > > > > > There is no shortcut for big.LITTLE. The right thing is to
> > > > > > > > implement
> > > > > > > > what
> > > > > > > > has
> > > > > > > > been discussed in the design document written by Dario. But
> > > > > > > > that's a
> > > > > > > > new
> > > > > > > > feature and would require some work to do it properly.
> > > > > > > > 
> > > > > > > > A command line option might be a good idea, but I would be more
> > > > > > > > of the
> > > > > > > > opinion
> > > > > > > > to delay that and see who is screaming about it.
> > > > > > > > 
> > > > > > > > My hunch is not many people will scream because today they tend
> > > > > > > > to
> > > > > > > > disable
> > > > > > > > one set of CPUs in the DT directly.
> > > > > > > 
> > > > > > > As discussed, are you going to resend with a command line option
> > > > > > > such as
> > > > > > > biglittle=unsafe or something like that?
> > > > > > 
> > > > > > I would prefer to avoid term big.LITTLE in the command line option
> > > > > > because
> > > > > > it
> > > > > > might be possible to have platform with more than two kind of CPUs.
> > > > > > How
> > > > > > about
> > > > > > "smp=unsafe"?
> > > > > 
> > > > > I am fine with not using big.LITTLE but smp=unsafe is a bit confusing.
> > > > > What do you think of: "heterogeneous=unsafe" it is a bit of a mouthful
> > > > > but it should be clearer.
> > > > 
> > > > Heterogeneous does not tell you what you are trying to do. I think it
> > > > needs to
> > > > be qualified with the smp (or something similar).\
> > > > 
> > > > How about mp_unsafe_heterogeneous=yes/no.
> > > 
> > > it's getting longer and longer, but OK :-)  At least it's descriptive.
> > 
> > It will be easier to spot in the logs :). I will resend the patch next week
> > with the command line added.
> 
> After looking at the design doc from Dario, I was wondering if we should name
> the option: hmp_unsafe (or amp_unsafe). This would make the name slightly
> shorter.
> 
> The description of the command line would be:
> 
> "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. By
> default cores which are not identical to the boot CPU will be parked and not
> used by Xen.".

Good idea, Julien. hmp_unsafe is far better.

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