Re: [Qemu-ppc] [PATCH v3 15/15] spapr/irq: add KVM support to the 'dual' machine

[David Gibson]

On Thu, Mar 21, 2019 at 03:49:14PM +0100, Cédric Le Goater wrote:
> The interrupt mode is chosen by the CAS negotiation process and
> activated after a reset to take into account the required changes in
> the machine. This brings new constraints on how the associated KVM IRQ
> device is initialized.
> 
> Currently, each model takes care of the initialization of the KVM
> device in their realize method but this is not possible anymore as the
> initialization needs to be done globaly when the interrupt mode is
> known, i.e. when machine is reseted. It also means that we need a way
> to delete a KVM device when another mode is chosen.
> 
> Also, to support migration, the QEMU objects holding the state to
> transfer should always be available but not necessarily activated.
> 
> The overall approach of this proposal is to initialize both interrupt
> mode at the QEMU level to keep the IRQ number space in sync and to
> allow switching from one mode to another. For the KVM side of things,
> the whole initialization of the KVM device, sources and presenters, is
> grouped in a single routine. The XICS and XIVE sPAPR IRQ reset
> handlers are modified accordingly to handle the init and the delete
> sequences of the KVM device.
> 
> Signed-off-by: Cédric Le Goater <address@hidden>

Reviewed-by: David Gibson <address@hidden>

