From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51283) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1boihu-0005ZQ-Jj for qemu-devel@nongnu.org; Mon, 26 Sep 2016 23:05:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1boihr-0003DG-C7 for qemu-devel@nongnu.org; Mon, 26 Sep 2016 23:05:22 -0400 Date: Tue, 27 Sep 2016 12:35:48 +1000 From: David Gibson Message-ID: <20160927023548.GC15376@umbus.fritz.box> References: <1473943560-14846-1-git-send-email-clg@kaod.org> <1473943560-14846-8-git-send-email-clg@kaod.org> <20160921060845.GC20488@umbus> <20160923024654.GT2085@umbus.fritz.box> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="8X7/QrJGcKSMr1RN" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure 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 --8X7/QrJGcKSMr1RN Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Sep 26, 2016 at 06:11:36PM +0200, C=E9dric Le Goater wrote: > On 09/23/2016 04:46 AM, David Gibson wrote: > > On Thu, Sep 22, 2016 at 10:25:59AM +0200, C=E9dric Le Goater wrote: > >>>> @@ -493,6 +525,8 @@ static void pnv_chip_power9_class_init(ObjectCla= ss *klass, void *data) > >>>> k->chip_cfam_id =3D 0x100d104980000000ull; /* P9 Nimbus DD1.0 */ > >>>> k->cores_mask =3D POWER9_CORE_MASK; > >>>> k->core_pir =3D pnv_chip_core_pir_p9; > >>>> + k->xscom_addr =3D pnv_chip_xscom_addr_p9; > >>>> + k->xscom_pcba =3D pnv_chip_xscom_pcba_p9; > >>> > >>> So if you do as BenH (and I) suggested and have the "scom address > >>> space" actually be addressed by (pcba << 3), I think you can probably > >>> avoid these. =20 > >> > >> I will look at that option again.=20 > >> > >> I was trying to untangle a few things at the same time. I have better > >> view of the problem to solve now. The bus is gone, that's was one=20 > >> thing. How we map these xscom regions is the next.=20 > >> > >> Ben suggested to add some P7/P8 mangling before the dispatch in=20 > >> the &address_space_xscom. This should make things cleaner. I had=20 > >> not thought of doing that and this is why I introduced these helpers : > >> > >> +uint32_t pnv_xscom_pcba(PnvXScomInterface *dev, uint64_t addr) > >> +uint64_t pnv_xscom_addr(PnvXScomInterface *dev, uint32_t pcba) > >> > >> which I don't really like ... > >> > >> but we must make sure that we can do the mapping of the xscom=20 > >> subregions in the &address_space_xscom using (pcba << 3) > >> > >> > >>> Instead you can handle it in the chip or ADU realize function by eith= er: > >>> > >>> P8: * map one big subregion for the ADU into &address_space_memory > >>> * have the handler for that subregion do the address mangling, > >>> then redispatch into the xscom address space > >>> > >>> P9: * Map the appropriate chunk of the xscom address space > >>> directly into address_space_memory > >> > >> Yes that was my feeling for a better solution but Ben chimed in with t= he=20 > >> HMER topic. I need to look at that. > >=20 > > Right. Doesn't change the basic concept though - it just means you > > need (slightly different) redispatchers for both P8 and P9. >=20 > In fact they are the same, you only need an "addr to pcba" handler at the > chip class level :=20 Ok. I'd been thinking of using different dispatchers as an alternative to using the chip class translator hook, but I guess if you have the decoding of those "core" registers here as well, then that doesn't make so much sense. > static uint64_t xscom_read(void *opaque, hwaddr addr, unsigned width) > { > PnvChip *chip =3D opaque; > uint32_t pcba =3D PNV_CHIP_GET_CLASS(chip)->xscom_pcba(addr); > uint64_t val =3D 0; > MemTxResult result; >=20 > ... >=20 > val =3D address_space_ldq(&chip->xscom_as, pcba << 3, > MEMTXATTRS_UNSPECIFIED, &result); > if (result !=3D MEMTX_OK) { >=20 > =20 >=20 > And so, the result is pretty clean. I killed the proxy object and merged= =20 > the regions in the chip but I have kept the pnv_xscom.c file because the= =20 > code related to xscom is rather large : ~250 lines.=20 Sure, makes sense. > The objects declaring a xscom region need to do some register shifting bu= t=20 > this is usual in mmio regions. >=20 > You will see in v4. Ok. > >>>> +static bool xscom_dispatch_read(PnvXScom *xscom, hwaddr addr, uint6= 4_t *val) > >>>> +{ > >>>> + uint32_t success; > >>>> + uint8_t data[8]; > >>>> + > >>>> + success =3D !address_space_rw(&xscom->xscom_as, addr, MEMTXATTR= S_UNSPECIFIED, > >>>> + data, 8, false); > >>>> + *val =3D (((uint64_t) data[0]) << 56 | > >>>> + ((uint64_t) data[1]) << 48 | > >>>> + ((uint64_t) data[2]) << 40 | > >>>> + ((uint64_t) data[3]) << 32 | > >>>> + ((uint64_t) data[4]) << 24 | > >>>> + ((uint64_t) data[5]) << 16 | > >>>> + ((uint64_t) data[6]) << 8 | > >>>> + ((uint64_t) data[7])); > >>> > >>> AFAICT this is basically assuming data is always encoded BE. With the > >>> right choice of endian flags on the individual SCOM device > >>> registrations with the scom address space, I think you should be able > >>> to avoid this mangling. > >> > >> yes. I should but curiously I had to do this, and this works the same = on > >> an intel host or a ppc64 host. > >=20 > > Hmm.. I suspect what you actually need is NATIVE_ENDIAN on the > > individual SCOM devices, with BIG_ENDIAN on the redispatcher region. >=20 > we should be using address_space_ldq and address_space_stq. Ok. > >>>> + > >>>> + success =3D !address_space_rw(&xscom->xscom_as, addr, MEMTXATTR= S_UNSPECIFIED, > >>>> + data, 8, true); > >>>> + return success; > >>>> +} > >>>> + > >>>> +static uint64_t xscom_read(void *opaque, hwaddr addr, unsigned widt= h) > >>>> +{ > >>>> + PnvXScom *s =3D opaque; > >>>> + uint32_t pcba =3D s->chip_class->xscom_pcba(addr); > >>>> + uint64_t val =3D 0; > >>>> + > >>>> + /* Handle some SCOMs here before dispatch */ > >>>> + switch (pcba) { > >>>> + case 0xf000f: > >>>> + val =3D s->chip_class->chip_cfam_id; > >>>> + break; > >>>> + case 0x1010c00: /* PIBAM FIR */ > >>>> + case 0x1010c03: /* PIBAM FIR MASK */ > >>>> + case 0x2020007: /* ADU stuff */ > >>>> + case 0x2020009: /* ADU stuff */ > >>>> + case 0x202000f: /* ADU stuff */ > >>>> + val =3D 0; > >>>> + break; > >>>> + case 0x2013f00: /* PBA stuff */ > >>>> + case 0x2013f01: /* PBA stuff */ > >>>> + case 0x2013f02: /* PBA stuff */ > >>>> + case 0x2013f03: /* PBA stuff */ > >>>> + case 0x2013f04: /* PBA stuff */ > >>>> + case 0x2013f05: /* PBA stuff */ > >>>> + case 0x2013f06: /* PBA stuff */ > >>>> + case 0x2013f07: /* PBA stuff */ > >>>> + val =3D 0; > >>>> + break; > >>> > >>> It'd be theoretically nicer to actually register regions for these > >>> special case addresses, but handling it here is a reasonable hack to > >>> get things working quickly for the time being. > >> > >> I will make a default region on the whole xscomm address space to catc= h=20 > >> these. > >=20 > > Ok. >=20 > Well, it does not bring much and we loose the ability to catch errors.=20 > I will leave it that way. >=20 > Thanks, >=20 > C.=20 >=20 --=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 --8X7/QrJGcKSMr1RN Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJX6dsBAAoJEGw4ysog2bOSwh0P/jsYqd/lFZF5yXGTlBWjB2RM dn1qlR2HMQwParNHTZhDUts5AlQ2koVpH1qk2L4t92InBzVYmeMIjPr8QtkfBu2G XZy4KrHyPj36Dv7pQwq41MIhvEjGK7Q/Q+A+/6MYXFzQgyfcQvIt5lK0wDzGid8N DpJbIVuIIS8Gm70f74Be/q5ei603yw6fPjoo6FnL+BSDsxvM5Mc9kfaavVo6FzL7 rsOSr+e9j3GfsTiGnZwawjAP5qgSxWSUg3CqzMXQj52nWb9avdqGgVWPO5mfSGgU u9I6tkOIGV5HPmNuJOqsYf6nzOIpgCKhF3mXzshks6H6mGAdHrGBTeElFosAPrDA RkHN+rVVXYPmOIY4BL5cZ0Rg5U+CiRiujVwJowvdC0rOw44SGOYWCJa/Sy/p0AL5 /2wTe2l0sURSu7QmXzU7j77ptaaU7cMJfs4yFc07VTJSODBNZFicFLI6Pn+CQm61 HJokgdyi2vmUPrV33+KiOdjs+7c9rPTPFVIiey3jT+m7tcgXgemXQe6h1RWhixfP mjvvpxyZwp9RmCbMAk59DSNwO/XLd22oAejq8bolngi3TSG3SgoYbRZt95/5OUFt KXZnMGr+4Q1l/j34ab9R+fgRMGgeiXFKNZRSh4hpCB9SWtCaksYjo9pTB3keBpX+ OHJcqnL4Br1TtjvyYRHS =Q1LH -----END PGP SIGNATURE----- --8X7/QrJGcKSMr1RN--