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

[Greg Kurz <groug@kaod.org>]

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

On Wed, 13 Feb 2019 14:26:01 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> 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 <groug@kaod.org>  
> 
> 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.
> 

Yeah. We'll see what comes out of the discussion at:

    https://patchwork.ozlabs.org/patch/1021496/

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


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