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 <c...@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 */
>>
>
>
