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 <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 */
>
--
Alexey
signature.asc
Description: OpenPGP digital signature
