Re: [Qemu-devel] [RFC PATCH v2 06/21] ppc/xive: introduce handlers for interrupt sources

[David Gibson]

On Tue, Sep 19, 2017 at 05:08:21PM +0200, Cédric Le Goater wrote:
> On 09/19/2017 04:48 AM, David Gibson wrote:
> > On Mon, Sep 11, 2017 at 07:12:20PM +0200, Cédric Le Goater wrote:
> >> These are very similar to the XICS handlers in a simpler form. They
> >> make use of the ICSIRQState array of the XICS interrupt source to
> >> differentiate the MSI from the LSI interrupts. The spapr_xive_irq()
> >> routine in charge of triggering the CPU interrupt line will be filled
> >> later on.
> >>
> >> The next patch will introduce the MMIO handlers to interact with XIVE
> >> interrupt sources.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> >>  hw/intc/spapr_xive.c        | 46 
> >> +++++++++++++++++++++++++++++++++++++++++++++
> >>  include/hw/ppc/spapr_xive.h |  1 +
> >>  2 files changed, 47 insertions(+)
> >>
> >> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> >> index 52c32f588d6d..1ed7b6a286e9 100644
> >> --- a/hw/intc/spapr_xive.c
> >> +++ b/hw/intc/spapr_xive.c
> >> @@ -27,6 +27,50 @@
> >>  
> >>  #include "xive-internal.h"
> >>  
> >> +static void spapr_xive_irq(sPAPRXive *xive, int srcno)
> >> +{
> >> +
> >> +}
> >> +
> >> +/*
> >> + * XIVE Interrupt Source
> >> + */
> >> +static void spapr_xive_source_set_irq_msi(sPAPRXive *xive, int srcno, int 
> >> val)
> >> +{
> >> +    if (val) {
> >> +        spapr_xive_irq(xive, srcno);
> >> +    }
> >> +}
> > 
> > So in XICS "srcno" (vs "irq") indicates an offset within a single ICS
> > object, as opposed to a global irq number.  Does that concept even
> > exist in XIVE?
> 
> We don't really care in the internals. 'srcno' is just an index in the 
> tables, may be I should change the name. It could be the same in XICS 
> but the xirr is manipulated at low level and so we need to propagate 
> the source offset in a couple of places. 

Right.  My point is that the XICS code deliberately uses srcno vs. irq
names to identify which space we're talking about.  If we re-use the
srcno name in XIVE where it doesn't really apply that could be
misleading.

> This to say that the 'irq' number is a guest level information which
> in the patchset should only be used at the hcall level to identify 
> a source.

Right, and if there's no need to introduce a number space other than
the guest one, we should keep using that everywhere - and give it a
consistent name to avoid confusion.

>  
> >> +
> >> +static void spapr_xive_source_set_irq_lsi(sPAPRXive *xive, int srcno, int 
> >> val)
> >> +{
> >> +    ICSIRQState *irq = &xive->ics->irqs[srcno];
> >> +
> >> +    if (val) {
> >> +        irq->status |= XICS_STATUS_ASSERTED;
> >> +    } else {
> >> +        irq->status &= ~XICS_STATUS_ASSERTED;
> > 
> > More mangling a XICS specific object for XIVE operations.  Please
> > stop.
> 
> ah ! we will still need the same information and that means introducing 
> a common source object. The patchset today just uses the XICS ICSIRQState 
> array as a common object.

It's not really the same information though.  For XICS irq->status is
*all* the information about the line's state, for XIVE, most of that
info is in the PQ bits which are elsewhere.  That makes at least some
of the information in ICSIRQState redundant, and therefore confusing
and misleading.

> >> +    }
> >> +
> >> +    if (irq->status & XICS_STATUS_ASSERTED
> >> +        && !(irq->status & XICS_STATUS_SENT)) {
> >> +        irq->status |= XICS_STATUS_SENT;
> >> +        spapr_xive_irq(xive, srcno);
> >> +    }
> >> +}
> >> +
> >> +static void spapr_xive_source_set_irq(void *opaque, int srcno, int val)
> >> +{
> >> +    sPAPRXive *xive = SPAPR_XIVE(opaque);
> >> +    ICSIRQState *irq = &xive->ics->irqs[srcno];
> >> +
> >> +    if (irq->flags & XICS_FLAGS_IRQ_LSI) {
> >> +        spapr_xive_source_set_irq_lsi(xive, srcno, val);
> >> +    } else {
> >> +        spapr_xive_source_set_irq_msi(xive, srcno, val);
> >> +    }
> >> +}
> >> +
> >>  /*
> >>   * Main XIVE object
> >>   */
> >> @@ -80,6 +124,8 @@ static void spapr_xive_realize(DeviceState *dev, Error 
> >> **errp)
> >>      }
> >>  
> >>      xive->ics = ICS_BASE(obj);
> >> +    xive->qirqs = qemu_allocate_irqs(spapr_xive_source_set_irq, xive,
> >> +                                     xive->nr_irqs);
> >>  
> >>      /* Allocate the last IRQ numbers for the IPIs */
> >>      for (i = xive->nr_irqs - xive->nr_targets; i < xive->nr_irqs; i++) {
> >> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> >> index 29112589b37f..eab92c4c1bb8 100644
> >> --- a/include/hw/ppc/spapr_xive.h
> >> +++ b/include/hw/ppc/spapr_xive.h
> >> @@ -38,6 +38,7 @@ struct sPAPRXive {
> >>  
> >>      /* IRQ */
> >>      ICSState     *ics;  /* XICS source inherited from the SPAPR machine */
> >> +    qemu_irq     *qirqs;
> >>  
> >>      /* XIVE internal tables */
> >>      uint8_t      *sbe;
> > 
> 

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