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

[Cédric Le Goater]

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. 

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.
 
>> +
>> +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.
 
>> +    }
>> +
>> +    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;
>