Re: [PATCH v18 5/7] ppc: spapr: Handle "ibm,nmi-register" and "ibm,nmi-interlock" RTAS calls

[David Gibson]

On Thu, Jan 09, 2020 at 12:19:12AM +0530, Ganesh wrote:
> 
> 
> On 1/8/20 6:34 AM, David Gibson wrote:
> > On Tue, Jan 07, 2020 at 11:57:08AM +0530, Ganesh wrote:
> > > On 1/3/20 7:49 AM, David Gibson wrote:
> > > > On Thu, Jan 02, 2020 at 01:21:09PM +0530, Ganesh Goudar wrote:
> > > > > From: Aravinda Prasad <address@hidden>
> > > > > 
> > > > > This patch adds support in QEMU to handle "ibm,nmi-register"
> > > > > and "ibm,nmi-interlock" RTAS calls.
> > > > > 
> > > > > The machine check notification address is saved when the
> > > > > OS issues "ibm,nmi-register" RTAS call.
> > > > > 
> > > > > This patch also handles the case when multiple processors
> > > > > experience machine check at or about the same time by
> > > > > handling "ibm,nmi-interlock" call. In such cases, as per
> > > > > PAPR, subsequent processors serialize waiting for the first
> > > > > processor to issue the "ibm,nmi-interlock" call. The second
> > > > > processor that also received a machine check error waits
> > > > > till the first processor is done reading the error log.
> > > > > The first processor issues "ibm,nmi-interlock" call
> > > > > when the error log is consumed.
> > > > > 
> > > > > [Move fwnmi registration to .apply hook]
> > > > > Signed-off-by: Ganesh Goudar <address@hidden>
> > > > > Signed-off-by: Aravinda Prasad <address@hidden>
> > > > > ---
> > > > >    hw/ppc/spapr_caps.c    |  6 +++++
> > > > >    hw/ppc/spapr_rtas.c    | 58 
> > > > > ++++++++++++++++++++++++++++++++++++++++++
> > > > >    include/hw/ppc/spapr.h |  7 ++++-
> > > > >    3 files changed, 70 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > > > > index 3001098601..e922419cfb 100644
> > > > > --- a/hw/ppc/spapr_caps.c
> > > > > +++ b/hw/ppc/spapr_caps.c
> > > > > @@ -502,6 +502,12 @@ static void 
> > > > > cap_fwnmi_mce_apply(SpaprMachineState *spapr, uint8_t val,
> > > > >        if (!val) {
> > > > >            return; /* Disabled by default */
> > > > >        }
> > > > > +
> > > > > +    if (!spapr->fwnmi_calls_registered && !kvmppc_set_fwnmi()) {
> > > > So, we definitely need the kvmppc_set_fwnmi() call here.  But in the
> > > > case where we *do* have KVM, but the call fails, we should fail the
> > > > .apply hook, rather than ignoring it silently.
> > > > 
> > > > As we've discussed although TCG behaviour with fwnmi isn't 100%
> > > > correct, it's close enough to pass for most purposes - so it's
> > > > reasonable to continue if the cap is selected.  But if the cap is
> > > > selected and we're running with KVM we *must* enable the capability in
> > > > KVM or we're not providing the environment the user requested.
> > > > 
> > > > > +        /* Register ibm,nmi-register and ibm,nmi-interlock RTAS 
> > > > > calls */
> > > > > +        spapr_fwnmi_register();
> > > > We discussed registering the hypercalls here, but I thought after that
> > > > I suggested just always registering them, and having them bail out
> > > > when called if the cap is not set.  I see that you've implemented that
> > > > bailout for register, though not for interlock.  I think that's
> > > > simpler than registering them here.
> > > something like this?, with bailout in interlock as well.
> > > 
> > > {
> > >      if (!val) {
> > >          return; /* Disabled by default */
> > >      }
> > > 
> > >      if (kvm_enabled()) {
> > >          if (kvmppc_set_fwnmi() < 0) {
> > >              error_report("Could not enable fwnmi capability");
> > >              exit(1)
> > >      }
> > > 
> > >      if (!spapr->fwnmi_calls_registered) {
> > >          /* Register ibm,nmi-register and ibm,nmi-interlock RTAS calls */
> > >          spapr_fwnmi_register();
> > >          spapr->fwnmi_calls_registered = true;
> > >      }
> > > }
> > Uh.. no.. not really.  I was suggesting registering the calls
> > unconditionally, but in each of the call implementations you have
> Yes I got your point, here I am just trying to avoid registering the
> calls more than once, otherwise we may hit an assert.

Oh, right.  My point is that an advantage of registering them
unconditionally is you can do it early with the rest of the statically
registered calls, and so the problem of multiple registering them no
longer arises.

> > 
> > 
> >      if(!fwnmi_enabed)
> >           return H_FUNCTION;
> > 
> > or maybe H_NOT_AVAILABLE.
> Sure, ill add this check in interlock call also.
> > 
> > > > > +        spapr->fwnmi_calls_registered = true;
> > > > > +    }
> > > > >    }
> > > > >    SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > > > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > > > > index 2c066a372d..54b142f35b 100644
> > > > > --- a/hw/ppc/spapr_rtas.c
> > > > > +++ b/hw/ppc/spapr_rtas.c
> > > > > @@ -400,6 +400,56 @@ static void rtas_get_power_level(PowerPCCPU 
> > > > > *cpu, SpaprMachineState *spapr,
> > > > >        rtas_st(rets, 1, 100);
> > > > >    }
> > > > > +static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
> > > > > +                                  SpaprMachineState *spapr,
> > > > > +                                  uint32_t token, uint32_t nargs,
> > > > > +                                  target_ulong args,
> > > > > +                                  uint32_t nret, target_ulong rets)
> > > > > +{
> > > > > +    hwaddr rtas_addr;
> > > > > +
> > > > > +    if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_OFF) {
> > > > > +        rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    rtas_addr = spapr_get_rtas_addr();
> > > > > +    if (!rtas_addr) {
> > > > > +        rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    spapr->guest_machine_check_addr = rtas_ld(args, 1);
> > > > > +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> > > > > +}
> > > > > +
> > > > > +static void rtas_ibm_nmi_interlock(PowerPCCPU *cpu,
> > > > > +                                   SpaprMachineState *spapr,
> > > > > +                                   uint32_t token, uint32_t nargs,
> > > > > +                                   target_ulong args,
> > > > > +                                   uint32_t nret, target_ulong rets)
> > > > > +{
> > > > > +    if (spapr->guest_machine_check_addr == -1) {
> > > > > +        /* NMI register not called */
> > > > > +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    if (spapr->mc_status != cpu->vcpu_id) {
> > > > > +        /* The vCPU that hit the NMI should invoke 
> > > > > "ibm,nmi-interlock" */
> > > > > +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    /*
> > > > > +     * vCPU issuing "ibm,nmi-interlock" is done with NMI handling,
> > > > > +     * hence unset mc_status.
> > > > > +     */
> > > > > +    spapr->mc_status = -1;
> > > > > +    qemu_cond_signal(&spapr->mc_delivery_cond);
> > > > > +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> > > > > +}
> > > > > +
> > > > >    static struct rtas_call {
> > > > >        const char *name;
> > > > >        spapr_rtas_fn fn;
> > > > > @@ -503,6 +553,14 @@ hwaddr spapr_get_rtas_addr(void)
> > > > >        return (hwaddr)fdt32_to_cpu(*rtas_data);
> > > > >    }
> > > > > +void spapr_fwnmi_register(void)
> > > > > +{
> > > > > +    spapr_rtas_register(RTAS_IBM_NMI_REGISTER, "ibm,nmi-register",
> > > > > +                        rtas_ibm_nmi_register);
> > > > > +    spapr_rtas_register(RTAS_IBM_NMI_INTERLOCK, "ibm,nmi-interlock",
> > > > > +                        rtas_ibm_nmi_interlock);
> > > > > +}
> > > > > +
> > > > >    static void core_rtas_register_types(void)
> > > > >    {
> > > > >        spapr_rtas_register(RTAS_DISPLAY_CHARACTER, 
> > > > > "display-character",
> > > > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > > > index 652a5514e8..a90e677cc3 100644
> > > > > --- a/include/hw/ppc/spapr.h
> > > > > +++ b/include/hw/ppc/spapr.h
> > > > > @@ -218,6 +218,8 @@ struct SpaprMachineState {
> > > > >        unsigned gpu_numa_id;
> > > > >        SpaprTpmProxy *tpm_proxy;
> > > > > +
> > > > > +    bool fwnmi_calls_registered;
> > > > >    };
> > > > >    #define H_SUCCESS         0
> > > > > @@ -656,8 +658,10 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu, 
> > > > > target_ulong opcode,
> > > > >    #define RTAS_IBM_REMOVE_PE_DMA_WINDOW           (RTAS_TOKEN_BASE + 
> > > > > 0x28)
> > > > >    #define RTAS_IBM_RESET_PE_DMA_WINDOW            (RTAS_TOKEN_BASE + 
> > > > > 0x29)
> > > > >    #define RTAS_IBM_SUSPEND_ME                     (RTAS_TOKEN_BASE + 
> > > > > 0x2A)
> > > > > +#define RTAS_IBM_NMI_REGISTER                   (RTAS_TOKEN_BASE + 
> > > > > 0x2B)
> > > > > +#define RTAS_IBM_NMI_INTERLOCK                  (RTAS_TOKEN_BASE + 
> > > > > 0x2C)
> > > > > -#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 
> > > > > 0x2B)
> > > > > +#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 
> > > > > 0x2D)
> > > > >    /* RTAS ibm,get-system-parameter token values */
> > > > >    #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20
> > > > > @@ -908,4 +912,5 @@ void spapr_check_pagesize(SpaprMachineState 
> > > > > *spapr, hwaddr pagesize,
> > > > >    void spapr_set_all_lpcrs(target_ulong value, target_ulong mask);
> > > > >    hwaddr spapr_get_rtas_addr(void);
> > > > > +void spapr_fwnmi_register(void);
> > > > >    #endif /* HW_SPAPR_H */
> 

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