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

[Nicholas Piggin <npiggin@gmail.com>]

On Thu, 20 Oct 2016 18:49:22 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Thu, 20 Oct 2016 17:59:12 +1100
> Nicholas Piggin <npiggin@gmail.com> 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 <npiggin@gmail.com>
> > ---
> >  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.

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