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

[Alexey Kardashevskiy]

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.

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


-- 
Alexey

signature.asc
Description: OpenPGP digital signature