Re: [PATCH v2 24/33] spapr, xics, xive: Move set_irq from SpaprIrq to SpaprInterruptController

[David Gibson]

On Mon, Sep 30, 2019 at 09:22:16AM +0200, Greg Kurz wrote:
> On Mon, 30 Sep 2019 12:41:39 +1000
> David Gibson <address@hidden> wrote:
> 
> > On Fri, Sep 27, 2019 at 04:27:12PM +0200, Greg Kurz wrote:
> > > On Fri, 27 Sep 2019 15:50:19 +1000
> > > David Gibson <address@hidden> wrote:
> > > 
> > > > This method depends only on the active irq controller.  Now that we've
> > > > formalized the notion of active controller we can dispatch directly 
> > > > through
> > > > that, rather than dispatching via SpaprIrq with the dual version having
> > > > to do a second conditional dispatch.
> > > > 
> > > > Signed-off-by: David Gibson <address@hidden>
> > > > ---
> > > >  hw/intc/spapr_xive.c       | 12 +++++++++++
> > > >  hw/intc/xics_spapr.c       |  9 +++++++++
> > > >  hw/ppc/spapr_irq.c         | 41 ++++++++++----------------------------
> > > >  include/hw/ppc/spapr_irq.h |  4 +++-
> > > >  4 files changed, 34 insertions(+), 32 deletions(-)
> > > > 
> > > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> > > > index ff1a175b44..52d5e71793 100644
> > > > --- a/hw/intc/spapr_xive.c
> > > > +++ b/hw/intc/spapr_xive.c
> > > > @@ -553,6 +553,17 @@ static int 
> > > > spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
> > > >      return 0;
> > > >  }
> > > >  
> > > > +static void spapr_xive_set_irq(SpaprInterruptController *intc, int 
> > > > irq, int val)
> > > > +{
> > > > +    SpaprXive *xive = SPAPR_XIVE(intc);
> > > > +
> > > > +    if (kvm_irqchip_in_kernel()) {
> > > > +        kvmppc_xive_source_set_irq(&xive->source, irq, val);
> > > > +    } else {
> > > > +        xive_source_set_irq(&xive->source, irq, val);
> > > > +    }
> > > > +}
> > > > +
> > > >  static void spapr_xive_class_init(ObjectClass *klass, void *data)
> > > >  {
> > > >      DeviceClass *dc = DEVICE_CLASS(klass);
> > > > @@ -574,6 +585,7 @@ static void spapr_xive_class_init(ObjectClass 
> > > > *klass, void *data)
> > > >      sicc->cpu_intc_create = spapr_xive_cpu_intc_create;
> > > >      sicc->claim_irq = spapr_xive_claim_irq;
> > > >      sicc->free_irq = spapr_xive_free_irq;
> > > > +    sicc->set_irq = spapr_xive_set_irq;
> > > >  }
> > > >  
> > > >  static const TypeInfo spapr_xive_info = {
> > > > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> > > > index 224fe1efcd..02372697f6 100644
> > > > --- a/hw/intc/xics_spapr.c
> > > > +++ b/hw/intc/xics_spapr.c
> > > > @@ -373,6 +373,14 @@ static void 
> > > > xics_spapr_free_irq(SpaprInterruptController *intc, int irq)
> > > >      memset(&ics->irqs[srcno], 0, sizeof(ICSIRQState));
> > > >  }
> > > >  
> > > > +static void xics_spapr_set_irq(SpaprInterruptController *intc, int 
> > > > irq, int val)
> > > > +{
> > > > +    ICSState *ics = ICS_SPAPR(intc);
> > > > +    uint32_t srcno = irq - ics->offset;
> > > > +
> > > > +    ics_set_irq(ics, srcno, val);
> > > 
> > > And we have:
> > > 
> > > void ics_set_irq(void *opaque, int srcno, int val)
> > > {
> > >     ICSState *ics = (ICSState *)opaque;
> > > 
> > >     if (kvm_irqchip_in_kernel()) {
> > >         ics_kvm_set_irq(ics, srcno, val);
> > >         return;
> > >     }
> > > 
> > >     if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) {
> > >         ics_set_irq_lsi(ics, srcno, val);
> > >     } else {
> > >         ics_set_irq_msi(ics, srcno, val);
> > >     }
> > > }
> > > 
> > > The kvm_irqchip_in_kernel() block would fit better in 
> > > xics_spapr_set_irq(),
> > > like it is already the case for XIVE.
> > 
> > Hmm.. I don't really see why you say that.
> > 
> 
> I mean this:
> 
> static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, int 
> val)
> {
>     IcsSpapr *icss = ICS_SPAPR(intc);
>     ICSState *ics = &icss->parent;
>     uint32_t srcno = irq - ics->offset;
> 
>     if (kvm_irqchip_in_kernel()) {
>         ics_kvm_set_irq(ics, srcno, val);
>     } else {
>         ics_set_irq(ics, srcno, val);
>     }
> }
> 
> It is very similar to spapr_xive_set_irq() and looks nicer to me.

Eh.. I don't really agree.  Seems to me KVM is essentialy an
implementation detail, so belongs in the ICS proper rather than the
papr specific wrapper.

It's not really obvious where it belongs, because the KVM
implementation only works for the PAPR version of the thing, but the
way we call it doesn't explicitly depend on any PAPR specific
information.

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