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

[David Gibson]

On Thu, Oct 24, 2019 at 01:13:05PM +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 registeration to .apply hook]

s/registeration/registration/

> Signed-off-by: Ganesh Goudar <address@hidden>
> Signed-off-by: Aravinda Prasad <address@hidden>
> ---
>  hw/ppc/spapr_caps.c    |  9 +++++--
>  hw/ppc/spapr_rtas.c    | 57 ++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h |  5 +++-
>  3 files changed, 68 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 976d709210..1675ebd45e 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -509,9 +509,14 @@ static void cap_fwnmi_mce_apply(SpaprMachineState 
> *spapr, uint8_t val,
>           * of software injected faults like duplicate SLBs).
>           */
>          warn_report("Firmware Assisted Non-Maskable Interrupts not supported 
> in TCG");

This logic still isn't quite right.  To start with the warn_report()
above possible wants to be more weakly worded.  With TCG, FWNMI won't
generally *do* anything, and there are some edge cases where the
behaviour is arguably incorrect.  However there's no reason we can't
make the RTAS calls work basically as expected and in almost all cases
things will behave correctly - at least according to the case where no
fwnmi events are delivered...

> -    } else if (kvm_enabled() && (kvmppc_set_fwnmi() != 0)) {
> -        error_setg(errp,
> +    } else if (kvm_enabled()) {
> +        if (!kvmppc_set_fwnmi()) {
> +            /* Register ibm,nmi-register and ibm,nmi-interlock RTAS calls */
> +            spapr_fwnmi_register();

..but here you only register the RTAS calls in the KVM case, which
breaks that.  If there really is a strong reason to do this, then the
warn_report() above should be error_setg() and fail the apply.

> +        } else {
> +            error_setg(errp,
>  "Firmware Assisted Non-Maskable Interrupts not supported by KVM, try 
> cap-fwnmi-mce=off");
> +        }
>      }
>  }
>  
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 2c066a372d..0328b1f341 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -400,6 +400,55 @@ 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 = spapr_get_rtas_addr();
> +
> +    if (!rtas_addr) {
> +        rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
> +        return;
> +    }
> +
> +    if (spapr_get_cap(spapr, SPAPR_CAP_FWNMI_MCE) == SPAPR_CAP_OFF) {
> +        rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);

Actually, since you explicitly test for the cap being enabled here,
there's no reason not to *always* register this RTAS call.  Also this
test for the feature flag should go first, before delving into the
device tree for the RTAS address.

> +        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 +552,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 4afa8d4d09..86f0fc8fdd 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -653,8 +653,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
> @@ -907,4 +909,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