From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39329) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bhSEm-0005oD-22 for qemu-devel@nongnu.org; Tue, 06 Sep 2016 22:05:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bhSEh-0008Em-7J for qemu-devel@nongnu.org; Tue, 06 Sep 2016 22:05:14 -0400 Date: Wed, 7 Sep 2016 11:51:58 +1000 From: David Gibson Message-ID: <20160907015158.GD2780@voom.fritz.box> References: <1472661255-20160-1-git-send-email-clg@kaod.org> <1472661255-20160-7-git-send-email-clg@kaod.org> <20160905041921.GD3816@voom.fritz.box> <17f34eeb-0921-98da-e969-d65f7f50de4a@kaod.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="5p8PegU4iirBW1oA" Content-Disposition: inline In-Reply-To: <17f34eeb-0921-98da-e969-d65f7f50de4a@kaod.org> Subject: Re: [Qemu-devel] [PATCH v2 6/7] ppc/pnv: add a XScomDevice to PnvCore 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 --5p8PegU4iirBW1oA Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Sep 06, 2016 at 03:54:10PM +0200, C=E9dric Le Goater wrote: > On 09/05/2016 06:19 AM, David Gibson wrote: > > On Wed, Aug 31, 2016 at 06:34:14PM +0200, C=E9dric Le Goater wrote: > >> Now that we are using real HW ids for the cores in PowerNV chips, we > >> can route the XSCOM accesses to them. We just need to attach a > >> XScomDevice to each core with the associated ranges in the XSCOM > >> address space. > >> > >> To start with, let's install the DTS (Digital Thermal Sensor) handlers > >> which are easy to handle. > >> > >> Signed-off-by: C=E9dric Le Goater > >> --- > >> hw/ppc/pnv.c | 9 +++++++ > >> hw/ppc/pnv_core.c | 67 ++++++++++++++++++++++++++++++++++++++= +++++++++ > >> include/hw/ppc/pnv_core.h | 13 +++++++++ > >> 3 files changed, 89 insertions(+) > >> > >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > >> index daf9f459ab0e..a31568415192 100644 > >> --- a/hw/ppc/pnv.c > >> +++ b/hw/ppc/pnv.c > >> @@ -527,6 +527,7 @@ static void pnv_chip_realize(DeviceState *dev, Err= or **errp) > >> for (i =3D 0, core_hwid =3D 0; (core_hwid < sizeof(chip->cores_ma= sk) * 8) > >> && (i < chip->num_cores); core_hwid++) { > >> PnvCore *pnv_core =3D &chip->cores[i]; > >> + DeviceState *qdev; > >> =20 > >> if (!(chip->cores_mask & (1 << core_hwid))) { > >> continue; > >> @@ -542,6 +543,14 @@ static void pnv_chip_realize(DeviceState *dev, Er= ror **errp) > >> &error_fatal); > >> object_unref(OBJECT(pnv_core)); > >> i++; > >> + > >> + /* Attach the core to its XSCOM bus */ > >> + qdev =3D qdev_create(&chip->xscom->bus, TYPE_PNV_CORE_XSCOM); > >=20 > > Again, I think this breaks QOM lifetime rules. > >=20 > >> + qdev_prop_set_uint32(qdev, "core-pir", > >> + P8_PIR(chip->chip_id, core_hwid)); > >> + qdev_init_nofail(qdev); > >=20 > > qdev_init_nofail() - which will abort on failure - shouldn't be used > > in a realize() function, which has a way to gracefully report errors. >=20 > yes. I kind of knew that was ugly but the main purpose of the patch=20 > is the use of scom. So I took the quick and dirty path :) >=20 > we should do what you propose below: > =20 > > So, in terms of the lifetime thing. I think one permitted solution is > > to embed the scom device state in the core device state and use > > object_initialize(). >=20 > yes. >=20 > > Alternatively, since SCOM is by its nature a sideband bus, I wonder if > > it would make sense to have ScomDevice be a QOM interface, rather than > > a full QOM class. That way the core device itself (and other devices > > with SCOM control) could present the SCOM device interface and be > > placed directly on the SCOM bus without having to introduce an extra > > object. =20 >=20 > what you are proposing is to have a PnvCore inherit from CPUCore and=20 > XScomDevice ? I tried that and did not find a way for multi-inheritance.= =20 QOM supports multiple interface inheritance, but not multiple direct inheritance. That's why youd need to change XScomDevice from a class to an interface, as I propose in a different mail. Not sure if that will cause other problems with the qbus infrastructure. Have a look at say, 'HotplugHandler' for an example of a QOM interface. > But yes, we would get rid of the extra object. At the same time, that=20 > extra object represents the xscom interface unit in a chiplet which=20 > exists on real HW. >=20 > > It will probably make accessing the object innards in response to=20 > > SCOM requests more straightforward as well. >=20 > The address space is probably the best approach. I have some experimental= =20 > patches which look pretty good : >=20 > address-space: xscom-0 > 0000000000000000-00000007ffffffff (prio 0, RW): xscom-0 > 0000000000b00200-0000000000b0021f (prio 0, RW): lpc-xscom > 0000000120000000-00000001207fffff (prio 0, RW): core-20-xscom > 0000000121000000-00000001217fffff (prio 0, RW): core-21-xscom > 0000000122000000-00000001227fffff (prio 0, RW): core-22-xscom > 0000000123000000-00000001237fffff (prio 0, RW): core-23-xscom > 0000000124000000-00000001247fffff (prio 0, RW): core-24-xscom > 0000000125000000-00000001257fffff (prio 0, RW): core-25-xscom > 0000000126000000-00000001267fffff (prio 0, RW): core-26-xscom > .... >=20 >=20 > > I'm not entirely sure if sharing just an interface will be sufficient > > for devices under a bus, but it's worth investigating. >=20 > yes. I need to step back a bit and look how I could rework the patchset= =20 > to QOMify the whole. This is really qdev oriented and there are a couple= =20 > of places where fixes are needed. I am seeing that better now.=20 >=20 > The address space should be investigated also. >=20 > >> + > >> + pnv_core->xd =3D PNV_CORE_XSCOM(qdev); > >> } > >> g_free(typename); > >> =20 > >> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c > >> index 825aea1194a1..feba374740dc 100644 > >> --- a/hw/ppc/pnv_core.c > >> +++ b/hw/ppc/pnv_core.c > >> @@ -18,7 +18,9 @@ > >> */ > >> #include "qemu/osdep.h" > >> #include "sysemu/sysemu.h" > >> +#include "qemu/error-report.h" > >> #include "qapi/error.h" > >> +#include "qemu/log.h" > >> #include "target-ppc/cpu.h" > >> #include "hw/ppc/ppc.h" > >> #include "hw/ppc/pnv.h" > >> @@ -144,10 +146,75 @@ static const TypeInfo pnv_core_info =3D { > >> .abstract =3D true, > >> }; > >> =20 > >> + > >> +#define DTS_RESULT0 0x50000 > >> +#define DTS_RESULT1 0x50001 > >> + > >> +static bool pnv_core_xscom_read(XScomDevice *dev, uint32_t range, > >> + uint32_t offset, uint64_t *out_val) > >> +{ > >> + switch (offset) { > >> + case DTS_RESULT0: > >> + *out_val =3D 0x26f024f023f0000ull; > >> + break; > >> + case DTS_RESULT1: > >> + *out_val =3D 0x24f000000000000ull; > >> + break; > >> + default: > >> + qemu_log_mask(LOG_GUEST_ERROR, "Warning: reading reg=3D0x%08x= ", offset); > >=20 > > Can't you just return false here and let the caller handle the error re= porting? >=20 > yes.=20 >=20 > Thanks, >=20 > C.=20 >=20 > >> + } > >> + > >> + return true; > >> +} > >> + > >> +static bool pnv_core_xscom_write(XScomDevice *dev, uint32_t range, > >> + uint32_t offset, uint64_t val) > >> +{ > >> + qemu_log_mask(LOG_GUEST_ERROR, "Warning: writing to reg=3D0x%08x"= , offset); > >> + return true; > >> +} > >> + > >> +#define EX_XSCOM_BASE 0x10000000 > >> +#define EX_XSCOM_SIZE 0x100000 > >> + > >> +static void pnv_core_xscom_realize(DeviceState *dev, Error **errp) > >> +{ > >> + XScomDevice *xd =3D XSCOM_DEVICE(dev); > >> + PnvCoreXScom *pnv_xd =3D PNV_CORE_XSCOM(dev); > >> + > >> + xd->ranges[0].addr =3D EX_XSCOM_BASE | P8_PIR2COREID(pnv_xd->core= _pir) << 24; > >> + xd->ranges[0].size =3D EX_XSCOM_SIZE; > >> +} > >> + > >> +static Property pnv_core_xscom_properties[] =3D { > >> + DEFINE_PROP_UINT32("core-pir", PnvCoreXScom, core_pir, 0), > >> + DEFINE_PROP_END_OF_LIST(), > >> +}; > >> + > >> +static void pnv_core_xscom_class_init(ObjectClass *klass, void *data) > >> +{ > >> + DeviceClass *dc =3D DEVICE_CLASS(klass); > >> + XScomDeviceClass *xdc =3D XSCOM_DEVICE_CLASS(klass); > >> + > >> + xdc->read =3D pnv_core_xscom_read; > >> + xdc->write =3D pnv_core_xscom_write; > >> + > >> + dc->realize =3D pnv_core_xscom_realize; > >> + dc->props =3D pnv_core_xscom_properties; > >> +} > >> + > >> +static const TypeInfo pnv_core_xscom_type_info =3D { > >> + .name =3D TYPE_PNV_CORE_XSCOM, > >> + .parent =3D TYPE_XSCOM_DEVICE, > >> + .instance_size =3D sizeof(PnvCoreXScom), > >> + .class_init =3D pnv_core_xscom_class_init, > >> +}; > >> + > >> static void pnv_core_register_types(void) > >> { > >> int i ; > >> =20 > >> + type_register_static(&pnv_core_xscom_type_info); > >> type_register_static(&pnv_core_info); > >> for (i =3D 0; i < ARRAY_SIZE(pnv_core_models); ++i) { > >> TypeInfo ti =3D { > >> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h > >> index 832c8756afaa..72936ccfd22f 100644 > >> --- a/include/hw/ppc/pnv_core.h > >> +++ b/include/hw/ppc/pnv_core.h > >> @@ -20,6 +20,18 @@ > >> #define _PPC_PNV_CORE_H > >> =20 > >> #include "hw/cpu/core.h" > >> +#include "hw/ppc/pnv_xscom.h" > >> + > >> +#define TYPE_PNV_CORE_XSCOM "powernv-cpu-core-xscom" > >> +#define PNV_CORE_XSCOM(obj) \ > >> + OBJECT_CHECK(PnvCoreXScom, (obj), TYPE_PNV_CORE_XSCOM) > >> + > >> +typedef struct PnvCoreXScom { > >> + XScomDevice xd; > >> + uint32_t core_pir; > >> +} PnvCoreXScom; > >> + > >> +#define P8_PIR2COREID(pir) (((pir) >> 3) & 0xf) > >> =20 > >> #define TYPE_PNV_CORE "powernv-cpu-core" > >> #define PNV_CORE(obj) \ > >> @@ -35,6 +47,7 @@ typedef struct PnvCore { > >> =20 > >> /*< public >*/ > >> void *threads; > >> + PnvCoreXScom *xd; > >> } PnvCore; > >> =20 > >> typedef struct PnvCoreClass { > >=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 --5p8PegU4iirBW1oA Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBAgAGBQJXz3K+AAoJEGw4ysog2bOSBc0QAIXhMMtT+CG272ATjExgZ0qv Hgkym8hWyq5JInMI8G961ByiFXP+ErWVXueSo3obLvJFlGdWcmPfxoDEPEZQv73l HAgiatIK2TIsQa7roeuQStM9nqOlpyUEX9e755WdCuHMo1TL1n40Z5/cVc1M2jHY vyZCqkcDop+0CeNohPHD6qoKmW4pPscd8cH4iQMtlW/RJOPS2BEPx0fvtVZ737Q7 P/vSz+PGo0tidyfBG/JZuW+w1jhy9ifY70RAwNsdSKzeXLcPd7hcBNTIp4wD9t/a 4JtMlvfwS832OcQ3NE4n/HOiemParRMfW4u7rmD2BJribz6KX4yj4OR/DI7FWYpE 4KqXBRZt03+UOHbzxjGtjXDt5DyuDHyzT7eFZIcy6eQSMKyMyGC/uSrmmJzkyRUU 2s6C2y3KHkjWArhyVCifKyaqhrzaUeYChmIPicxqTLyUBTaGiscH+kZcWShzv5fA H6zNvpm72WnJNs2ZNEO0eCD3Cjj34AKVXworv0ZGugihN1L4h466semdkVuZm5X0 Gi75kxhewGYKsZ4IXa3ZnrrrHLj+OlsLgFWAOipg8KWlh3pcUwkcinxnTtIMum/Z UJy14cFJvTv1yThNZ6L7RwIePFnw6/uIILSRh61m0G1CS3yKyKZ46iOdxdYKfBjQ 03X4rBY+pV+K+JdvnU30 =qqI0 -----END PGP SIGNATURE----- --5p8PegU4iirBW1oA--