From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38619) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bp4fO-0005En-2O for qemu-devel@nongnu.org; Tue, 27 Sep 2016 22:32:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bp4fL-00012B-Ct for qemu-devel@nongnu.org; Tue, 27 Sep 2016 22:32:12 -0400 Date: Wed, 28 Sep 2016 11:40:11 +1000 From: David Gibson Message-ID: <20160928014011.GA18880@umbus> 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> <20160927023548.GC15376@umbus.fritz.box> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="wRRV7LY7NUeQGEoC" 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 --wRRV7LY7NUeQGEoC Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Sep 27, 2016 at 07:54:37AM +0200, C=E9dric Le Goater wrote: > On 09/27/2016 04:35 AM, David Gibson wrote: > > 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(ObjectC= lass *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 probab= ly > >>>>> 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 ei= ther: > >>>>> > >>>>> P8: * map one big subregion for the ADU into &address_space_mem= ory > >>>>> * have the handler for that subregion do the address mangli= ng, > >>>>> 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= the=20 > >>>> HMER topic. I need to look at that. > >>> > >>> Right. Doesn't change the basic concept though - it just means you > >>> need (slightly different) redispatchers for both P8 and P9. > >> > >> In fact they are the same, you only need an "addr to pcba" handler at = the > >> chip class level :=20 > >=20 > > Ok. I'd been thinking of using different dispatchers as an > > alternative to using the chip class translator hook,=20 >=20 > ah. yes, why not. We could have per-chip dispatchers but they=20 > would have a lot in common. Would they? Unless you're counting the core register dispatch - and it sounds like splitting that for P8 vs. P9 would be a good idea anyway - I don't see that there's much in common besides the address translation. Note of course, that you can add a helper function that both dispatchers can use if it's useful. > However, I think we can get rid of=20 > the xscom_pcba' handlers, they should not be needed any where=20 > else than in the XSCOM dispatchers.=20 >=20 > > but I guess if you have the decoding of those "core" registers=20 > > here as well, then that doesn't make so much sense. >=20 > yes and there is also the handling of the XSCOM failures. Hm, ok. > I can add some prologue handler to cover those "core" registers > but adding a MemoryRegion, ops, init and mapping would be a lot=20 > of churn just to return 0. >=20 > Thanks, >=20 > C.=20 >=20 >=20 > >> 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; > >> > >> ... > >> > >> val =3D address_space_ldq(&chip->xscom_as, pcba << 3, > >> MEMTXATTRS_UNSPECIFIED, &result); > >> if (result !=3D MEMTX_OK) { > >> > >> =20 > >> > >> And so, the result is pretty clean. I killed the proxy object and merg= ed=20 > >> the regions in the chip but I have kept the pnv_xscom.c file because t= he=20 > >> code related to xscom is rather large : ~250 lines.=20 > >=20 > > Sure, makes sense. > >=20 > >> The objects declaring a xscom region need to do some register shifting= but=20 > >> this is usual in mmio regions. > >> > >> You will see in v4. > >=20 > > Ok. > >=20 > >>>>>> +static bool xscom_dispatch_read(PnvXScom *xscom, hwaddr addr, uin= t64_t *val) > >>>>>> +{ > >>>>>> + uint32_t success; > >>>>>> + uint8_t data[8]; > >>>>>> + > >>>>>> + success =3D !address_space_rw(&xscom->xscom_as, addr, MEMTXAT= TRS_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 ab= le > >>>>> to avoid this mangling. > >>>> > >>>> yes. I should but curiously I had to do this, and this works the sam= e on > >>>> an intel host or a ppc64 host. > >>> > >>> Hmm.. I suspect what you actually need is NATIVE_ENDIAN on the > >>> individual SCOM devices, with BIG_ENDIAN on the redispatcher region. > >> > >> we should be using address_space_ldq and address_space_stq. > >=20 > > Ok. > >=20 > >>>>>> + > >>>>>> + success =3D !address_space_rw(&xscom->xscom_as, addr, MEMTXAT= TRS_UNSPECIFIED, > >>>>>> + data, 8, true); > >>>>>> + return success; > >>>>>> +} > >>>>>> + > >>>>>> +static uint64_t xscom_read(void *opaque, hwaddr addr, unsigned wi= dth) > >>>>>> +{ > >>>>>> + 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 ca= tch=20 > >>>> these. > >>> > >>> Ok. > >> > >> Well, it does not bring much and we loose the ability to catch errors.= =20 > >> I will leave it that way. > >> > >> Thanks, > >> > >> C.=20 > >> > >=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 --wRRV7LY7NUeQGEoC Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJX6x96AAoJEGw4ysog2bOS/GIQANsaRWY9U0Nl8greu3F5J592 ftw4YYOVnXYMVQTvmmov9U2IuBkwKTyMHgZgZGFfn8vcyjIsRqLxEqWFLOBz7rsH B1AIig6Rb9/uWXOtJuT4vK9N2Qvpn/WMqwAxTdq9YTbeJOrF6gFQk5GH/zIdY5je iWyitndKadh1boZLmMepWT3P/iFkXM9h02YMZwwqKuMVdvEwq1y4a0wro+ccR6gL XA9mLEDHGdbpQfIGHYsUq1X4gxjKrJ2bqyxyGthrB3G31HGRFf3s7ZDqHK7l83Dc 7QffSEYynkhxijnoWiCY/EqGNMpu+B0Ui5YlPa8oNd8WLqOUg8GVrbZxTQZgwlMv o4EkV4gqQeYgW2WhlaK47JX5bsJAKi+K5oypEcMDUTNNUVGj7Pm/0hUiqn7A/sBp rLHBZ/GwDigtgHdMx5IYVBdj994AaGFsvnLa0DbxDhi5ScraRvPomjP4Ju3jUNXs oot/6s6r01Q8GuSaH+x/EU8ZIRBV49P+fc3Vyc/JjtlPAHpKiaSjClpmRDQPplaW ce7qknAOih/UktMMO0eU+wtH91rzv+yijOt9dIU8pJW2l69K5dzoBWJk57psWkKH VNkZ65nx6HNEIHkLYbqllRrm0pZckuPWNlAplJxwC0dk+zuNmgmTBhZ/YjujH9DW yXuyS36p8qfxD+C+CHCR =wMhq -----END PGP SIGNATURE----- --wRRV7LY7NUeQGEoC--