Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/3] ppc/spapr: implement H_SIGNAL_SYS_RESET

[David Gibson]

On Fri, Oct 21, 2016 at 11:56:34AM +1100, Nicholas Piggin wrote:
> On Thu, 20 Oct 2016 18:49:22 +0200
> Greg Kurz <address@hidden> wrote:
> 
> > On Thu, 20 Oct 2016 17:59:12 +1100
> > Nicholas Piggin <address@hidden> wrote:
> > 
> > > The H_SIGNAL_SYS_RESET hcall allows a guest CPU to raise a system
> > > reset exception on other CPUs in the same guest.
> > >   
> > 
> > Actually on all CPUs or all-but-self CPUs or a specific CPU (including 
> > self).
> 
> Exactly right. I'll improve the changelog and try to add something more
> official.
> 
>  
> > > Signed-off-by: Nicholas Piggin <address@hidden>
> > > ---
> > >  hw/ppc/spapr_hcall.c   | 42 ++++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/ppc/spapr.h |  8 +++++++-
> > >  2 files changed, 49 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > > index c5e7e8c..5ae84f0 100644
> > > --- a/hw/ppc/spapr_hcall.c
> > > +++ b/hw/ppc/spapr_hcall.c
> > > @@ -880,6 +880,47 @@ static target_ulong h_set_mode(PowerPCCPU *cpu, 
> > > sPAPRMachineState *spapr,
> > >      return ret;
> > >  }
> > >  
> > > +static void do_sys_reset(CPUState *cs, void *arg)
> > > +{
> > > +    cpu_synchronize_state(cs);
> > > +    ppc_cpu_do_system_reset(cs);
> > > +}
> > > +  
> > 
> > We already have the following function in hw/ppc/spapr.c, which serves the
> > same purpose:
> > 
> > static void ppc_cpu_do_nmi_on_cpu(CPUState *cs, void *arg)
> > {
> >     cpu_synchronize_state(cs);
> >     ppc_cpu_do_system_reset(cs);
> > }
> > 
> > What about using it ?
> 
> I suppose. "nmi" isn't really architected, whereas system reset is.
> I was considering ppc_cpu_do_nmi_on_cpu call do_sys_reset, but it
> didn't seem like much of an improvement. I'll use a single function
> for both if you prefer.

I think system_reset is a better name, so it's probably best to rename
the existing function.

> 
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -339,7 +339,13 @@ struct sPAPRMachineState {
> > >  #define H_XIRR_X                0x2FC
> > >  #define H_RANDOM                0x300
> > >  #define H_SET_MODE              0x31C
> > > -#define MAX_HCALL_OPCODE        H_SET_MODE
> > > +#define H_SIGNAL_SYS_RESET      0x380
> > > +#define MAX_HCALL_OPCODE        H_SIGNAL_SYS_RESET
> > > +
> > > +/* Parameters to H_SIGNAL_SYS_RESET */
> > > +#define H_SIGNAL_SYS_RESET_ALL         -1
> > > +#define H_SIGNAL_SYS_RESET_ALLBUTSELF  -2
> > > +  
> > 
> > I'd rather move these to hw/ppc/spapr_hcall.c just above 
> > h_signal_sys_reset(),
> > the same way it is done for h_bulk_remove().
> 
> Will do.
> 
> Thanks,
> Nick
> 

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