From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35399) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bzcFG-0008IT-QD for qemu-devel@nongnu.org; Thu, 27 Oct 2016 00:24:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bzcFE-0007Dn-MR for qemu-devel@nongnu.org; Thu, 27 Oct 2016 00:24:50 -0400 Date: Thu, 27 Oct 2016 14:09:52 +1100 From: David Gibson Message-ID: <20161027030952.GG19918@umbus.fritz.box> References: <1477129610-31353-1-git-send-email-clg@kaod.org> <1477129610-31353-12-git-send-email-clg@kaod.org> <20161025050850.GX11052@umbus.fritz.box> <3dea52d5-db52-f664-eb4d-8ea34d77711c@kaod.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="4ndw/alBWmZEhfcZ" Content-Disposition: inline In-Reply-To: <3dea52d5-db52-f664-eb4d-8ea34d77711c@kaod.org> Subject: Re: [Qemu-devel] [PATCH v5 11/17] ppc/xics: Add "native" XICS subclass 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, Benjamin Herrenschmidt , qemu-devel@nongnu.org, Alexander Graf --4ndw/alBWmZEhfcZ Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Oct 26, 2016 at 09:13:18AM +0200, C=E9dric Le Goater wrote: > On 10/25/2016 07:08 AM, David Gibson wrote: > > On Sat, Oct 22, 2016 at 11:46:44AM +0200, C=E9dric Le Goater wrote: > >> This provides access to the MMIO based Interrupt Presentation > >> Controllers (ICP) as found on a POWER8 system. > >> > >> A new XICSNative class is introduced to hold the MMIO region of the > >> ICPs. Each thread of the system has a subregion, indexed by its PIR > >> number, holding a XIVE (External Interrupt Vector Entry). This > >> provides a mean to make the link with the ICPState of the CPU. > >> > >> Signed-off-by: C=E9dric Le Goater > >> --- > >> > >> Changes since v4: > >> > >> - replaced the pir_table by memory subregions using an ICP.=20 > >> - removed the find_icp() and cpu_setup() handlers which became > >> useless with the memory regions. > >> - removed the superfluous inits done in xics_native_initfn. This is > >> covered in the parent class init. > >> - took ownership of the patch. > >> > >> default-configs/ppc64-softmmu.mak | 3 +- > >> hw/intc/Makefile.objs | 1 + > >> hw/intc/xics_native.c | 304 +++++++++++++++++++++++++++++= +++++++++ > >> include/hw/ppc/pnv.h | 19 +++ > >> include/hw/ppc/xics.h | 24 +++ > >> 5 files changed, 350 insertions(+), 1 deletion(-) > >> create mode 100644 hw/intc/xics_native.c > >> > >> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64= -softmmu.mak > >> index 67a9bcaa67fa..a22c93a48686 100644 > >> --- a/default-configs/ppc64-softmmu.mak > >> +++ b/default-configs/ppc64-softmmu.mak > >> @@ -48,8 +48,9 @@ CONFIG_PLATFORM_BUS=3Dy > >> CONFIG_ETSEC=3Dy > >> CONFIG_LIBDECNUMBER=3Dy > >> # For pSeries > >> -CONFIG_XICS=3D$(CONFIG_PSERIES) > >> +CONFIG_XICS=3D$(or $(CONFIG_PSERIES),$(CONFIG_POWERNV)) > >> CONFIG_XICS_SPAPR=3D$(CONFIG_PSERIES) > >> +CONFIG_XICS_NATIVE=3D$(CONFIG_POWERNV) > >> CONFIG_XICS_KVM=3D$(and $(CONFIG_PSERIES),$(CONFIG_KVM)) > >> # For PReP > >> CONFIG_MC146818RTC=3Dy > >> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs > >> index 2f44a2da26e9..e44a29d75b32 100644 > >> --- a/hw/intc/Makefile.objs > >> +++ b/hw/intc/Makefile.objs > >> @@ -34,6 +34,7 @@ obj-$(CONFIG_RASPI) +=3D bcm2835_ic.o bcm2836_contro= l.o > >> obj-$(CONFIG_SH4) +=3D sh_intc.o > >> obj-$(CONFIG_XICS) +=3D xics.o > >> obj-$(CONFIG_XICS_SPAPR) +=3D xics_spapr.o > >> +obj-$(CONFIG_XICS_NATIVE) +=3D xics_native.o > >> obj-$(CONFIG_XICS_KVM) +=3D xics_kvm.o > >> obj-$(CONFIG_ALLWINNER_A10_PIC) +=3D allwinner-a10-pic.o > >> obj-$(CONFIG_S390_FLIC) +=3D s390_flic.o > >> diff --git a/hw/intc/xics_native.c b/hw/intc/xics_native.c > >> new file mode 100644 > >> index 000000000000..bbdd786aeb50 > >> --- /dev/null > >> +++ b/hw/intc/xics_native.c > >> @@ -0,0 +1,304 @@ > >> +/* > >> + * QEMU PowerPC PowerNV machine model > >> + * > >> + * Native version of ICS/ICP > >> + * > >> + * Copyright (c) 2016, IBM Corporation. > >> + * > >> + * This library is free software; you can redistribute it and/or > >> + * modify it under the terms of the GNU Lesser General Public > >> + * License as published by the Free Software Foundation; either > >> + * version 2 of the License, or (at your option) any later version. > >> + * > >> + * This library 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 > >> + * Lesser General Public License for more details. > >> + * > >> + * You should have received a copy of the GNU Lesser General Public > >> + * License along with this library; if not, see . > >> + */ > >> + > >> +#include "qemu/osdep.h" > >> +#include "qapi/error.h" > >> +#include "qemu-common.h" > >> +#include "cpu.h" > >> +#include "hw/hw.h" > >> +#include "qemu/log.h" > >> +#include "qapi/error.h" > >> + > >> +#include "hw/ppc/fdt.h" > >> +#include "hw/ppc/xics.h" > >> +#include "hw/ppc/pnv.h" > >> + > >> +#include > >> + > >> +static void xics_native_reset(void *opaque) > >> +{ > >> + device_reset(DEVICE(opaque)); > >> +} > >> + > >> +static void xics_native_initfn(Object *obj) > >> +{ > >> + qemu_register_reset(xics_native_reset, obj); > >> +} > >=20 > > I think we need to investigate why the xics native is not showing up > > on the SysBus. As a "raw" MMIO device, it really should.=20 >=20 > Well, it has sysbus mmio region, but it is not created with qdev_create(.= =2E.)=20 > so it is not under sysbus and the reset does not get called. That is my > understanding of the problem. >=20 > May be we shouldn't be using a sysbus mmio region ? =20 Yeah, maybe not. We don't really fit the sysbus model well. I do kind of wonder if the xics object should be an mmio device at all, or if just the individual ICPs should be. But that might make for more trouble. > > If it was, device_reset should be called without these shenannigans. >=20 > yes. >=20 >=20 > >> + > >> +static uint64_t xics_native_read(void *opaque, hwaddr addr, unsigned = width) > >> +{ > >> + ICPState *icp =3D opaque; > >> + bool byte0 =3D (width =3D=3D 1 && (addr & 0x3) =3D=3D 0); > >> + uint64_t val =3D 0xffffffff; > >> + > >> + switch (addr & 0xffc) { > >> + case 0: /* poll */ > >> + val =3D icp_ipoll(icp, NULL); > >> + if (byte0) { > >> + val >>=3D 24; > >> + } else if (width !=3D 4) { > >> + goto bad_access; > >> + } > >> + break; > >> + case 4: /* xirr */ > >> + if (byte0) { > >> + val =3D icp_ipoll(icp, NULL) >> 24; > >> + } else if (width =3D=3D 4) { > >> + val =3D icp_accept(icp); > >> + } else { > >> + goto bad_access; > >> + } > >> + break; > >> + case 12: > >> + if (byte0) { > >> + val =3D icp->mfrr; > >> + } else { > >> + goto bad_access; > >> + } > >> + break; > >> + case 16: > >> + if (width =3D=3D 4) { > >> + val =3D icp->links[0]; > >> + } else { > >> + goto bad_access; > >> + } > >> + break; > >> + case 20: > >> + if (width =3D=3D 4) { > >> + val =3D icp->links[1]; > >> + } else { > >> + goto bad_access; > >> + } > >> + break; > >> + case 24: > >> + if (width =3D=3D 4) { > >> + val =3D icp->links[2]; > >> + } else { > >> + goto bad_access; > >> + } > >> + break; > >> + default: > >> +bad_access: > >> + qemu_log_mask(LOG_GUEST_ERROR, "XICS: Bad ICP access 0x%" > >> + HWADDR_PRIx"/%d\n", addr, width); > >> + } > >> + > >> + return val; > >> +} > >> + > >> +static void xics_native_write(void *opaque, hwaddr addr, uint64_t val, > >> + unsigned width) > >> +{ > >> + ICPState *icp =3D opaque; > >> + bool byte0 =3D (width =3D=3D 1 && (addr & 0x3) =3D=3D 0); > >> + > >> + switch (addr & 0xffc) { > >> + case 4: /* xirr */ > >> + if (byte0) { > >> + icp_set_cppr(icp, val); > >> + } else if (width =3D=3D 4) { > >> + icp_eoi(icp, val); > >> + } else { > >> + goto bad_access; > >> + } > >> + break; > >> + case 12: > >> + if (byte0) { > >> + icp_set_mfrr(icp, val); > >> + } else { > >> + goto bad_access; > >> + } > >> + break; > >> + case 16: > >> + if (width =3D=3D 4) { > >> + icp->links[0] =3D val; > >> + } else { > >> + goto bad_access; > >> + } > >> + break; > >> + case 20: > >> + if (width =3D=3D 4) { > >> + icp->links[1] =3D val; > >> + } else { > >> + goto bad_access; > >> + } > >> + break; > >> + case 24: > >> + if (width =3D=3D 4) { > >> + icp->links[2] =3D val; > >> + } else { > >> + goto bad_access; > >> + } > >> + break; > >> + default: > >> +bad_access: > >> + qemu_log_mask(LOG_GUEST_ERROR, "XICS: Bad ICP access 0x%" > >> + HWADDR_PRIx"/%d\n", addr, width); > >> + } > >> +} > >> + > >> +static const MemoryRegionOps xics_native_ops =3D { > >> + .read =3D xics_native_read, > >> + .write =3D xics_native_write, > >> + .valid.min_access_size =3D 1, > >> + .valid.max_access_size =3D 4, > >> + .impl.min_access_size =3D 1, > >> + .impl.max_access_size =3D 4, > >> + .endianness =3D DEVICE_BIG_ENDIAN, > >> +}; > >> + > >> +static uint64_t xics_native_default_read(void *opaque, hwaddr addr, > >> + unsigned width) > >> +{ > >> + qemu_log_mask(LOG_GUEST_ERROR, "XICS: Bad ICP access 0x%" > >> + HWADDR_PRIx"/%d\n", addr, width); > >> + return 0xffffffffffffffffull; > >> +} > >> + > >> +static void xics_native_default_write(void *opaque, hwaddr addr, uint= 64_t val, > >> + unsigned width) > >> +{ > >> + qemu_log_mask(LOG_GUEST_ERROR, "XICS: Bad ICP access 0x%" > >> + HWADDR_PRIx"/%d\n", addr, width); > >> +} > >> + > >> +static const MemoryRegionOps xics_native_default_ops =3D { > >> + .read =3D xics_native_default_read, > >> + .write =3D xics_native_default_write, > >> + .valid.min_access_size =3D 1, > >> + .valid.max_access_size =3D 4, > >> + .impl.min_access_size =3D 1, > >> + .impl.max_access_size =3D 4, > >> + .endianness =3D DEVICE_BIG_ENDIAN, > >> +}; > >> + > >> +static void xics_native_set_nr_servers(XICSState *xics, uint32_t nr_s= ervers, > >> + Error **errp) > >> +{ > >> + xics_set_nr_servers(xics, nr_servers, TYPE_ICP, errp); > >> +} > >> + > >> +static void xics_native_realize(DeviceState *dev, Error **errp) > >> +{ > >> + XICSState *xics =3D XICS_COMMON(dev); > >> + XICSNative *xicsn =3D XICS_NATIVE(dev); > >> + Error *error =3D NULL; > >> + int i; > >> + > >> + if (!xics->nr_servers) { > >> + error_setg(errp, "Number of servers needs to be greater than = 0"); > >> + return; > >> + } > >> + > >> + for (i =3D 0; i < xics->nr_servers; i++) { > >> + object_property_set_bool(OBJECT(&xics->ss[i]), true, "realize= d", > >> + &error); > >> + if (error) { > >> + error_propagate(errp, error); > >> + return; > >> + } > >> + } > >> + > >> + /* > >> + * Initialize the container region for the ICPs and the subregion > >> + * for each cpu. The mmapping will be done at the board level > >> + * depending on the pir of the core. > >> + * > >> + * TODO: build a name with the chip id > >> + */ > >> + memory_region_init_io(&xicsn->icp_mmio, OBJECT(dev), > >> + &xics_native_default_ops, xicsn, "icp-0", > >> + PNV_XICS_SIZE); > >=20 > > I don't think you should need these native ops. I believe you can > > construct a memory region as a "pure" container, then just put the > > populated regions inside it. >=20 > It is a way to track bogus read/writes the guest shouldn't be doing > but it is not that important. I can remove it. Right. I don't recall exactly what will happen if you don't populate this at all, but I think you should eventually arrive at the global fallback handler which should give you a similar effect. --=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 --4ndw/alBWmZEhfcZ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYEW/9AAoJEGw4ysog2bOSZr4P/39qRIxqT0HUiLSy0/WBl//m i9mGSPHtJywq36aoIM/QWS/+mcRmdmQO8H0Eee0O/HvarYJF2Hh0VEqsyrwq1sEh wdzHxT+L7nitDPF9wFlvuCk7BuG0S2nhEg/nPdW4DCCCnrDD2E+M7JdOkIeZo4gY qMSvivzCi+31fDOxtRTGwoToxg7GqVQ1+JVW4ZbKnfGvfxlhe3kOHw/58FLc7+Pa tYMNYxIK5Zno194iImXcpZNUBnnCJef2IlzyuAlxD/si2mSxEj5RNQgNxGFvdskI djOkCK+E4XvH//eOEmcIxJpuhQSX5sKlrNK/ik058/jpPsRKlsorw2MLd2Q/TB/R OktAxJggKTWFzizxIPWI50WIDGYegS6UCZElA2hheQ7sKLEKy+D1E+A6V1pm++8N 0UVvt4sTA/PK+YEdPZX9PxxOOLNlBUG6YAHHWEcY5W1R4EpxHB139mTEognvZRj4 wHYC9B2lAPs+JcHAfLNMFYlnLZMnFHATdDNJEEt+F1Dx/ziX2USuBOL64I/RcTyA KCG6nF9cinzpqB+Aa5BnB22eUF2ZFDxmzkDOL4zKw91gkMhVAGVT5C12fzcu5k5v mk0L4CIE8rF9+hAYAPDdva8zpKeD6JX9d0nKwX7Pv1kn9U+ac059ewvJvT0wMCkC dChtW+A5DN09V5paRarh =7J57 -----END PGP SIGNATURE----- --4ndw/alBWmZEhfcZ--