Re: [Qemu-ppc] [PATCH 2/8] spapr: Clean up rtas_start_cpu() & rtas_stop_self()

[David Gibson]

On Thu, May 03, 2018 at 05:13:31PM +0200, Greg Kurz wrote:
> On Thu,  3 May 2018 16:21:39 +1000
> David Gibson <address@hidden> wrote:
> 
> > This makes several minor cleanups to these functions:
> >   * Follow usual convention of an early exit on error, rather than having
> >     most of the body in an if
> >   * Clearer naming of cpu and cpu_.  Now callcpu is the cpu from which the
> >     RTAS call is invoked, newcpu is the cpu which we're starting
> >   * Use cpu_synchronize_state() instead of kvm_cpu_synchronize_state()
> >     directly
> >   * Remove pointless comment describing what cpu_synchronize_state() does
> >   * Use ppc_store_lpcr() instead of directly writing the register field
> > 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> >  hw/ppc/spapr_rtas.c | 66 ++++++++++++++++++++++-----------------------
> >  1 file changed, 32 insertions(+), 34 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > index 0ec5fa4cfe..b251c130cb 100644
> > --- a/hw/ppc/spapr_rtas.c
> > +++ b/hw/ppc/spapr_rtas.c
> > @@ -32,7 +32,7 @@
> >  #include "hw/qdev.h"
> >  #include "sysemu/device_tree.h"
> >  #include "sysemu/cpus.h"
> > -#include "sysemu/kvm.h"
> > +#include "sysemu/hw_accel.h"
> >  
> >  #include "hw/ppc/spapr.h"
> >  #include "hw/ppc/spapr_vio.h"
> > @@ -45,6 +45,7 @@
> >  #include "qemu/cutils.h"
> >  #include "trace.h"
> >  #include "hw/ppc/fdt.h"
> > +#include "target/ppc/mmu-hash64.h"
> >  
> >  static void rtas_display_character(PowerPCCPU *cpu, sPAPRMachineState 
> > *spapr,
> >                                     uint32_t token, uint32_t nargs,
> > @@ -140,13 +141,15 @@ static void spapr_cpu_set_endianness(PowerPCCPU *cpu)
> >      }
> >  }
> >  
> > -static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMachineState *spapr,
> > +static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
> >                             uint32_t token, uint32_t nargs,
> >                             target_ulong args,
> >                             uint32_t nret, target_ulong rets)
> >  {
> >      target_ulong id, start, r3;
> > -    PowerPCCPU *cpu;
> > +    PowerPCCPU *newcpu;
> > +    CPUPPCState *env;
> > +    PowerPCCPUClass *pcc;
> >  
> >      if (nargs != 3 || nret != 1) {
> >          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > @@ -157,41 +160,37 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, 
> > sPAPRMachineState *spapr,
> >      start = rtas_ld(args, 1);
> >      r3 = rtas_ld(args, 2);
> >  
> > -    cpu = spapr_find_cpu(id);
> > -    if (cpu != NULL) {
> > -        CPUState *cs = CPU(cpu);
> 
> The function ends up with four invocations of CPU(newcpu), ie, we'll
> go down the full type checking path four times. Not very painful since
> rtas_start_cpu() is not a hot path, but it would certainly not hurt
> to do it only once like the current code does.
> 
> Anyway, with or without this change:

Ah, yeah, I was thinking of CPU() as a trivial offset, and forgot it
also did type checking.

Eh, as you say it's not a hot path and I don't really want to
introduce a fourth cpu parameter/variable so let's leave it as is.

> 
> Reviewed-by: Greg Kurz <address@hidden>
> 
> > -        CPUPPCState *env = &cpu->env;
> > -        PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > +    newcpu = spapr_find_cpu(id);
> > +    if (!newcpu) {
> > +        /* Didn't find a matching cpu */
> > +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > +        return;
> > +    }
> >  
> > -        if (!cs->halted) {
> > -            rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> > -            return;
> > -        }
> > +    env = &newcpu->env;
> > +    pcc = POWERPC_CPU_GET_CLASS(newcpu);
> >  
> > -        /* This will make sure qemu state is up to date with kvm, and
> > -         * mark it dirty so our changes get flushed back before the
> > -         * new cpu enters */
> > -        kvm_cpu_synchronize_state(cs);
> > +    if (!CPU(newcpu)->halted) {
> > +        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> > +        return;
> > +    }
> >  
> > -        env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> > +    cpu_synchronize_state(CPU(newcpu));
> >  
> > -        /* Enable Power-saving mode Exit Cause exceptions for the new CPU 
> > */
> > -        env->spr[SPR_LPCR] |= pcc->lpcr_pm;
> > +    env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> > +    spapr_cpu_set_endianness(newcpu);
> > +    spapr_cpu_update_tb_offset(newcpu);
> > +    /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
> > +    ppc_store_lpcr(newcpu, env->spr[SPR_LPCR] | pcc->lpcr_pm);
> >  
> > -        env->nip = start;
> > -        env->gpr[3] = r3;
> > -        cs->halted = 0;
> > -        spapr_cpu_set_endianness(cpu);
> > -        spapr_cpu_update_tb_offset(cpu);
> > +    env->nip = start;
> > +    env->gpr[3] = r3;
> >  
> > -        qemu_cpu_kick(cs);
> > +    CPU(newcpu)->halted = 0;
> >  
> > -        rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> > -        return;
> > -    }
> > +    qemu_cpu_kick(CPU(newcpu));
> >  
> > -    /* Didn't find a matching cpu */
> > -    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> >  }
> >  
> >  static void rtas_stop_self(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > @@ -203,13 +202,12 @@ static void rtas_stop_self(PowerPCCPU *cpu, 
> > sPAPRMachineState *spapr,
> >      CPUPPCState *env = &cpu->env;
> >      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> >  
> > -    cs->halted = 1;
> > -    qemu_cpu_kick(cs);
> > -
> >      /* Disable Power-saving mode Exit Cause exceptions for the CPU.
> >       * This could deliver an interrupt on a dying CPU and crash the
> >       * guest */
> > -    env->spr[SPR_LPCR] &= ~pcc->lpcr_pm;
> > +    ppc_store_lpcr(cpu, env->spr[SPR_LPCR] & ~pcc->lpcr_pm);
> > +    cs->halted = 1;
> > +    qemu_cpu_kick(cs);
> >  }
> >  
> >  static inline int sysparm_st(target_ulong addr, target_ulong len,
> 

-- 
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