From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46184) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gRqWd-0002q3-OO for qemu-devel@nongnu.org; Tue, 27 Nov 2018 22:28:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gRqWa-00036r-LP for qemu-devel@nongnu.org; Tue, 27 Nov 2018 22:28:31 -0500 Date: Wed, 28 Nov 2018 14:28:19 +1100 From: David Gibson Message-ID: <20181128032819.GY2251@umbus.fritz.box> References: <20181116105729.23240-1-clg@kaod.org> <20181116105729.23240-16-clg@kaod.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="yhqQ34TVR4fE8mPU" Content-Disposition: inline In-Reply-To: <20181116105729.23240-16-clg@kaod.org> Subject: Re: [Qemu-devel] [PATCH v5 15/36] spapr: introdude a new machine IRQ backend for XIVE List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?C=E9dric?= Le Goater Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, Benjamin Herrenschmidt --yhqQ34TVR4fE8mPU Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Nov 16, 2018 at 11:57:08AM +0100, C=E9dric Le Goater wrote: > The XIVE IRQ backend uses the same layout as the new XICS backend but > covers the full range of the IRQ number space. The IRQ numbers for the > CPU IPIs are allocated at the bottom of this space, below 4K, to > preserve compatibility with XICS which does not use that range. >=20 > This should be enough given that the maximum number of CPUs is 1024 > for the sPAPR machine under QEMU. For the record, the biggest POWER8 > or POWER9 system has a maximum of 1536 HW threads (16 sockets, 192 > cores, SMT8). >=20 > Signed-off-by: C=E9dric Le Goater > --- > include/hw/ppc/spapr.h | 2 + > include/hw/ppc/spapr_irq.h | 7 ++- > hw/ppc/spapr.c | 2 +- > hw/ppc/spapr_irq.c | 119 ++++++++++++++++++++++++++++++++++++- > 4 files changed, 124 insertions(+), 6 deletions(-) >=20 > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 6279711fe8f7..1fbc2663e06c 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -16,6 +16,7 @@ typedef struct sPAPREventLogEntry sPAPREventLogEntry; > typedef struct sPAPREventSource sPAPREventSource; > typedef struct sPAPRPendingHPT sPAPRPendingHPT; > typedef struct ICSState ICSState; > +typedef struct sPAPRXive sPAPRXive; > =20 > #define HPTE64_V_HPTE_DIRTY 0x0000000000000040ULL > #define SPAPR_ENTRY_POINT 0x100 > @@ -175,6 +176,7 @@ struct sPAPRMachineState { > const char *icp_type; > int32_t irq_map_nr; > unsigned long *irq_map; > + sPAPRXive *xive; > =20 > bool cmd_line_caps[SPAPR_CAP_NUM]; > sPAPRCapabilities def, eff, mig; > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h > index 0e9229bf219e..c854ae527808 100644 > --- a/include/hw/ppc/spapr_irq.h > +++ b/include/hw/ppc/spapr_irq.h > @@ -13,6 +13,7 @@ > /* > * IRQ range offsets per device type > */ > +#define SPAPR_IRQ_IPI 0x0 > #define SPAPR_IRQ_EPOW 0x1000 /* XICS_IRQ_BASE offset */ > #define SPAPR_IRQ_HOTPLUG 0x1001 > #define SPAPR_IRQ_VIO 0x1100 /* 256 VIO devices */ > @@ -33,7 +34,8 @@ typedef struct sPAPRIrq { > uint32_t nr_irqs; > uint32_t nr_msis; > =20 > - void (*init)(sPAPRMachineState *spapr, int nr_irqs, Error **errp); > + void (*init)(sPAPRMachineState *spapr, int nr_irqs, int nr_servers, > + Error **errp); > int (*claim)(sPAPRMachineState *spapr, int irq, bool lsi, Error **er= rp); > void (*free)(sPAPRMachineState *spapr, int irq, int num); > qemu_irq (*qirq)(sPAPRMachineState *spapr, int irq); > @@ -42,8 +44,9 @@ typedef struct sPAPRIrq { > =20 > extern sPAPRIrq spapr_irq_xics; > extern sPAPRIrq spapr_irq_xics_legacy; > +extern sPAPRIrq spapr_irq_xive; > =20 > -void spapr_irq_init(sPAPRMachineState *spapr, Error **errp); > +void spapr_irq_init(sPAPRMachineState *spapr, int nr_servers, Error **er= rp); I don't see why nr_servers needs to become a parameter, since it can be derived from spapr within this routine. > int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error *= *errp); > void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num); > qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq); > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index e470efe7993c..9f8c19e56e7a 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2594,7 +2594,7 @@ static void spapr_machine_init(MachineState *machin= e) > spapr_set_vsmt_mode(spapr, &error_fatal); > =20 > /* Set up Interrupt Controller before we create the VCPUs */ > - spapr_irq_init(spapr, &error_fatal); > + spapr_irq_init(spapr, xics_max_server_number(spapr), &error_fatal); We should rename xics_max_server_number() since it's no longer xics specific. > /* Set up containers for ibm,client-architecture-support negotiated = options > */ > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > index bac450ffff23..2569ae1bc7f8 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -12,6 +12,7 @@ > #include "qemu/error-report.h" > #include "qapi/error.h" > #include "hw/ppc/spapr.h" > +#include "hw/ppc/spapr_xive.h" > #include "hw/ppc/xics.h" > #include "sysemu/kvm.h" > =20 > @@ -91,7 +92,7 @@ error: > } > =20 > static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs, > - Error **errp) > + int nr_servers, Error **errp) > { > MachineState *machine =3D MACHINE(spapr); > Error *local_err =3D NULL; > @@ -204,10 +205,122 @@ sPAPRIrq spapr_irq_xics =3D { > .print_info =3D spapr_irq_print_info_xics, > }; > =20 > + /* > + * XIVE IRQ backend. > + */ > +static sPAPRXive *spapr_xive_create(sPAPRMachineState *spapr, > + const char *type_xive, int nr_irqs, > + int nr_servers, Error **errp) > +{ > + sPAPRXive *xive; > + Error *local_err =3D NULL; > + Object *obj; > + uint32_t nr_ends =3D nr_servers << 3; /* 8 priority ENDs per CPU */ > + int i; > + > + obj =3D object_new(type_xive); What's the reason for making the type a parameter, rather than just using the #define here. > + object_property_set_int(obj, nr_irqs, "nr-irqs", &error_abort); > + object_property_set_int(obj, nr_ends, "nr-ends", &error_abort); This is still within the sPAPR code, and you have a pointer to the MachineState, so I don't see why you could't just derive nr_irqs and nr_servers from that, rather than having them passed in. > + object_property_set_bool(obj, true, "realized", &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return NULL; > + } > + qdev_set_parent_bus(DEVICE(obj), sysbus_get_default()); Whereas the XiveSource and XiveRouter I think make more sense as "device components" rather than SysBusDevice subclasses, I think it *does* make sense for the PAPR-XIVE object to be a full fledged SysBusDevice. And for that reason, I think it makes more sense to create it with qdev_create(), which should avoid having to manually fiddle with the parent bus. > + xive =3D SPAPR_XIVE(obj); > + > + /* Enable the CPU IPIs */ > + for (i =3D 0; i < nr_servers; ++i) { > + spapr_xive_irq_enable(xive, SPAPR_IRQ_IPI + i, false); This comment possibly belonged on an earlier patch. I don't love the "..._enable" name - to me that suggests something runtime rather than configuration time. A better option isn't quickly occurring to me though :/. > + } > + > + return xive; > +} > + > +static void spapr_irq_init_xive(sPAPRMachineState *spapr, int nr_irqs, > + int nr_servers, Error **errp) > +{ > + MachineState *machine =3D MACHINE(spapr); > + Error *local_err =3D NULL; > + > + /* KVM XIVE support */ > + if (kvm_enabled()) { > + if (machine_kernel_irqchip_required(machine)) { > + error_setg(errp, "kernel_irqchip requested. no XIVE support"= ); > + return; > + } > + } > + > + /* QEMU XIVE support */ > + spapr->xive =3D spapr_xive_create(spapr, TYPE_SPAPR_XIVE, nr_irqs, n= r_servers, > + &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > +} > + > +static int spapr_irq_claim_xive(sPAPRMachineState *spapr, int irq, bool = lsi, > + Error **errp) > +{ > + if (!spapr_xive_irq_enable(spapr->xive, irq, lsi)) { > + error_setg(errp, "IRQ %d is invalid", irq); > + return -1; > + } > + return 0; > +} > + > +static void spapr_irq_free_xive(sPAPRMachineState *spapr, int irq, int n= um) > +{ > + int i; > + > + for (i =3D irq; i < irq + num; ++i) { > + spapr_xive_irq_disable(spapr->xive, i); > + } > +} > + > +static qemu_irq spapr_qirq_xive(sPAPRMachineState *spapr, int irq) > +{ > + return spapr_xive_qirq(spapr->xive, irq); > +} > + > +static void spapr_irq_print_info_xive(sPAPRMachineState *spapr, > + Monitor *mon) > +{ > + CPUState *cs; > + > + CPU_FOREACH(cs) { > + PowerPCCPU *cpu =3D POWERPC_CPU(cs); > + > + xive_tctx_pic_print_info(XIVE_TCTX(cpu->intc), mon); > + } > + > + spapr_xive_pic_print_info(spapr->xive, mon); Any reason the info dumping routines are split into two? > +} > + > +/* > + * XIVE uses the full IRQ number space. Set it to 8K to be compatible > + * with XICS. > + */ > + > +#define SPAPR_IRQ_XIVE_NR_IRQS 0x2000 > +#define SPAPR_IRQ_XIVE_NR_MSIS (SPAPR_IRQ_XIVE_NR_IRQS - SPAPR_IRQ_M= SI) > + > +sPAPRIrq spapr_irq_xive =3D { > + .nr_irqs =3D SPAPR_IRQ_XIVE_NR_IRQS, > + .nr_msis =3D SPAPR_IRQ_XIVE_NR_MSIS, > + > + .init =3D spapr_irq_init_xive, > + .claim =3D spapr_irq_claim_xive, > + .free =3D spapr_irq_free_xive, > + .qirq =3D spapr_qirq_xive, > + .print_info =3D spapr_irq_print_info_xive, > +}; > + > /* > * sPAPR IRQ frontend routines for devices > */ > -void spapr_irq_init(sPAPRMachineState *spapr, Error **errp) > +void spapr_irq_init(sPAPRMachineState *spapr, int nr_servers, Error **er= rp) > { > sPAPRMachineClass *smc =3D SPAPR_MACHINE_GET_CLASS(spapr); > =20 > @@ -216,7 +329,7 @@ void spapr_irq_init(sPAPRMachineState *spapr, Error *= *errp) > spapr_irq_msi_init(spapr, smc->irq->nr_msis); > } > =20 > - smc->irq->init(spapr, smc->irq->nr_irqs, errp); > + smc->irq->init(spapr, smc->irq->nr_irqs, nr_servers, errp); > } > =20 > int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error *= *errp) --=20 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 --yhqQ34TVR4fE8mPU Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEdfRlhq5hpmzETofcbDjKyiDZs5IFAlv+C1MACgkQbDjKyiDZ s5I9MA//VSA0a4n4eeeCAP55NQVsF5tN8zwpE4m2rKHr5DSlu02nUvdqbLebvtmZ rXM7Hv0cA8SY4cVUfWyFPj5oKbgwi2jU5gELnGKGvz9iA+pEv7V0GHi9uyX2qQgj XpY7TWAEAUhpuffmP6IDISl2VNw1ztu72M5d5i1+5VOeByfUpfBK3iMc4B0E9lnF 0BLYr0TR1nscFuc6fYdQIO9V604OpPeojx1tb4VTUiiVwvNw5CBtTimpd1s3kGxw 2uzFUzyiY8NOejbWYbGzsCzybxY3PArrzwtGyK+f4M05i+VmNrENl0dnFL0b+WrF HsDML6oRKH76UCPoMTqubup6itujInXGK/XKrsQecYDr9Evb5y2DwCQRMjB1zC7P Wuv3YGEhdpovuMbw+SwJ7qRPCd1X8Dgb4cwk/7fHuUPKIgiNvduGRY4RD59fDgiO Dnt7Pzu2Zpugf++tNwweRpoYU4gFdHrkNsWPHF9cKCFwKeYnaIdv9UaBDxv2qQgC 5CtDRloJegEy2Njv5hfN0GavaabzhBpTo0EmwdDi71NiSWSOXIjFmwnNBxFf7Tk4 oMeJ49nl79wjuOtlo+8dF/atFAkSiBnue5VAAINWrMq3Ez13VRh5WJT0gxHuKP0T KFCwxngoC8jYml5WXin5eJnQ9rWpz5ykH/+ABxgAemOYgmHjjn4= =9U5x -----END PGP SIGNATURE----- --yhqQ34TVR4fE8mPU--