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

[Greg Kurz]

On Fri,  5 Jan 2018 14:38:06 +1100
David Gibson <address@hidden> 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 <address@hidden>
> Reviewed-by: Alexey Kardashevskiy <address@hidden>
> ---
> 
> 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 <address@hidden>

> +            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 */