Re: [Qemu-devel] [PATCH] spapr: Correct compatibility mode setting for hotplugged CPUs

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3796 bytes --]

On Thu, Jan 11, 2018 at 01:24:30PM +0100, Greg Kurz wrote:
> On Fri, 5 Jan 2018 14:07:29 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Jan 04, 2018 at 06:47:18PM +0100, Greg Kurz wrote:
> > > On Thu,  4 Jan 2018 15:24:05 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > Currently the pseries machine sets the compatibility mode for the
> > > > guest's cpus in two places: 1) at machine reset and 2) after CAS
> > > > negotiation.
> > > > 
> > > > This means that if we set or negotiate a compatiblity mode, then
> > > > hotplug a cpu, the hotplugged cpu doesn't get the right mode set and
> > > > will incorrectly have the full native features.
> > > > 
> > > > To correct this, we set the compatibility mode on a cpu when it is
> > > > brought online with the 'start-cpu' RTAS call.  Given that we no
> > > > longer need to set the compatibility mode on all CPUs at machine
> > > > reset, so we change that to only set the mode for the boot cpu.
> > > > 
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > >  hw/ppc/spapr.c      | 2 +-
> > > >  hw/ppc/spapr_rtas.c | 8 ++++++++
> > > >  2 files changed, 9 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index e22888ba06..d1acfe8858 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -1510,7 +1510,7 @@ static void spapr_machine_reset(void)
> > > >          spapr_ovec_cleanup(spapr->ov5_cas);
> > > >          spapr->ov5_cas = spapr_ovec_new();
> > > >  
> > > > -        ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
> > > > +        ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
> > > >      }
> > > >  
> > > >      fdt = spapr_build_fdt(spapr, rtas_addr, spapr->rtas_size);
> > > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > > > index 4bb939d3d1..2ed00548c1 100644
> > > > --- a/hw/ppc/spapr_rtas.c
> > > > +++ b/hw/ppc/spapr_rtas.c
> > > > @@ -163,6 +163,7 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> > > >          CPUState *cs = CPU(cpu);
> > > >          CPUPPCState *env = &cpu->env;
> > > >          PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > > > +        Error *local_err = NULL;
> > > >  
> > > >          if (!cs->halted) {
> > > >              rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> > > > @@ -174,6 +175,13 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> > > >           * new cpu enters */
> > > >          kvm_cpu_synchronize_state(cs);
> > > >  
> > > > +        /* Set compatibility mode to match existing cpus */
> > > > +        ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &local_err);  
> > > 
> > > Is it okay to report a simple HW error to the guest here instead of aborting
> > > like we do with first_cpu at reset time ?  
> > 
> > Should be: this happens before we turn the cpu on, so the effect will
> > be that the guest fails to online the cpu.  That seems like a better
> > failure mode than killing the already running guest.
> > 
> 
> Of course, I generally agree with the "better failure mode". My point was
> just that if we managed to set the compat mode with the first cpu but we
> fail to propagate the same compat mode to subsequent cpus, then this is
> a bug in QEMU or KVM.

Ah, I see your point.  Yes, I guess that's true.  Nonetheless, I think
the better failure mode is still a better idea, even if something goes
wrong elsewhere.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]