> ---
> 
>  Changes since v2:
> 
>  - introduced the use of spapr_irq_init_device() 
> 
>  include/hw/ppc/xive.h    |  1 -
>  hw/intc/spapr_xive_kvm.c | 29 ++++++++++++++++++++++-
>  hw/intc/xics_kvm.c       | 26 ++++++++++++++++++++
>  hw/intc/xive.c           |  4 ----
>  hw/ppc/spapr_irq.c       | 51 ++++++++++++++++++++++++++++++----------
>  5 files changed, 92 insertions(+), 19 deletions(-)
> 
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index edb8937f17fb..d872f96d1a1b 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -432,7 +432,6 @@ static inline uint32_t xive_nvt_cam_line(uint8_t nvt_blk, 
> uint32_t nvt_idx)
>   */
>  
>  void kvmppc_xive_source_reset_one(XiveSource *xsrc, int srcno, Error **errp);
> -void kvmppc_xive_source_reset(XiveSource *xsrc, Error **errp);
>  void kvmppc_xive_source_set_irq(void *opaque, int srcno, int val);
>  void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp);
>  void kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp);
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 78d2ea433c98..5577a88bf10f 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -238,7 +238,7 @@ void kvmppc_xive_source_reset_one(XiveSource *xsrc, int 
> srcno, Error **errp)
>                        true, errp);
>  }
>  
> -void kvmppc_xive_source_reset(XiveSource *xsrc, Error **errp)
> +static void kvmppc_xive_source_reset(XiveSource *xsrc, Error **errp)
>  {
>      int i;
>  
> @@ -690,6 +690,15 @@ void kvmppc_xive_connect(SpaprXive *xive, Error **errp)
>      Error *local_err = NULL;
>      size_t esb_len = (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
>      size_t tima_len = 4ull << TM_SHIFT;
> +    CPUState *cs;
> +
> +    /*
> +     * The KVM XIVE device already in use. This is the case when
> +     * rebooting under the XIVE-only interrupt mode.
> +     */
> +    if (xive->fd != -1) {
> +        return;
> +    }
>  
>      if (!kvmppc_has_cap_xive()) {
>          error_setg(errp, "IRQ_XIVE capability must be present for KVM");
> @@ -738,6 +747,24 @@ void kvmppc_xive_connect(SpaprXive *xive, Error **errp)
>      xive->change = qemu_add_vm_change_state_handler(
>          kvmppc_xive_change_state_handler, xive);
>  
> +    /* Connect the presenters to the initial VCPUs of the machine */
> +    CPU_FOREACH(cs) {
> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> +        kvmppc_xive_cpu_connect(spapr_cpu_state(cpu)->tctx, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    }
> +
> +    /* Update the KVM sources */
> +    kvmppc_xive_source_reset(xsrc, &local_err);
> +    if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +    }
> +
>      kvm_kernel_irqchip = true;
>      kvm_msi_via_irqfd_allowed = true;
>      kvm_gsi_direct_mapping = true;
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index b3a01326d261..b33d1f2f789e 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -33,6 +33,7 @@
>  #include "trace.h"
>  #include "sysemu/kvm.h"
>  #include "hw/ppc/spapr.h"
> +#include "hw/ppc/spapr_cpu_core.h"
>  #include "hw/ppc/xics.h"
>  #include "hw/ppc/xics_spapr.h"
>  #include "kvm_ppc.h"
> @@ -342,6 +343,16 @@ static void rtas_dummy(PowerPCCPU *cpu, 
> SpaprMachineState *spapr,
>  int xics_kvm_init(SpaprMachineState *spapr, Error **errp)
>  {
>      int rc;
> +    CPUState *cs;
> +    Error *local_err = NULL;
> +
> +    /*
> +     * The KVM XICS device already in use. This is the case when
> +     * rebooting under the XICS-only interrupt mode.
> +     */
> +    if (kernel_xics_fd != -1) {
> +        return 0;
> +    }
>  
>      if (!kvm_enabled() || !kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS)) 
> {
>          error_setg(errp,
> @@ -390,6 +401,21 @@ int xics_kvm_init(SpaprMachineState *spapr, Error **errp)
>      kvm_msi_via_irqfd_allowed = true;
>      kvm_gsi_direct_mapping = true;
>  
> +    /* Connect the presenters to the initial VCPUs of the machine */
> +    CPU_FOREACH(cs) {
> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> +        icp_kvm_realize(DEVICE(spapr_cpu_state(cpu)->icp), &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            goto fail;
> +        }
> +        icp_set_kvm_state(spapr_cpu_state(cpu)->icp);
> +    }
> +
> +    /* Update the KVM sources */
> +    ics_set_kvm_state(spapr->ics);
> +
>      return 0;
>  
>  fail:
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 901689b0f015..1909c0dec5b2 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -993,10 +993,6 @@ static void xive_source_reset(void *dev)
>  
>      /* PQs are initialized to 0b01 (Q=1) which corresponds to "ints off" */
>      memset(xsrc->status, XIVE_ESB_OFF, xsrc->nr_irqs);
> -
> -    if (kvm_irqchip_in_kernel()) {
> -        kvmppc_xive_source_reset(xsrc, &error_fatal);
> -    }
>  }
>  
>  static void xive_source_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 0c65de35f52a..170c0bbb025e 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -101,12 +101,6 @@ static void spapr_irq_init_xics(SpaprMachineState 
> *spapr, int nr_irqs,
>      Object *obj;
>      Error *local_err = NULL;
>  
> -    spapr_irq_init_device(spapr, &spapr_irq_xics, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> -
>      obj = object_new(TYPE_ICS_SIMPLE);
>      object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
>      object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
> @@ -225,7 +219,13 @@ static void spapr_irq_set_irq_xics(void *opaque, int 
> srcno, int val)
>  
>  static void spapr_irq_reset_xics(SpaprMachineState *spapr, Error **errp)
>  {
> -    /* TODO: create the KVM XICS device */
> +    Error *local_err = NULL;
> +
> +    spapr_irq_init_device(spapr, &spapr_irq_xics, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  }
>  
>  static const char *spapr_irq_get_nodename_xics(SpaprMachineState *spapr)
> @@ -381,6 +381,7 @@ static int spapr_irq_post_load_xive(SpaprMachineState 
> *spapr, int version_id)
>  static void spapr_irq_reset_xive(SpaprMachineState *spapr, Error **errp)
>  {
>      CPUState *cs;
> +    Error *local_err = NULL;
>  
>      CPU_FOREACH(cs) {
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
> @@ -389,6 +390,12 @@ static void spapr_irq_reset_xive(SpaprMachineState 
> *spapr, Error **errp)
>          spapr_xive_set_tctx_os_cam(spapr_cpu_state(cpu)->tctx);
>      }
>  
> +    spapr_irq_init_device(spapr, &spapr_irq_xive, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
>      /* Activate the XIVE MMIOs */
>      spapr_xive_mmio_set_enabled(spapr->xive, true);
>  }
> @@ -471,14 +478,8 @@ static SpaprIrq *spapr_irq_current(SpaprMachineState 
> *spapr)
>  static void spapr_irq_init_dual(SpaprMachineState *spapr, int nr_irqs,
>                                  Error **errp)
>  {
> -    MachineState *machine = MACHINE(spapr);
>      Error *local_err = NULL;
>  
> -    if (kvm_enabled() && machine_kernel_irqchip_allowed(machine)) {
> -        error_setg(errp, "No KVM support for the 'dual' machine");
> -        return;
> -    }
> -
>      spapr_irq_xics.init(spapr, spapr_irq_xics.nr_irqs, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> @@ -557,6 +558,9 @@ static int spapr_irq_post_load_dual(SpaprMachineState 
> *spapr, int version_id)
>       * defaults to XICS at startup.
>       */
>      if (spapr_ovec_test(spapr->ov5_cas, OV5_XIVE_EXPLOIT)) {
> +        if (kvm_irqchip_in_kernel()) {
> +            xics_kvm_disconnect(spapr, &error_fatal);
> +        }
>          spapr_irq_xive.reset(spapr, &error_fatal);
>      }
>  
> @@ -565,12 +569,30 @@ static int spapr_irq_post_load_dual(SpaprMachineState 
> *spapr, int version_id)
>  
>  static void spapr_irq_reset_dual(SpaprMachineState *spapr, Error **errp)
>  {
> +    Error *local_err = NULL;
> +
>      /*
>       * Deactivate the XIVE MMIOs. The XIVE backend will reenable them
>       * if selected.
>       */
>      spapr_xive_mmio_set_enabled(spapr->xive, false);
>  
> +    /* Destroy all KVM devices */
> +    if (kvm_irqchip_in_kernel()) {
> +        xics_kvm_disconnect(spapr, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            error_prepend(errp, "KVM XICS disconnect failed: ");
> +            return;
> +        }
> +        kvmppc_xive_disconnect(spapr->xive, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            error_prepend(errp, "KVM XIVE disconnect failed: ");
> +            return;
> +        }
> +    }
> +
>      spapr_irq_current(spapr)->reset(spapr, errp);
>  }
>  
> @@ -759,6 +781,9 @@ SpaprIrq spapr_irq_xics_legacy = {
>      .dt_populate = spapr_dt_xics,
>      .cpu_intc_create = spapr_irq_cpu_intc_create_xics,
>      .post_load   = spapr_irq_post_load_xics,
> +    .reset       = spapr_irq_reset_xics,
>      .set_irq     = spapr_irq_set_irq_xics,
>      .get_nodename = spapr_irq_get_nodename_xics,
> +    .init_emu    = spapr_irq_init_emu_xics,
> +    .init_kvm    = spapr_irq_init_kvm_xics,
>  };

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