Re: [Qemu-devel] [PATCH v5 10/36] spapr/xive: introduce a XIVE interrupt controller

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

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

On Fri, Nov 16, 2018 at 11:57:03AM +0100, Cédric Le Goater wrote:
> sPAPRXive models the XIVE interrupt controller of the sPAPR machine.
> It inherits from the XiveRouter and provisions storage for the routing
> tables :
> 
>   - Event Assignment Structure (EAS)
>   - Event Notification Descriptor (END)
> 
> The sPAPRXive model incorporates an internal XiveSource for the IPIs
> and for the interrupts of the virtual devices of the guest. This model
> is consistent with XIVE architecture which also incorporates an
> internal IVSE for IPIs and accelerator interrupts in the IVRE
> sub-engine.
> 
> The sPAPRXive model exports two memory regions, one for the ESB
> trigger and management pages used to control the sources and one for
> the TIMA pages. They are mapped by default at the addresses found on
> chip 0 of a baremetal system. This is also consistent with the XIVE
> architecture which defines a Virtualization Controller BAR for the
> internal IVSE ESB pages and a Thread Managment BAR for the TIMA.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  default-configs/ppc64-softmmu.mak |   1 +
>  include/hw/ppc/spapr_xive.h       |  46 +++++
>  hw/intc/spapr_xive.c              | 323 ++++++++++++++++++++++++++++++
>  hw/intc/Makefile.objs             |   1 +
>  4 files changed, 371 insertions(+)
>  create mode 100644 include/hw/ppc/spapr_xive.h
>  create mode 100644 hw/intc/spapr_xive.c
> 
> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
> index 2d1e7c5c4668..7f34ad0528ed 100644
> --- a/default-configs/ppc64-softmmu.mak
> +++ b/default-configs/ppc64-softmmu.mak
> @@ -17,6 +17,7 @@ CONFIG_XICS=$(CONFIG_PSERIES)
>  CONFIG_XICS_SPAPR=$(CONFIG_PSERIES)
>  CONFIG_XICS_KVM=$(call land,$(CONFIG_PSERIES),$(CONFIG_KVM))
>  CONFIG_XIVE=$(CONFIG_PSERIES)
> +CONFIG_XIVE_SPAPR=$(CONFIG_PSERIES)
>  CONFIG_MEM_DEVICE=y
>  CONFIG_DIMM=y
>  CONFIG_SPAPR_RNG=y
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> new file mode 100644
> index 000000000000..06727bd86aa9
> --- /dev/null
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -0,0 +1,46 @@
> +/*
> + * QEMU PowerPC sPAPR XIVE interrupt controller model
> + *
> + * Copyright (c) 2017-2018, IBM Corporation.
> + *
> + * This code is licensed under the GPL version 2 or later. See the
> + * COPYING file in the top-level directory.
> + */
> +
> +#ifndef PPC_SPAPR_XIVE_H
> +#define PPC_SPAPR_XIVE_H
> +
> +#include "hw/sysbus.h"
> +#include "hw/ppc/xive.h"
> +
> +#define TYPE_SPAPR_XIVE "spapr-xive"
> +#define SPAPR_XIVE(obj) OBJECT_CHECK(sPAPRXive, (obj), TYPE_SPAPR_XIVE)
> +
> +typedef struct sPAPRXive {
> +    XiveRouter    parent;
> +
> +    /* Internal interrupt source for IPIs and virtual devices */
> +    XiveSource    source;
> +    hwaddr        vc_base;
> +
> +    /* END ESB MMIOs */
> +    XiveENDSource end_source;
> +    hwaddr        end_base;
> +
> +    /* Routing table */
> +    XiveEAS       *eat;
> +    uint32_t      nr_irqs;
> +    XiveEND       *endt;
> +    uint32_t      nr_ends;
> +
> +    /* TIMA mapping address */
> +    hwaddr        tm_base;
> +    MemoryRegion  tm_mmio;
> +} sPAPRXive;
> +
> +bool spapr_xive_irq_enable(sPAPRXive *xive, uint32_t lisn, bool lsi);
> +bool spapr_xive_irq_disable(sPAPRXive *xive, uint32_t lisn);
> +void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
> +qemu_irq spapr_xive_qirq(sPAPRXive *xive, uint32_t lisn);
> +
> +#endif /* PPC_SPAPR_XIVE_H */
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> new file mode 100644
> index 000000000000..5d038146c08e
> --- /dev/null
> +++ b/hw/intc/spapr_xive.c
> @@ -0,0 +1,323 @@
> +/*
> + * QEMU PowerPC sPAPR XIVE interrupt controller model
> + *
> + * Copyright (c) 2017-2018, IBM Corporation.
> + *
> + * This code is licensed under the GPL version 2 or later. See the
> + * COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qapi/error.h"
> +#include "target/ppc/cpu.h"
> +#include "sysemu/cpus.h"
> +#include "monitor/monitor.h"
> +#include "hw/ppc/spapr.h"
> +#include "hw/ppc/spapr_xive.h"
> +#include "hw/ppc/xive.h"
> +#include "hw/ppc/xive_regs.h"
> +
> +/*
> + * XIVE Virtualization Controller BAR and Thread Managment BAR that we
> + * use for the ESB pages and the TIMA pages
> + */
> +#define SPAPR_XIVE_VC_BASE   0x0006010000000000ull
> +#define SPAPR_XIVE_TM_BASE   0x0006030203180000ull
> +
> +void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon)
> +{
> +    int i;
> +    uint32_t offset = 0;
> +
> +    monitor_printf(mon, "XIVE Source %08x .. %08x\n", offset,
> +                   offset + xive->source.nr_irqs - 1);
> +    xive_source_pic_print_info(&xive->source, offset, mon);
> +
> +    monitor_printf(mon, "XIVE EAT %08x .. %08x\n", 0, xive->nr_irqs - 1);
> +    for (i = 0; i < xive->nr_irqs; i++) {
> +        xive_eas_pic_print_info(&xive->eat[i], i, mon);
> +    }
> +
> +    monitor_printf(mon, "XIVE ENDT %08x .. %08x\n", 0, xive->nr_ends - 1);
> +    for (i = 0; i < xive->nr_ends; i++) {
> +        xive_end_pic_print_info(&xive->endt[i], i, mon);
> +    }

AIUI the PAPR model hides the details of ENDs, EQs and NVTs - instead
each logical EAS just points at a (thread, priority) pair, which under
the hood has exactly one END and one NVT bound to it.

Given that, would it make more sense to reformat the info here to show
things in terms of those (thread, priority) pairs, rather than the
internal EAS and END details?

> +}
> +
> +/* Map the ESB pages and the TIMA pages */
> +static void spapr_xive_mmio_map(sPAPRXive *xive)
> +{
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&xive->source), 0, xive->vc_base);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&xive->end_source), 0, xive->end_base);

