From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49646) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bolLs-0001U5-J2 for qemu-devel@nongnu.org; Tue, 27 Sep 2016 01:54:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bolLp-0003aw-Ex for qemu-devel@nongnu.org; Tue, 27 Sep 2016 01:54:48 -0400 Received: from 8.mo4.mail-out.ovh.net ([188.165.33.112]:58880) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bolLp-0003aY-4b for qemu-devel@nongnu.org; Tue, 27 Sep 2016 01:54:45 -0400 Received: from player691.ha.ovh.net (b9.ovh.net [213.186.33.59]) by mo4.mail-out.ovh.net (Postfix) with ESMTP id 9F8F5131B3 for ; Tue, 27 Sep 2016 07:54:42 +0200 (CEST) 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> From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Message-ID: Date: Tue, 27 Sep 2016 07:54:37 +0200 MIME-Version: 1.0 In-Reply-To: <20160927023548.GC15376@umbus.fritz.box> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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: David Gibson Cc: qemu-ppc@nongnu.org, Benjamin Herrenschmidt , qemu-devel@nongnu.org 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 bette= r >>>> 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 ah. yes, why not. We could have per-chip dispatchers but they=20 would have a lot in common. 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 > but I guess if you have the decoding of those "core" registers=20 > here as well, then that doesn't make so much sense. yes and there is also the handling of the XSCOM failures. 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. Thanks, C.=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 t= o >>>>> 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