Message ID | 20180105033806.14678-1-david@gibson.dropbear.id.au |
---|---|
State | New |
Headers | show |
Series | [PATCHv2] spapr: Correct compatibility mode setting for hotplugged CPUs | expand |
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> > --- > > 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 */
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 */ > >
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) { + 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 */