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

[satheesh rajendran <sathnaga@linux.vnet.ibm.com>]

On Fri, Jan 05, 2018 at 12:19:14PM +0100, Greg Kurz wrote:
> On Fri,  5 Jan 2018 14:38:06 +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>
> > Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > ---

Tested in upstream with this patch and found fixed.
./ppc64-softmmu/qemu-system-ppc64 -M pseries,max-cpu-compat=power8,accel=kvm \
                                  -smp 1,cores=1,threads=1,sockets=1,maxcpus=2 \
                                  -m 2048 -nographic -enable-kvm /home/sath/images/hostos-ppc64le.qcow2 \
                                  -monitor stdio -serial /dev/pts/7
QEMU 2.11.50 monitor - type 'help' for more information
(qemu) device_add host-spapr-cpu-core,id=core1,core-id=1
(qemu) 

inside guest:
# taskset -c 1 ./a.out 
Illegal instruction


Reported-and-tested-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>

Regards,
-Satheesh


> > 
> > Changes since v1:
> >  * Add an error_report_err() to display and free the error from
> >    ppc_set_compat() in rtas_start_cpu().
> >  
> 
> It looks like this change wasn't committed when git-send-email was
> invoked, and you missed Daniel's comment (typo in commit message)
> and R-b...
> 
> > 
> >  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);
> > +        if (local_err) {
> 
> With a call to error_report_err() here, you can add:
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> > +            rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> > +            return;
> > +        }
> > +
> >          env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> >  
> >          /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
> 
>