Re: [Qemu-devel] [RFC PATCH 06/26] ppc/xive: introduce a XIVE interrupt source model

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

On 07/24/2017 08:00 AM, Alexey Kardashevskiy wrote:
> On 24/07/17 14:02, David Gibson wrote:
>> On Wed, Jul 05, 2017 at 07:13:19PM +0200, Cédric Le Goater wrote:
>>> This is very similar to the current ICS_SIMPLE model in XICS. We try
>>> to reuse the ICS model because the sPAPR machine is tied to the
>>> XICSFabric interface and should be using a common framework to switch
>>> from one controller model to another: XICS <-> XIVE.
>>
>> Hm.  I'm not entirely concvinced re-using the xics ICSState class in
>> this way is a good idea, though maybe it's a reasonable first step.
>> With this patch alone some code is shared, but there are some real
>> uglies around the edges.
> 
> 
> Agree, using the "ICS" term in XIVE is quite confusing as "ICS" is not
> mentioned in neither XIVE nor P9 specs.

Indeed. 

The XIVE specs mention Source Controller (P3SC) or Interrupt 
Virtualization Source Engine (IVSE). The sPAPR specs use 
Interrupt Source a lot.

Let's unify them all under one name ? I propose ICS :)

Thanks,

C. 


 
>>
>> Seems to me at least long term you need to either 1) make the XIVE ics
>> separate, even if it has similarities to the XICS one or 2) truly
>> unify them, with a common base type and methods to handle the
>> differences.
>>
>>
>>> 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/xive.c        | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  include/hw/ppc/xive.h |  12 ++++++
>>>  2 files changed, 122 insertions(+)
>>>
>>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>>> index 5b14d8155317..9ff14c0da595 100644
>>> --- a/hw/intc/xive.c
>>> +++ b/hw/intc/xive.c
>>> @@ -26,6 +26,115 @@
>>>  
>>>  #include "xive-internal.h"
>>>  
>>> +static void xive_icp_irq(XiveICSState *xs, int lisn)
>>> +{
>>> +
>>> +}
>>> +
>>> +/*
>>> + * XIVE Interrupt Source
>>> + */
>>> +static void xive_ics_set_irq_msi(XiveICSState *xs, int srcno, int val)
>>> +{
>>> +    if (val) {
>>> +        xive_icp_irq(xs, srcno + ICS_BASE(xs)->offset);
>>> +    }
>>> +}
>>> +
>>> +static void xive_ics_set_irq_lsi(XiveICSState *xs, int srcno, int val)
>>> +{
>>> +    ICSIRQState *irq = &ICS_BASE(xs)->irqs[srcno];
>>> +
>>> +    if (val) {
>>> +        irq->status |= XICS_STATUS_ASSERTED;
>>> +    } else {
>>> +        irq->status &= ~XICS_STATUS_ASSERTED;
>>> +    }
>>> +
>>> +    if (irq->status & XICS_STATUS_ASSERTED
>>> +        && !(irq->status & XICS_STATUS_SENT)) {
>>> +        irq->status |= XICS_STATUS_SENT;
>>> +        xive_icp_irq(xs, srcno + ICS_BASE(xs)->offset);
>>> +    }
>>> +}
>>> +
>>> +static void xive_ics_set_irq(void *opaque, int srcno, int val)
>>> +{
>>> +    XiveICSState *xs = ICS_XIVE(opaque);
>>> +    ICSIRQState *irq = &ICS_BASE(xs)->irqs[srcno];
>>> +
>>> +    if (irq->flags & XICS_FLAGS_IRQ_LSI) {
>>> +        xive_ics_set_irq_lsi(xs, srcno, val);
>>> +    } else {
>>> +        xive_ics_set_irq_msi(xs, srcno, val);
>>> +    }
>>> +}
>>
>> e.g. you have some code re-use, but still need to more-or-less
>> duplicate the set_irq code as above.
>>
>>> +static void xive_ics_reset(void *dev)
>>> +{
>>> +    ICSState *ics = ICS_BASE(dev);
>>> +    int i;
>>> +    uint8_t flags[ics->nr_irqs];
>>> +
>>> +    for (i = 0; i < ics->nr_irqs; i++) {
>>> +        flags[i] = ics->irqs[i].flags;
>>> +    }
>>> +
>>> +    memset(ics->irqs, 0, sizeof(ICSIRQState) * ics->nr_irqs);
>>> +
>>> +    for (i = 0; i < ics->nr_irqs; i++) {
>>> +        ics->irqs[i].flags = flags[i];
>>> +    }
>>
>> This save, clear, restore is also kind ugly.  I'm also not sure why
>> this needs a reset method when I can't find one for the xics ICS.
>>
>> Does the xics irqstate structure really cover what you need for xive?
>> I had the impression elsewhere that xive had a different priority
>> model to xics.  And there's the xics pointer in the icsstate structure
>> which is definitely redundant.
>>
>>> +}
>>> +
>>> +static void xive_ics_realize(ICSState *ics, Error **errp)
>>> +{
>>> +    XiveICSState *xs = ICS_XIVE(ics);
>>> +    Object *obj;
>>> +    Error *err = NULL;
>>> +
>>> +    obj = object_property_get_link(OBJECT(xs), "xive", &err);
>>> +    if (!obj) {
>>> +        error_setg(errp, "%s: required link 'xive' not found: %s",
>>> +                   __func__, error_get_pretty(err));
>>> +        return;
>>> +    }
>>> +    xs->xive = XIVE(obj);
>>> +
>>> +    if (!ics->nr_irqs) {
>>> +        error_setg(errp, "Number of interrupts needs to be greater 0");
>>> +        return;
>>> +    }
>>> +
>>> +    ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
>>> +    ics->qirqs = qemu_allocate_irqs(xive_ics_set_irq, xs, ics->nr_irqs);
>>> +
>>> +    qemu_register_reset(xive_ics_reset, xs);
>>> +}
>>> +
>>> +static Property xive_ics_properties[] = {
>>> +    DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0),
>>> +    DEFINE_PROP_UINT32("irq-base", ICSState, offset, 0),
>>> +    DEFINE_PROP_END_OF_LIST(),
>>> +};
>>> +
>>> +static void xive_ics_class_init(ObjectClass *klass, void *data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>> +    ICSStateClass *isc = ICS_BASE_CLASS(klass);
>>> +
>>> +    isc->realize = xive_ics_realize;
>>> +
>>> +    dc->props = xive_ics_properties;
>>> +}
>>> +
>>> +static const TypeInfo xive_ics_info = {
>>> +    .name = TYPE_ICS_XIVE,
>>> +    .parent = TYPE_ICS_BASE,
>>> +    .instance_size = sizeof(XiveICSState),
>>> +    .class_init = xive_ics_class_init,
>>> +};
>>> +
>>>  /*
>>>   * Main XIVE object
>>>   */
>>> @@ -123,6 +232,7 @@ static const TypeInfo xive_info = {
>>>  static void xive_register_types(void)
>>>  {
>>>      type_register_static(&xive_info);
>>> +    type_register_static(&xive_ics_info);
>>>  }
>>>  
>>>  type_init(xive_register_types)
>>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>>> index 863f5a9c6b5f..544cc6e0c796 100644
>>> --- a/include/hw/ppc/xive.h
>>> +++ b/include/hw/ppc/xive.h
>>> @@ -19,9 +19,21 @@
>>>  #ifndef PPC_XIVE_H
>>>  #define PPC_XIVE_H
>>>  
>>> +#include "hw/ppc/xics.h"
>>> +
>>>  typedef struct XIVE XIVE;
>>> +typedef struct XiveICSState XiveICSState;
>>>  
>>>  #define TYPE_XIVE "xive"
>>>  #define XIVE(obj) OBJECT_CHECK(XIVE, (obj), TYPE_XIVE)
>>>  
>>> +#define TYPE_ICS_XIVE "xive-source"
>>> +#define ICS_XIVE(obj) OBJECT_CHECK(XiveICSState, (obj), TYPE_ICS_XIVE)
>>> +
>>> +struct XiveICSState {
>>> +    ICSState parent_obj;
>>> +
>>> +    XIVE         *xive;
>>> +};
>>
>>>  #endif /* PPC_XIVE_H */
>>
> 
>