| Message ID | 1499274819-15607-7-git-send-email-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
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. 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 */
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 <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 */ >
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 <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. 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 */ >
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 */ >> > >
On 25/07/17 01:20, Cédric Le Goater wrote: > 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 :) Too late because it is a part of XICS :) Ben calls them "source controller", seems appropriate.
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); + } +} + +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]; + } +} + +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 */
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. 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(+)