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

["Cédric Le Goater" <clg@kaod.org>]

On 09/20/2017 06:38 AM, David Gibson wrote:
> 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 <clg@kaod.org>
>>>> ---
>>>>  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.

yes. ok I will be careful with the naming. 

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

yes. I agree. I think XICS could benefit from some cleanups.

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

This is true.

> That makes at least some of the information in ICSIRQState redundant,> and therefore confusing and misleading.

I will respin the patchset in a different way to distinguish 
xive from xics clearly. I will keep CAS and migration for later. 
The source should not be too complex to handle but I don't know 
for the ICP. 

Thanks,

C.

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