Re: [Qemu-devel] [PATCH 2/3] xics/spapr: Register RTAS/hypercalls once at machine init

[David Gibson]

On Thu, Jun 13, 2019 at 06:44:59PM +0200, Greg Kurz wrote:
> QEMU may crash when running a spapr machine in 'dual' interrupt controller
> mode on some older (but not that old, eg. ubuntu 18.04.2) KVMs with partial
> XIVE support:
> 
> qemu-system-ppc64: hw/ppc/spapr_rtas.c:411: spapr_rtas_register:
>  Assertion `!name || !rtas_table[token].name' failed.
> 
> XICS is controlled by the guest thanks to a set of RTAS calls. Depending
> on whether KVM XICS is used or not, the RTAS calls are handled by KVM or
> QEMU. In both cases, QEMU needs to expose the RTAS calls to the guest
> through the "rtas" node of the device tree.
> 
> The spapr_rtas_register() helper takes care of all of that: it adds the
> RTAS call token to the "rtas" node and registers a QEMU callback to be
> invoked when the guest issues the RTAS call. In the KVM XICS case, QEMU
> registers a dummy callback that just prints an error since it isn't
> supposed to be invoked, ever.
> 
> Historically, the XICS controller was setup during machine init and
> released during final teardown. This changed when the 'dual' interrupt
> controller mode was added to the spapr machine: in this case we need
> to tear the XICS down and set it up again during machine reset. The
> crash happens because we indeed have an incompatibility with older
> KVMs that forces QEMU to fallback on emulated XICS, which tries to
> re-registers the same RTAS calls.
> 
> This could be fixed by adding proper rollback that would unregister
> RTAS calls on error. But since the emulated RTAS calls in QEMU can
> now detect when they are mistakenly called while KVM XICS is in
> use, it seems simpler to register them once and for all at machine
> init. This fixes the crash and allows to remove some now useless
> lines of code.
> 
> Signed-off-by: Greg Kurz <address@hidden>

Applied, thanks.

> ---
>  hw/intc/xics_kvm.c          |   19 -------------------
>  hw/intc/xics_spapr.c        |    8 --------
>  hw/ppc/spapr_irq.c          |    3 ++-
>  include/hw/ppc/spapr.h      |    4 ----
>  include/hw/ppc/xics.h       |    1 -
>  include/hw/ppc/xics_spapr.h |    1 +
>  6 files changed, 3 insertions(+), 33 deletions(-)
> 
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 5ba5b775615e..5c4208f43008 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -331,15 +331,6 @@ void ics_kvm_set_irq(ICSState *ics, int srcno, int val)
>      }
>  }
>  
> -static void rtas_dummy(PowerPCCPU *cpu, SpaprMachineState *spapr,
> -                       uint32_t token,
> -                       uint32_t nargs, target_ulong args,
> -                       uint32_t nret, target_ulong rets)
> -{
> -    error_report("pseries: %s must never be called for in-kernel XICS",
> -                 __func__);
> -}
> -
>  int xics_kvm_init(SpaprMachineState *spapr, Error **errp)
>  {
>      int rc;
> @@ -360,11 +351,6 @@ int xics_kvm_init(SpaprMachineState *spapr, Error **errp)
>          goto fail;
>      }
>  
> -    spapr_rtas_register(RTAS_IBM_SET_XIVE, "ibm,set-xive", rtas_dummy);
> -    spapr_rtas_register(RTAS_IBM_GET_XIVE, "ibm,get-xive", rtas_dummy);
> -    spapr_rtas_register(RTAS_IBM_INT_OFF, "ibm,int-off", rtas_dummy);
> -    spapr_rtas_register(RTAS_IBM_INT_ON, "ibm,int-on", rtas_dummy);
> -
>      rc = kvmppc_define_rtas_kernel_token(RTAS_IBM_SET_XIVE, "ibm,set-xive");
>      if (rc < 0) {
>          error_setg(errp, "kvmppc_define_rtas_kernel_token: ibm,set-xive");
> @@ -454,11 +440,6 @@ void xics_kvm_disconnect(SpaprMachineState *spapr, Error 
> **errp)
>      close(kernel_xics_fd);
>      kernel_xics_fd = -1;
>  
> -    spapr_rtas_unregister(RTAS_IBM_SET_XIVE);
> -    spapr_rtas_unregister(RTAS_IBM_GET_XIVE);
> -    spapr_rtas_unregister(RTAS_IBM_INT_OFF);
> -    spapr_rtas_unregister(RTAS_IBM_INT_ON);
> -
>      kvmppc_define_rtas_kernel_token(0, "ibm,set-xive");
>      kvmppc_define_rtas_kernel_token(0, "ibm,get-xive");
>      kvmppc_define_rtas_kernel_token(0, "ibm,int-on");
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index d470ab5f7a2a..8d605b68a7a0 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -285,14 +285,6 @@ static void rtas_int_on(PowerPCCPU *cpu, 
> SpaprMachineState *spapr,
>  
>  void xics_spapr_init(SpaprMachineState *spapr)
>  {
> -    /* Emulated mode can only be initialized once. */
> -    if (spapr->ics->init) {
> -        return;
> -    }
> -
> -    spapr->ics->init = true;
> -
> -    /* Registration of global state belongs into realize */
>      spapr_rtas_register(RTAS_IBM_SET_XIVE, "ibm,set-xive", rtas_set_xive);
>      spapr_rtas_register(RTAS_IBM_GET_XIVE, "ibm,get-xive", rtas_get_xive);
>      spapr_rtas_register(RTAS_IBM_INT_OFF, "ibm,int-off", rtas_int_off);
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 3156daf09381..dfb99f35ea00 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -114,6 +114,8 @@ static void spapr_irq_init_xics(SpaprMachineState *spapr, 
> int nr_irqs,
>      }
>  
>      spapr->ics = ICS_BASE(obj);
> +
> +    xics_spapr_init(spapr);
>  }
>  
>  #define ICS_IRQ_FREE(ics, srcno)   \
> @@ -236,7 +238,6 @@ static const char 
> *spapr_irq_get_nodename_xics(SpaprMachineState *spapr)
>  
>  static void spapr_irq_init_emu_xics(SpaprMachineState *spapr, Error **errp)
>  {
> -    xics_spapr_init(spapr);
>  }
>  
>  static void spapr_irq_init_kvm_xics(SpaprMachineState *spapr, Error **errp)
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 4f5becf1f3cc..60553d32c4fa 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -676,10 +676,6 @@ typedef void (*spapr_rtas_fn)(PowerPCCPU *cpu, 
> SpaprMachineState *sm,
>                                uint32_t nargs, target_ulong args,
>                                uint32_t nret, target_ulong rets);
>  void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn);
> -static inline void spapr_rtas_unregister(int token)
> -{
> -    spapr_rtas_register(token, NULL, NULL);
> -}
>  target_ulong spapr_rtas_call(PowerPCCPU *cpu, SpaprMachineState *sm,
>                               uint32_t token, uint32_t nargs, target_ulong 
> args,
>                               uint32_t nret, target_ulong rets);
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index d6f8e4c4c282..eb65ad7e43b7 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -119,7 +119,6 @@ struct ICSState {
>      uint32_t offset;
>      ICSIRQState *irqs;
>      XICSFabric *xics;
> -    bool init; /* sPAPR ICS device initialized */
>  };
>  
>  #define ICS_PROP_XICS "xics"
> diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
> index 2476b540edfa..6c1d9ee55945 100644
> --- a/include/hw/ppc/xics_spapr.h
> +++ b/include/hw/ppc/xics_spapr.h
> @@ -36,5 +36,6 @@ void spapr_dt_xics(SpaprMachineState *spapr, uint32_t 
> nr_servers, void *fdt,
>  int xics_kvm_init(SpaprMachineState *spapr, Error **errp);
>  void xics_kvm_disconnect(SpaprMachineState *spapr, Error **errp);
>  void xics_spapr_init(SpaprMachineState *spapr);
> +void xics_spapr_connect(SpaprMachineState *spapr);
>  
>  #endif /* XICS_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