Re: [qemu-s390x] [PATCH v4 01/15] spapr_irq: Add an @xics_offset field to sPAPRIrq

[David Gibson]

On Tue, Feb 12, 2019 at 07:24:00PM +0100, Greg Kurz wrote:
> Only pseries machines, either recent ones started with ic-mode=xics
> or older ones using the legacy irq allocation scheme, need to set the
> @offset of the ICS to XICS_IRQ_BASE. Recent pseries started with
> ic-mode=dual set it to 0 and powernv machines set it to some other
> value at runtime.
> 
> It thus doesn't really help to set the default value of the ICS offset
> to XICS_IRQ_BASE in ics_base_instance_init().
> 
> Drop that code from XICS and let the pseries code set the offset
> explicitely for clarity.
> 
> Signed-off-by: Greg Kurz <address@hidden>

So this actually relates to a discussion I've had on some of Cédric's
more recent patches.  Changing the ics offset in ic-mode=dual doesn't
make sense to me.  The global (guest) interrupt numbers need to match
between XICS and XIVE, but the global interrupt numbers don't have to
match the ICS source numbers, which is what ics->offset is about.

> ---
>  hw/intc/xics.c             |    8 --------
>  hw/ppc/spapr_irq.c         |   33 ++++++++++++++++++++-------------
>  include/hw/ppc/spapr_irq.h |    1 +
>  3 files changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 16e8ffa2aaf7..7cac138067e2 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -638,13 +638,6 @@ static void ics_base_realize(DeviceState *dev, Error 
> **errp)
>      ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
>  }
>  
> -static void ics_base_instance_init(Object *obj)
> -{
> -    ICSState *ics = ICS_BASE(obj);
> -
> -    ics->offset = XICS_IRQ_BASE;
> -}
> -
>  static int ics_base_dispatch_pre_save(void *opaque)
>  {
>      ICSState *ics = opaque;
> @@ -720,7 +713,6 @@ static const TypeInfo ics_base_info = {
>      .parent = TYPE_DEVICE,
>      .abstract = true,
>      .instance_size = sizeof(ICSState),
> -    .instance_init = ics_base_instance_init,
>      .class_init = ics_base_class_init,
>      .class_size = sizeof(ICSStateClass),
>  };
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 80b0083b8e38..8217e0215411 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -68,10 +68,11 @@ void spapr_irq_msi_reset(sPAPRMachineState *spapr)
>  
>  static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
>                                    const char *type_ics,
> -                                  int nr_irqs, Error **errp)
> +                                  int nr_irqs, int offset, Error **errp)
>  {
>      Error *local_err = NULL;
>      Object *obj;
> +    ICSState *ics;
>  
>      obj = object_new(type_ics);
>      object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
> @@ -86,7 +87,10 @@ static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
>          goto error;
>      }
>  
> -    return ICS_BASE(obj);
> +    ics = ICS_BASE(obj);
> +    ics->offset = offset;
> +
> +    return ics;
>  
>  error:
>      error_propagate(errp, local_err);
> @@ -104,6 +108,7 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, 
> Error **errp)
>              !xics_kvm_init(spapr, &local_err)) {
>              spapr->icp_type = TYPE_KVM_ICP;
>              spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
> +                                          spapr->irq->xics_offset,
>                                            &local_err);
>          }
>          if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> @@ -119,6 +124,7 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, 
> Error **errp)
>          xics_spapr_init(spapr);
>          spapr->icp_type = TYPE_ICP;
>          spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
> +                                      spapr->irq->xics_offset,
>                                        &local_err);
>      }
>  
> @@ -246,6 +252,7 @@ sPAPRIrq spapr_irq_xics = {
>      .nr_irqs     = SPAPR_IRQ_XICS_NR_IRQS,
>      .nr_msis     = SPAPR_IRQ_XICS_NR_MSIS,
>      .ov5         = SPAPR_OV5_XIVE_LEGACY,
> +    .xics_offset = XICS_IRQ_BASE,
>  
>      .init        = spapr_irq_init_xics,
>      .claim       = spapr_irq_claim_xics,
> @@ -451,17 +458,6 @@ static void spapr_irq_init_dual(sPAPRMachineState 
> *spapr, Error **errp)
>          return;
>      }
>  
> -    /*
> -     * Align the XICS and the XIVE IRQ number space under QEMU.
> -     *
> -     * However, the XICS KVM device still considers that the IRQ
> -     * numbers should start at XICS_IRQ_BASE (0x1000). Either we
> -     * should introduce a KVM device ioctl to set the offset or ignore
> -     * the lower 4K numbers when using the get/set ioctl of the XICS
> -     * KVM device. The second option seems the least intrusive.
> -     */
> -    spapr->ics->offset = 0;
> -
>      spapr_irq_xive.init(spapr, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> @@ -582,6 +578,16 @@ sPAPRIrq spapr_irq_dual = {
>      .nr_irqs     = SPAPR_IRQ_DUAL_NR_IRQS,
>      .nr_msis     = SPAPR_IRQ_DUAL_NR_MSIS,
>      .ov5         = SPAPR_OV5_XIVE_BOTH,
> +    /*
> +     * Align the XICS and the XIVE IRQ number space under QEMU.
> +     *
> +     * However, the XICS KVM device still considers that the IRQ
> +     * numbers should start at XICS_IRQ_BASE (0x1000). Either we
> +     * should introduce a KVM device ioctl to set the offset or ignore
> +     * the lower 4K numbers when using the get/set ioctl of the XICS
> +     * KVM device. The second option seems the least intrusive.
> +     */
> +    .xics_offset = 0,
>  
>      .init        = spapr_irq_init_dual,
>      .claim       = spapr_irq_claim_dual,
> @@ -712,6 +718,7 @@ sPAPRIrq spapr_irq_xics_legacy = {
>      .nr_irqs     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
>      .nr_msis     = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
>      .ov5         = SPAPR_OV5_XIVE_LEGACY,
> +    .xics_offset = XICS_IRQ_BASE,
>  
>      .init        = spapr_irq_init_xics,
>      .claim       = spapr_irq_claim_xics,
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index 14b02c3aca33..5e30858dc22a 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -34,6 +34,7 @@ typedef struct sPAPRIrq {
>      uint32_t    nr_irqs;
>      uint32_t    nr_msis;
>      uint8_t     ov5;
> +    uint32_t    xics_offset;
>  
>      void (*init)(sPAPRMachineState *spapr, Error **errp);
>      int (*claim)(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp);
> 

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