Re: [Qemu-devel] [PATCH v5 34/36] spapr: add KVM support to the 'dual' machine

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 13583 bytes --]

On Fri, Nov 16, 2018 at 11:57:27AM +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 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 and keep the IRQ number space in sync 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 delete
> sequences of the KVM device. The post_load handlers also are, to take
> into account a possible change of interrupt mode after transfer.
> 
> As KVM is now initialized at reset, we loose the possiblity to
> fallback to the QEMU emulated mode in case of failure and failures
> become fatal to the machine.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/intc/spapr_xive_kvm.c | 48 +++++++++++-----------
>  hw/intc/xics_kvm.c       | 18 ++++++---
>  hw/ppc/spapr_irq.c       | 86 +++++++++++++++++++++++++++++-----------
>  3 files changed, 98 insertions(+), 54 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 0672d8bcbc6b..9c7d36f51e3d 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -148,7 +148,6 @@ static void xive_tctx_kvm_init(XiveTCTX *tctx, Error **errp)
>  
>  static void xive_tctx_kvm_realize(DeviceState *dev, Error **errp)
>  {
> -    XiveTCTX *tctx = XIVE_TCTX_KVM(dev);
>      XiveTCTXClass *xtc = XIVE_TCTX_BASE_GET_CLASS(dev);
>      Error *local_err = NULL;
>  
> @@ -157,12 +156,6 @@ static void xive_tctx_kvm_realize(DeviceState *dev, Error **errp)
>          error_propagate(errp, local_err);
>          return;
>      }
> -
> -    xive_tctx_kvm_init(tctx, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
>  }
>  
>  static void xive_tctx_kvm_class_init(ObjectClass *klass, void *data)
> @@ -222,12 +215,9 @@ static void xive_source_kvm_init(XiveSource *xsrc, Error **errp)
>  
>  static void xive_source_kvm_reset(DeviceState *dev)
>  {
> -    XiveSource *xsrc = XIVE_SOURCE_KVM(dev);
>      XiveSourceClass *xsc = XIVE_SOURCE_BASE_GET_CLASS(dev);
>  
>      xsc->parent_reset(dev);
> -
> -    xive_source_kvm_init(xsrc, &error_fatal);
>  }
>  
>  /*
> @@ -346,12 +336,6 @@ static void xive_source_kvm_realize(DeviceState *dev, Error **errp)
>  
>      xsrc->qirqs = qemu_allocate_irqs(xive_source_kvm_set_irq, xsrc,
>                                       xsrc->nr_irqs);
> -
> -    xive_source_kvm_mmap(xsrc, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
>  }
>  
>  static void xive_source_kvm_unrealize(DeviceState *dev, Error **errp)
> @@ -823,6 +807,7 @@ void spapr_xive_kvm_init(sPAPRXive *xive, Error **errp)
>  {
>      Error *local_err = NULL;
>      size_t tima_len;
> +    CPUState *cs;
>  
>      if (!kvm_enabled() || !kvmppc_has_cap_xive()) {
>          error_setg(errp,
> @@ -850,7 +835,18 @@ void spapr_xive_kvm_init(sPAPRXive *xive, Error **errp)
>          return;
>      }
>  
> -    /* Let the XiveSource KVM model handle the mapping for the moment */
> +    xive_source_kvm_mmap(&xive->source, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    /* Create the KVM interrupt sources */
> +    xive_source_kvm_init(&xive->source, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      /* TIMA KVM mapping
>       *
> @@ -869,6 +865,17 @@ void spapr_xive_kvm_init(sPAPRXive *xive, Error **errp)
>                                        "xive.tima", tima_len, xive->tm_mmap);
>      sysbus_init_mmio(SYS_BUS_DEVICE(xive), &xive->tm_mmio);
>  
> +    /* Connect the presenters to the VCPU */
> +    CPU_FOREACH(cs) {
> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> +        xive_tctx_kvm_init(XIVE_TCTX_BASE(cpu->intc), &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;
> @@ -920,16 +927,9 @@ void spapr_xive_kvm_fini(sPAPRXive *xive, Error **errp)
>  
>  static void spapr_xive_kvm_realize(DeviceState *dev, Error **errp)
>  {
> -    sPAPRXive *xive = SPAPR_XIVE_KVM(dev);
>      sPAPRXiveClass *sxc = SPAPR_XIVE_BASE_GET_CLASS(dev);
>      Error *local_err = NULL;
>  
> -    spapr_xive_kvm_init(xive, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> -
>      /* Initialize the source and the local routing tables */
>      sxc->parent_realize(dev, &local_err);
>      if (local_err) {
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index a7e3ec32a761..c89fa943847c 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -190,12 +190,6 @@ static void icp_kvm_realize(DeviceState *dev, Error **errp)
>          error_propagate(errp, local_err);
>          return;
>      }
> -
> -    icp_kvm_init(ICP(dev), &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
>  }
>  
>  static void icp_kvm_class_init(ObjectClass *klass, void *data)
> @@ -427,6 +421,8 @@ static void rtas_dummy(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  int xics_kvm_init(sPAPRMachineState *spapr, Error **errp)
>  {
>      int rc;
> +    CPUState *cs;
> +    Error *local_err = NULL;
>  
>      if (!kvm_enabled() || !kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS)) {
>          error_setg(errp,
> @@ -475,6 +471,16 @@ int xics_kvm_init(sPAPRMachineState *spapr, Error **errp)
>      kvm_msi_via_irqfd_allowed = true;
>      kvm_gsi_direct_mapping = true;
>  
> +    /* Connect the presenters to the VCPU */
> +    CPU_FOREACH(cs) {
> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> +        icp_kvm_init(ICP(cpu->intc), &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            goto fail;
> +        }
> +    }
>      return 0;
>  
>  fail:
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 79ead51c630d..f1720a8dda33 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -98,20 +98,14 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
>      MachineState *machine = MACHINE(spapr);
>      Error *local_err = NULL;
>  
> -    if (kvm_enabled()) {
> -        if (machine_kernel_irqchip_allowed(machine) &&
> -            !xics_kvm_init(spapr, &local_err)) {
> -            spapr->icp_type = TYPE_KVM_ICP;
> -            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
> -                                          &local_err);
> -        }
> -        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> -            error_prepend(&local_err,
> -                          "kernel_irqchip requested but unavailable: ");
> -            goto error;
> +    if (kvm_enabled() && machine_kernel_irqchip_allowed(machine)) {
> +        spapr->icp_type = TYPE_KVM_ICP;
> +        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
> +                                      &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
>          }
> -        error_free(local_err);
> -        local_err = NULL;
>      }
>  
>      if (!spapr->ics) {
> @@ -119,10 +113,11 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
>          spapr->icp_type = TYPE_ICP;
>          spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
>                                        &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
>      }
> -
> -error:
> -    error_propagate(errp, local_err);
>  }
>  
>  #define ICS_IRQ_FREE(ics, srcno)   \
> @@ -218,11 +213,28 @@ static int spapr_irq_post_load_xics(sPAPRMachineState *spapr, int version_id)
>  
>  static void spapr_irq_reset_xics(sPAPRMachineState *spapr, Error **errp)
>  {
> +    MachineState *machine = MACHINE(spapr);
>      CPUState *cs;
> +    Error *local_err = NULL;
>  
>      CPU_FOREACH(cs) {
>          spapr_cpu_core_set_intc(POWERPC_CPU(cs), spapr->icp_type);
>      }
> +
> +    if (kvm_enabled() && machine_kernel_irqchip_allowed(machine)) {

Aren't both devices '_fini'-ed by the machine level reset handler, why
does it need a _fini here as well as an init?

> +        xics_kvm_fini(spapr, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            error_prepend(errp, "KVM XICS fini failed: ");
> +            return;
> +        }
> +        xics_kvm_init(spapr, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            error_prepend(errp, "KVM XICS init failed: ");
> +            return;
> +        }
> +    }
>  }
>  
>  #define SPAPR_IRQ_XICS_NR_IRQS     0x1000
> @@ -288,10 +300,8 @@ static void spapr_irq_init_xive(sPAPRMachineState *spapr, int nr_irqs,
>          spapr->xive_tctx_type = TYPE_XIVE_TCTX_KVM;
>          spapr->xive = spapr_xive_create(spapr, TYPE_SPAPR_XIVE_KVM, nr_irqs,
>                                          nr_servers, &local_err);
> -
> -        if (local_err && machine_kernel_irqchip_required(machine)) {
> +        if (local_err) {
>              error_propagate(errp, local_err);
> -            error_prepend(errp, "kernel_irqchip requested but init failed : ");
>              return;
>          }
>  
> @@ -375,12 +385,29 @@ static int spapr_irq_post_load_xive(sPAPRMachineState *spapr, int version_id)
>  
>  static void spapr_irq_reset_xive(sPAPRMachineState *spapr, Error **errp)
>  {
> +    MachineState *machine = MACHINE(spapr);
>      CPUState *cs;
> +    Error *local_err = NULL;
>  
>      CPU_FOREACH(cs) {
>          spapr_cpu_core_set_intc(POWERPC_CPU(cs), spapr->xive_tctx_type);
>      }
>  
> +    if (kvm_enabled() && machine_kernel_irqchip_allowed(machine)) {
> +        spapr_xive_kvm_fini(spapr->xive, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            error_prepend(errp, "KVM XIVE fini failed: ");
> +            return;
> +        }
> +        spapr_xive_kvm_init(spapr->xive, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            error_prepend(errp, "KVM XIVE init failed: ");
> +            return;
> +        }
> +    }
> +
>      spapr_xive_mmio_map(spapr->xive);
>  }
>  
> @@ -432,11 +459,6 @@ static void spapr_irq_init_dual(sPAPRMachineState *spapr, int nr_irqs,
>  {
>      Error *local_err = NULL;
>  
> -    if (kvm_enabled()) {
> -        error_setg(errp, "No KVM support for the 'dual' machine");
> -        return;
> -    }
> -
>      spapr_irq_xics.init(spapr, spapr_irq_xics.nr_irqs, nr_servers, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> @@ -510,10 +532,15 @@ static Object *spapr_irq_cpu_intc_create_dual(sPAPRMachineState *spapr,
>  
>  static int spapr_irq_post_load_dual(sPAPRMachineState *spapr, int version_id)
>  {
> +    MachineState *machine = MACHINE(spapr);
> +
>      /*
>       * Force a reset of the XIVE backend after migration.
>       */
>      if (spapr_ovec_test(spapr->ov5_cas, OV5_XIVE_EXPLOIT)) {
> +        if (kvm_enabled() && machine_kernel_irqchip_allowed(machine)) {
> +            xics_kvm_fini(spapr, &error_fatal);
> +        }
>          spapr_irq_xive.reset(spapr, &error_fatal);
>      }
>  
> @@ -522,6 +549,17 @@ static int spapr_irq_post_load_dual(sPAPRMachineState *spapr, int version_id)
>  
>  static void spapr_irq_reset_dual(sPAPRMachineState *spapr, Error **errp)
>  {
> +    MachineState *machine = MACHINE(spapr);
> +
> +    /*
> +     * Destroy all the KVM IRQ devices. This also clears the VCPU
> +     * presenters
> +     */
> +    if (kvm_enabled() && machine_kernel_irqchip_allowed(machine)) {
> +        xics_kvm_fini(spapr, &error_fatal);
> +        spapr_xive_kvm_fini(spapr->xive, &error_fatal);
> +    }
> +
>      /*
>       * Only XICS is reseted at startup as it is the default interrupt
>       * mode.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]