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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6072 bytes --]

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

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]