On Thu, Jan 11, 2018 at 01:24:30PM +0100, Greg Kurz wrote: > On Fri, 5 Jan 2018 14:07:29 +1100 > David Gibson <da...@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 <da...@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 <da...@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
signature.asc
Description: PGP signature