Uh.. I didn't think the PAPR model exposed the END sources to the guest?

> +    sysbus_mmio_map(SYS_BUS_DEVICE(xive), 0, xive->tm_base);
> +}
> +
> +static void spapr_xive_reset(DeviceState *dev)
> +{
> +    sPAPRXive *xive = SPAPR_XIVE(dev);
> +    int i;
> +
> +    /* Xive Source reset is done through SysBus, it should put all
> +     * IRQs to OFF (!P|Q) */
> +
> +    /* Mask all valid EASs in the IRQ number space. */
> +    for (i = 0; i < xive->nr_irqs; i++) {
> +        XiveEAS *eas = &xive->eat[i];
> +        if (eas->w & EAS_VALID) {
> +            eas->w |= EAS_MASKED;

To ensure consistent behaviour across reboots, it would be better to
reset the whole of the EAS, except those which have to be preserved
across reboots (which would be VALID, and maybe nothing else?).

> +        }
> +    }
> +
> +    for (i = 0; i < xive->nr_ends; i++) {
> +        xive_end_reset(&xive->endt[i]);
> +    }
> +
> +    spapr_xive_mmio_map(xive);

You shouldn't need to re-etablish MMIO mappings at reset time, only
during initialization.

> +}
> +
> +static void spapr_xive_instance_init(Object *obj)
> +{
> +    sPAPRXive *xive = SPAPR_XIVE(obj);
> +
> +    object_initialize(&xive->source, sizeof(xive->source), TYPE_XIVE_SOURCE);

Yeah, embedding the source here makes sense, but it's a strong
indication that XiveSource should not be a SysBusDevice subclass.  I
really think it wants to be a TYPE_DEVICE subclass - and, in fact, I
think it can be object_initialize() embedded everywhere it's used.

I've also said elswhere that I suspect XiveRouter should also not be a
SysBusDevice.  With that approach it might make sense to embed it
here, rather than subclassing it (the old composition vs. inheritance
debate).

> +    object_property_add_child(obj, "source", OBJECT(&xive->source), NULL);
> +
> +    object_initialize(&xive->end_source, sizeof(xive->end_source),
> +                      TYPE_XIVE_END_SOURCE);
> +    object_property_add_child(obj, "end_source", OBJECT(&xive->end_source),
> +                              NULL);
> +}
> +
> +static void spapr_xive_realize(DeviceState *dev, Error **errp)
> +{
> +    sPAPRXive *xive = SPAPR_XIVE(dev);
> +    XiveSource *xsrc = &xive->source;
> +    XiveENDSource *end_xsrc = &xive->end_source;
> +    Error *local_err = NULL;
> +
> +    if (!xive->nr_irqs) {
> +        error_setg(errp, "Number of interrupt needs to be greater 0");
> +        return;
> +    }
> +
> +    if (!xive->nr_ends) {
> +        error_setg(errp, "Number of interrupt needs to be greater 0");
> +        return;
> +    }
> +
> +    /*
> +     * Initialize the internal sources, for IPIs and virtual devices.
> +     */
> +    object_property_set_int(OBJECT(xsrc), xive->nr_irqs, "nr-irqs",
> +                            &error_fatal);
> +    object_property_add_const_link(OBJECT(xsrc), "xive", OBJECT(xive),
> +                                   &error_fatal);
> +    object_property_set_bool(OBJECT(xsrc), true, "realized", &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    qdev_set_parent_bus(DEVICE(xsrc), sysbus_get_default());
> +
> +    /*
> +     * Initialize the END ESB source
> +     */
> +    object_property_set_int(OBJECT(end_xsrc), xive->nr_irqs, "nr-ends",
> +                            &error_fatal);
> +    object_property_add_const_link(OBJECT(end_xsrc), "xive", OBJECT(xive),
> +                                   &error_fatal);
> +    object_property_set_bool(OBJECT(end_xsrc), true, "realized", &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    qdev_set_parent_bus(DEVICE(end_xsrc), sysbus_get_default());
> +
> +    /* Set the mapping address of the END ESB pages after the source ESBs */
> +    xive->end_base = xive->vc_base + (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
> +
> +    /*
> +     * Allocate the routing tables
> +     */
> +    xive->eat = g_new0(XiveEAS, xive->nr_irqs);
> +    xive->endt = g_new0(XiveEND, xive->nr_ends);
> +
> +    /* TIMA initialization */
> +    memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &xive_tm_ops, xive,
> +                          "xive.tima", 4ull << TM_SHIFT);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &xive->tm_mmio);
> +}
> +
> +static int spapr_xive_get_eas(XiveRouter *xrtr, uint32_t lisn, XiveEAS *eas)
> +{
> +    sPAPRXive *xive = SPAPR_XIVE(xrtr);
> +
> +    if (lisn >= xive->nr_irqs) {
> +        return -1;
> +    }
> +
> +    *eas = xive->eat[lisn];
> +    return 0;
> +}
> +
> +static int spapr_xive_set_eas(XiveRouter *xrtr, uint32_t lisn, XiveEAS *eas)
> +{
> +    sPAPRXive *xive = SPAPR_XIVE(xrtr);
> +
> +    if (lisn >= xive->nr_irqs) {
> +        return -1;
> +    }
> +
> +    xive->eat[lisn] = *eas;
> +    return 0;
> +}
> +
> +static int spapr_xive_get_end(XiveRouter *xrtr,
> +                              uint8_t end_blk, uint32_t end_idx, XiveEND *end)
> +{
> +    sPAPRXive *xive = SPAPR_XIVE(xrtr);
> +
> +    if (end_idx >= xive->nr_ends) {
> +        return -1;
> +    }
> +
> +    memcpy(end, &xive->endt[end_idx], sizeof(XiveEND));
> +    return 0;
> +}
> +
> +static int spapr_xive_set_end(XiveRouter *xrtr,
> +                              uint8_t end_blk, uint32_t end_idx, XiveEND *end)
> +{
> +    sPAPRXive *xive = SPAPR_XIVE(xrtr);
> +
> +    if (end_idx >= xive->nr_ends) {
> +        return -1;
> +    }
> +
> +    memcpy(&xive->endt[end_idx], end, sizeof(XiveEND));
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_spapr_xive_end = {
> +    .name = TYPE_SPAPR_XIVE "/end",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField []) {
> +        VMSTATE_UINT32(w0, XiveEND),
> +        VMSTATE_UINT32(w1, XiveEND),
> +        VMSTATE_UINT32(w2, XiveEND),
> +        VMSTATE_UINT32(w3, XiveEND),
> +        VMSTATE_UINT32(w4, XiveEND),
> +        VMSTATE_UINT32(w5, XiveEND),
> +        VMSTATE_UINT32(w6, XiveEND),
> +        VMSTATE_UINT32(w7, XiveEND),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static const VMStateDescription vmstate_spapr_xive_eas = {
> +    .name = TYPE_SPAPR_XIVE "/eas",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField []) {
> +        VMSTATE_UINT64(w, XiveEAS),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static const VMStateDescription vmstate_spapr_xive = {
> +    .name = TYPE_SPAPR_XIVE,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_EQUAL(nr_irqs, sPAPRXive, NULL),
> +        VMSTATE_STRUCT_VARRAY_POINTER_UINT32(eat, sPAPRXive, nr_irqs,
> +                                     vmstate_spapr_xive_eas, XiveEAS),
> +        VMSTATE_STRUCT_VARRAY_POINTER_UINT32(endt, sPAPRXive, nr_ends,
> +                                             vmstate_spapr_xive_end, XiveEND),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static Property spapr_xive_properties[] = {
> +    DEFINE_PROP_UINT32("nr-irqs", sPAPRXive, nr_irqs, 0),
> +    DEFINE_PROP_UINT32("nr-ends", sPAPRXive, nr_ends, 0),
> +    DEFINE_PROP_UINT64("vc-base", sPAPRXive, vc_base, SPAPR_XIVE_VC_BASE),
> +    DEFINE_PROP_UINT64("tm-base", sPAPRXive, tm_base, SPAPR_XIVE_TM_BASE),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void spapr_xive_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    XiveRouterClass *xrc = XIVE_ROUTER_CLASS(klass);
> +
> +    dc->desc    = "sPAPR XIVE Interrupt Controller";
> +    dc->props   = spapr_xive_properties;
> +    dc->realize = spapr_xive_realize;
> +    dc->reset   = spapr_xive_reset;
> +    dc->vmsd    = &vmstate_spapr_xive;
> +
> +    xrc->get_eas = spapr_xive_get_eas;
> +    xrc->set_eas = spapr_xive_set_eas;
> +    xrc->get_end = spapr_xive_get_end;
> +    xrc->set_end = spapr_xive_set_end;
> +}
> +
> +static const TypeInfo spapr_xive_info = {
> +    .name = TYPE_SPAPR_XIVE,
> +    .parent = TYPE_XIVE_ROUTER,
> +    .instance_init = spapr_xive_instance_init,
> +    .instance_size = sizeof(sPAPRXive),
> +    .class_init = spapr_xive_class_init,
> +};
> +
> +static void spapr_xive_register_types(void)
> +{
> +    type_register_static(&spapr_xive_info);
> +}
> +
> +type_init(spapr_xive_register_types)
> +
> +bool spapr_xive_irq_enable(sPAPRXive *xive, uint32_t lisn, bool lsi)
> +{
> +    XiveSource *xsrc = &xive->source;
> +
> +    if (lisn >= xive->nr_irqs) {
> +        return false;
> +    }
> +
> +    xive->eat[lisn].w |= EAS_VALID;
> +    xive_source_irq_set(xsrc, lisn, lsi);
> +    return true;
> +}
> +
> +bool spapr_xive_irq_disable(sPAPRXive *xive, uint32_t lisn)
> +{
> +    XiveSource *xsrc = &xive->source;
> +
> +    if (lisn >= xive->nr_irqs) {
> +        return false;
> +    }
> +
> +    xive->eat[lisn].w &= ~EAS_VALID;
> +    xive_source_irq_set(xsrc, lisn, false);
> +    return true;
> +}
> +
> +qemu_irq spapr_xive_qirq(sPAPRXive *xive, uint32_t lisn)
> +{
> +    XiveSource *xsrc = &xive->source;
> +
> +    if (lisn >= xive->nr_irqs) {
> +        return NULL;
> +    }
> +
> +    if (!(xive->eat[lisn].w & EAS_VALID)) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid LISN %x\n", lisn);

I don't think this is a guest error - gettint the qirq by number
should generally be something qemu code does.

> +        return NULL;
> +    }
> +
> +    return xive_source_qirq(xsrc, lisn);
> +}
> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> index 72a46ed91c31..301a8e972d91 100644
> --- a/hw/intc/Makefile.objs
> +++ b/hw/intc/Makefile.objs
> @@ -38,6 +38,7 @@ obj-$(CONFIG_XICS) += xics.o
>  obj-$(CONFIG_XICS_SPAPR) += xics_spapr.o
>  obj-$(CONFIG_XICS_KVM) += xics_kvm.o
>  obj-$(CONFIG_XIVE) += xive.o
> +obj-$(CONFIG_XIVE_SPAPR) += spapr_xive.o
>  obj-$(CONFIG_POWERNV) += xics_pnv.o
>  obj-$(CONFIG_ALLWINNER_A10_PIC) += allwinner-a10-pic.o
>  obj-$(CONFIG_S390_FLIC) += s390_flic.o

-- 
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 --]