Re: [Qemu-devel] [RFC PATCH 04/26] ppc/xive: introduce a skeleton for the XIVE interrupt controller model

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

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

On Wed, Jul 19, 2017 at 01:08:49PM +1000, David Gibson wrote:
> On Wed, Jul 05, 2017 at 07:13:17PM +0200, Cédric Le Goater wrote:
> > Let's provide an empty shell for the XIVE controller model with a
> > couple of attributes for the IRQ number allocator. The latter is
> > largely inspired by OPAL which allocates IPI IRQ numbers from the
> > bottom of the IRQ number space and allocates the HW IRQ numbers from
> > the top.
> > 
> > The number of IPIs is simply deduced from the max number of CPUs the
> > guest supports and we provision a arbitrary number of HW irqs.
> > 
> > The XIVE object is kept private because it will hold internal tables
> > which do not need to be exposed to sPAPR.
> > 
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> >  default-configs/ppc64-softmmu.mak |  1 +
> >  hw/intc/Makefile.objs             |  1 +
> >  hw/intc/xive-internal.h           | 28 ++++++++++++
> >  hw/intc/xive.c                    | 94 +++++++++++++++++++++++++++++++++++++++
> >  include/hw/ppc/xive.h             | 27 +++++++++++
> >  5 files changed, 151 insertions(+)
> >  create mode 100644 hw/intc/xive-internal.h
> >  create mode 100644 hw/intc/xive.c
> >  create mode 100644 include/hw/ppc/xive.h
> > 
> > diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
> > index 46c95993217d..1179c07e6e9f 100644
> > --- a/default-configs/ppc64-softmmu.mak
> > +++ b/default-configs/ppc64-softmmu.mak
> > @@ -56,6 +56,7 @@ CONFIG_SM501=y
> >  CONFIG_XICS=$(CONFIG_PSERIES)
> >  CONFIG_XICS_SPAPR=$(CONFIG_PSERIES)
> >  CONFIG_XICS_KVM=$(and $(CONFIG_PSERIES),$(CONFIG_KVM))
> > +CONFIG_XIVE=$(CONFIG_PSERIES)
> >  # For PReP
> >  CONFIG_SERIAL_ISA=y
> >  CONFIG_MC146818RTC=y
> > diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> > index 78426a7dafcd..28b83456bfcc 100644
> > --- a/hw/intc/Makefile.objs
> > +++ b/hw/intc/Makefile.objs
> > @@ -35,6 +35,7 @@ obj-$(CONFIG_SH4) += sh_intc.o
> >  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_POWERNV) += xics_pnv.o
> >  obj-$(CONFIG_ALLWINNER_A10_PIC) += allwinner-a10-pic.o
> >  obj-$(CONFIG_S390_FLIC) += s390_flic.o
> > diff --git a/hw/intc/xive-internal.h b/hw/intc/xive-internal.h
> > new file mode 100644
> > index 000000000000..155c2dcd6066
> > --- /dev/null
> > +++ b/hw/intc/xive-internal.h
> > @@ -0,0 +1,28 @@
> > +/*
> > + * Copyright 2016,2017 IBM Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
> > +#ifndef _INTC_XIVE_INTERNAL_H
> > +#define _INTC_XIVE_INTERNAL_H
> > +
> > +#include <hw/sysbus.h>
> > +
> > +struct XIVE {
> > +    SysBusDevice parent;
> 
> XIVE probably shouldn't be a SysBusDevice.  According to agraf, that
> should only be used for things which have an MMIO presence on a bus
> structure that's not worth the bother of more specifically modelling.
> 
> I don't think that's the case for XIVE, so it should just have
> TYPE_DEVICE as its parent.  There are several pseries things which
> already get this wrong (mostly because I made them before fully
> understanding the role of the SysBus), but we should avoid adding
> others.
> 
> > +    /* Properties */
> > +    uint32_t     nr_targets;
> > +
> > +    /* IRQ number allocator */
> > +    uint32_t     int_count;     /* Number of interrupts: nr_targets + HW IRQs */
> > +    uint32_t     int_base;      /* Min index */
> > +    uint32_t     int_max;       /* Max index */
> > +    uint32_t     int_hw_bot;    /* Bottom index of HW IRQ allocator */
> > +    uint32_t     int_ipi_top;   /* Highest IPI index handed out so far + 1 */
> > +};
> > +
> > +#endif /* _INTC_XIVE_INTERNAL_H */
> > diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> > new file mode 100644
> > index 000000000000..5b4ea915d87c
> > --- /dev/null
> > +++ b/hw/intc/xive.c
> > @@ -0,0 +1,94 @@
> > +/*
> > + * QEMU PowerPC XIVE model
> > + *
> > + * Copyright (c) 2017, IBM Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License, version 2, as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +#include "qemu/osdep.h"
> > +#include "qemu/log.h"
> > +#include "qapi/error.h"
> > +#include "target/ppc/cpu.h"
> > +#include "sysemu/cpus.h"
> > +#include "sysemu/dma.h"
> > +#include "monitor/monitor.h"
> > +#include "hw/ppc/xive.h"
> > +
> > +#include "xive-internal.h"
> > +
> > +/*
> > + * Main XIVE object
> 
> As with XICs, does it really make sense for there to be a "main" XIVE
> object, or should be an interface attached to the machine?
> 
> > + */
> > +
> > +/* Let's provision some HW IRQ numbers. We could use a XIVE property
> > + * also but it does not seem necessary for the moment.
> > + */
> > +#define MAX_HW_IRQS_ENTRIES (8 * 1024)
> > +
> > +static void xive_init(Object *obj)
> > +{
> > +    ;
> > +}
> > +
> > +static void xive_realize(DeviceState *dev, Error **errp)
> > +{
> > +    XIVE *x = XIVE(dev);
> > +
> > +    if (!x->nr_targets) {
> > +        error_setg(errp, "Number of interrupt targets needs to be greater 0");
> > +        return;
> > +    }
> > +
> > +    /* Initialize IRQ number allocator. Let's use a base number if we
> > +     * need to introduce a notion of blocks one day.
> > +     */
> > +    x->int_base = 0;
> > +    x->int_count = x->nr_targets + MAX_HW_IRQS_ENTRIES;
> > +    x->int_max = x->int_base + x->int_count;

Also storing a value which is easily derivable from values already in
the structure is a bit silly.  I think you should drop either
int_count or int_max.

> > +    x->int_hw_bot = x->int_max;
> > +    x->int_ipi_top = x->int_base;
> > +
> > +    /* Reserve some numbers as OPAL does ? */
> > +    if (x->int_ipi_top < 0x10) {
> > +        x->int_ipi_top = 0x10;
> > +    }
> 
> I'm somewhat uncomfortable with an irq allocater here in the intc
> code.  As a rule, irq allocation is the responsibility of the machine,
> not any sub-component.  Furthermore, it should allocate in a way which
> is repeatable, since they need to stay stable across reboots and
> migrations.
> 
> And, yes, we have an allocator of sorts in XICS - it has caused a
> number of problems in the past.
> 
> > +}
> > +
> > +static Property xive_properties[] = {
> > +    DEFINE_PROP_UINT32("nr-targets", XIVE, nr_targets, 0),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void xive_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > +    dc->realize = xive_realize;
> > +    dc->props = xive_properties;
> > +    dc->desc = "XIVE";
> > +}
> > +
> > +static const TypeInfo xive_info = {
> > +    .name = TYPE_XIVE,
> > +    .parent = TYPE_SYS_BUS_DEVICE,
> > +    .instance_init = xive_init,
> > +    .instance_size = sizeof(XIVE),
> > +    .class_init = xive_class_init,
> > +};
> > +
> > +static void xive_register_types(void)
> > +{
> > +    type_register_static(&xive_info);
> > +}
> > +
> > +type_init(xive_register_types)
> > diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> > new file mode 100644
> > index 000000000000..863f5a9c6b5f
> > --- /dev/null
> > +++ b/include/hw/ppc/xive.h
> > @@ -0,0 +1,27 @@
> > +/*
> > + * QEMU PowerPC XIVE model
> > + *
> > + * Copyright (c) 2017, IBM Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License, version 2, as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#ifndef PPC_XIVE_H
> > +#define PPC_XIVE_H
> > +
> > +typedef struct XIVE XIVE;
> > +
> > +#define TYPE_XIVE "xive"
> > +#define XIVE(obj) OBJECT_CHECK(XIVE, (obj), TYPE_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 --]