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

[Cédric Le Goater]

On 07/24/2017 06:02 AM, 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.

yes. I agree. The patchset is here to discuss these model issues. 

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

OK. We should also discuss the IRQ number allocator. That's another 
email thread.

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

yes. I am not sure how to do this though. We could use some property 
on the ICS to know in which interrupt mode we are running and branch, 
but wouldn't that pollute a lot the current code ? 

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

Hmm, this is a copy paste of ics_simple_reset() but we can fix both.

> Does the xics irqstate structure really cover what you need for xive?

There is too much in it. We only need the flags to know if the IRQ is
allocated and if it is a LSI. In fact, the ICSIRQState array is the
only information we need to share to support resets between the XIVE 
and XICS modes. The allocator should be the same of course. But it
is a bit of a hack for now.

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

In the hcalls, we need to do ICS lookups using IRQ numbers and this
is when the ics_get() handler of the XICSFabric interface is used.
I agree we could find some other ways but that is what we have put
in place for sPAPR and PowerNV.

Thanks,

C. 

> 
>> +}
>> +
>> +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 */
>