Re: [Qemu-devel] [PATCH v3 13/15] spapr/irq: introduce a spapr_irq_init_device() helper

[David Gibson]

On Thu, Mar 21, 2019 at 03:49:12PM +0100, Cédric Le Goater wrote:
> The way the XICS and the XIVE devices are initialized follows the same
> pattern. First, try to connect to the KVM device and if not possible
> fallback on the emulated device, unless a kernel_irqchip is required.
> The spapr_irq_init_device() routine implements this sequence in
> generic way using new sPAPR IRQ handlers ->init_emu() and ->init_kvm().
> 
> The XIVE init sequence is moved under the associated sPAPR IRQ
> ->init() handler. This will change again when KVM support is added for
> the dual interrupt mode.
> 
> Signed-off-by: Cédric Le Goater <address@hidden>

Reviewed-by: David Gibson <address@hidden>

> ---
>  include/hw/ppc/spapr_irq.h  |  2 +
>  include/hw/ppc/spapr_xive.h |  1 +
>  hw/intc/spapr_xive.c        | 26 +++--------
>  hw/ppc/spapr_irq.c          | 89 +++++++++++++++++++++++++++++--------
>  4 files changed, 78 insertions(+), 40 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index b855f74e4428..14cab73c9c07 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -48,6 +48,8 @@ typedef struct SpaprIrq {
>      void (*reset)(SpaprMachineState *spapr, Error **errp);
>      void (*set_irq)(void *opaque, int srcno, int val);
>      const char *(*get_nodename)(SpaprMachineState *spapr);
> +    void (*init_emu)(SpaprMachineState *spapr, Error **errp);
> +    void (*init_kvm)(SpaprMachineState *spapr, Error **errp);
>  } SpaprIrq;
>  
>  extern SpaprIrq spapr_irq_xics;
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index 0b5e972d52c8..b26befcf6b56 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -66,6 +66,7 @@ void spapr_xive_map_mmio(SpaprXive *xive);
>  
>  int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx,
>                               uint32_t *out_server, uint8_t *out_prio);
> +void spapr_xive_init(SpaprXive *xive, Error **errp);
>  
>  /*
>   * KVM XIVE device helpers
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index a3cbbdf7aff0..f889c89dc2e9 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -280,7 +280,6 @@ static void spapr_xive_realize(DeviceState *dev, Error 
> **errp)
>      XiveSource *xsrc = &xive->source;
>      XiveENDSource *end_xsrc = &xive->end_source;
>      Error *local_err = NULL;
> -    MachineState *machine = MACHINE(qdev_get_machine());
>  
>      if (!xive->nr_irqs) {
>          error_setg(errp, "Number of interrupt needs to be greater 0");
> @@ -331,27 +330,12 @@ static void spapr_xive_realize(DeviceState *dev, Error 
> **errp)
>                             xive->tm_base + XIVE_TM_USER_PAGE * (1 << 
> TM_SHIFT));
>  
>      qemu_register_reset(spapr_xive_reset, dev);
> +}
>  
> -    if (kvm_enabled() && machine_kernel_irqchip_allowed(machine)) {
> -        kvmppc_xive_connect(xive, &local_err);
> -        if (local_err && machine_kernel_irqchip_required(machine)) {
> -            error_prepend(&local_err,
> -                          "kernel_irqchip requested but unavailable: ");
> -            error_propagate(errp, local_err);
> -            return;
> -        }
> -
> -        if (!local_err) {
> -            return;
> -        }
> -
> -        /*
> -         * We failed to initialize the XIVE KVM device, fallback to
> -         * emulated mode
> -         */
> -        error_prepend(&local_err, "kernel_irqchip allowed but unavailable: 
> ");
> -        warn_report_err(local_err);
> -    }
> +void spapr_xive_init(SpaprXive *xive, Error **errp)
> +{
> +    XiveSource *xsrc = &xive->source;
> +    XiveENDSource *end_xsrc = &xive->end_source;
>  
>      /* TIMA initialization */
>      memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &xive_tm_ops, xive,
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index d7130f66dbaa..0c65de35f52a 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -61,36 +61,50 @@ void spapr_irq_msi_reset(SpaprMachineState *spapr)
>      bitmap_clear(spapr->irq_map, 0, spapr->irq_map_nr);
>  }
>  
> -
> -/*
> - * XICS IRQ backend.
> - */
> -
> -static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs,
> -                                Error **errp)
> +static void spapr_irq_init_device(SpaprMachineState *spapr,
> +                                  SpaprIrq *irq, Error **errp)
>  {
>      MachineState *machine = MACHINE(spapr);
> -    Object *obj;
>      Error *local_err = NULL;
> -    bool xics_kvm = false;
>  
> -    if (kvm_enabled()) {
> -        if (machine_kernel_irqchip_allowed(machine) &&
> -            !xics_kvm_init(spapr, &local_err)) {
> -            xics_kvm = true;
> -        }
> -        if (machine_kernel_irqchip_required(machine) && !xics_kvm) {
> +    if (kvm_enabled() && machine_kernel_irqchip_allowed(machine)) {
> +        irq->init_kvm(spapr, &local_err);
> +        if (local_err && machine_kernel_irqchip_required(machine)) {
>              error_prepend(&local_err,
>                            "kernel_irqchip requested but unavailable: ");
>              error_propagate(errp, local_err);
>              return;
>          }
> -        error_free(local_err);
> -        local_err = NULL;
> +
> +        if (!local_err) {
> +            return;
> +        }
> +
> +        /*
> +         * We failed to initialize the KVM device, fallback to
> +         * emulated mode
> +         */
> +        error_prepend(&local_err, "kernel_irqchip allowed but unavailable: 
> ");
> +        warn_report_err(local_err);
>      }
>  
> -    if (!xics_kvm) {
> -        xics_spapr_init(spapr);
> +    irq->init_emu(spapr, errp);
> +}
> +
> +/*
> + * XICS IRQ backend.
> + */
> +
> +static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs,
> +                                Error **errp)
> +{
> +    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);
> @@ -219,6 +233,18 @@ static const char 
> *spapr_irq_get_nodename_xics(SpaprMachineState *spapr)
>      return XICS_NODENAME;
>  }
>  
> +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)
> +{
> +    if (kvm_enabled()) {
> +        xics_kvm_init(spapr, errp);
> +    }
> +}
> +
>  #define SPAPR_IRQ_XICS_NR_IRQS     0x1000
>  #define SPAPR_IRQ_XICS_NR_MSIS     \
>      (XICS_IRQ_BASE + SPAPR_IRQ_XICS_NR_IRQS - SPAPR_IRQ_MSI)
> @@ -239,6 +265,8 @@ SpaprIrq spapr_irq_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,
>  };
>  
>  /*
> @@ -250,6 +278,7 @@ static void spapr_irq_init_xive(SpaprMachineState *spapr, 
> int nr_irqs,
>      uint32_t nr_servers = spapr_max_server_number(spapr);
>      DeviceState *dev;
>      int i;
> +    Error *local_err = NULL;
>  
>      dev = qdev_create(NULL, TYPE_SPAPR_XIVE);
>      qdev_prop_set_uint32(dev, "nr-irqs", nr_irqs);
> @@ -267,6 +296,12 @@ static void spapr_irq_init_xive(SpaprMachineState 
> *spapr, int nr_irqs,
>      }
>  
>      spapr_xive_hcall_init(spapr);
> +
> +    spapr_irq_init_device(spapr, &spapr_irq_xive, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  }
>  
>  static int spapr_irq_claim_xive(SpaprMachineState *spapr, int irq, bool lsi,
> @@ -374,6 +409,18 @@ static const char 
> *spapr_irq_get_nodename_xive(SpaprMachineState *spapr)
>      return spapr->xive->nodename;
>  }
>  
> +static void spapr_irq_init_emu_xive(SpaprMachineState *spapr, Error **errp)
> +{
> +    spapr_xive_init(spapr->xive, errp);
> +}
> +
> +static void spapr_irq_init_kvm_xive(SpaprMachineState *spapr, Error **errp)
> +{
> +    if (kvm_enabled()) {
> +        kvmppc_xive_connect(spapr->xive, errp);
> +    }
> +}
> +
>  /*
>   * XIVE uses the full IRQ number space. Set it to 8K to be compatible
>   * with XICS.
> @@ -398,6 +445,8 @@ SpaprIrq spapr_irq_xive = {
>      .reset       = spapr_irq_reset_xive,
>      .set_irq     = spapr_irq_set_irq_xive,
>      .get_nodename = spapr_irq_get_nodename_xive,
> +    .init_emu    = spapr_irq_init_emu_xive,
> +    .init_kvm    = spapr_irq_init_kvm_xive,
>  };
>  
>  /*
> @@ -559,6 +608,8 @@ SpaprIrq spapr_irq_dual = {
>      .reset       = spapr_irq_reset_dual,
>      .set_irq     = spapr_irq_set_irq_dual,
>      .get_nodename = spapr_irq_get_nodename_dual,
> +    .init_emu    = NULL, /* should not be used */
> +    .init_kvm    = NULL, /* should not be used */
>  };
>  
>  /*

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