From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55746) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZbJTv-0003Nv-7D for qemu-devel@nongnu.org; Sun, 13 Sep 2015 22:27:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZbJTs-0002Cb-3O for qemu-devel@nongnu.org; Sun, 13 Sep 2015 22:26:59 -0400 Date: Mon, 14 Sep 2015 12:25:57 +1000 From: David Gibson Message-ID: <20150914022557.GE2547@voom.fritz.box> References: <20150901003808.GI11475@voom.redhat.com> <55E583A6.4000600@redhat.com> <20150908050316.GA372@tungsten.ozlabs.ibm.com> <20150908051528.GD24774@voom.redhat.com> <55F0A036.2090508@redhat.com> <55F13241.8040004@redhat.com> <20150910104051.GC11781@voom> <55F1719B.5070208@redhat.com> <20150911004527.GF11781@voom> <55F28314.7070608@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="IU5/I01NYhRvwH70" Content-Disposition: inline In-Reply-To: <55F28314.7070608@redhat.com> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth Cc: Alexander Graf , armbru@redhat.com, qemu-devel@nongnu.org, michael@ellerman.id.au, qemu-ppc@nongnu.org, amit.shah@redhat.com, Sam Bobroff --IU5/I01NYhRvwH70 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Sep 11, 2015 at 09:30:28AM +0200, Thomas Huth wrote: > On 11/09/15 02:45, David Gibson wrote: > > On Thu, Sep 10, 2015 at 02:03:39PM +0200, Thomas Huth wrote: > >> On 10/09/15 12:40, David Gibson wrote: > >>> On Thu, Sep 10, 2015 at 09:33:21AM +0200, Thomas Huth wrote: > >>>> On 09/09/15 23:10, Thomas Huth wrote: > >>>>> On 08/09/15 07:15, David Gibson wrote: > >>>> ... > >>>>>> At this point rather than just implementing them as discrete machi= ne > >>>>>> options, I suspect it will be more maintainable to split out the > >>>>>> h-random implementation as a pseudo-device with its own qdev and so > >>>>>> forth. We already do similarly for the RTAS time of day functions > >>>>>> (spapr-rtc). > >>>>> > >>>>> I gave that I try, but it does not work as expected. To be able to > >>>>> specify the options, I'd need to instantiate this device with the > >>>>> "-device" option, right? Something like: > >>>>> > >>>>> -device spapr-rng,backend=3Drng0,usekvm=3D0 > >>>>> > >>>>> Now this does not work when I use TYPE_SYS_BUS_DEVICE as parent cla= ss > >>>>> like it is done for spapr-rtc, since the user apparently can not pl= ug > >>>>> device to this bus on machine spapr (you can also not plug an spapr= -rtc > >>>>> device this way!). > >>>>> > >>>>> The spapr-vlan, spapr-vty, etc. devices are TYPE_VIO_SPAPR_DEVICE, = so I > >>>>> also tried that instead, but then the rng device suddenly shows up = under > >>>>> /vdevice in the device tree - that's also not what we want, I guess. > >>>> > >>>> I did some more tests, and I think I can get this working with one s= mall > >>>> modification to spapr_vio.c > >> ... > >>>> i.e. when the dt_name has not been set, the device won't be added to= the > >>>> /vdevice device tree node. If that's acceptable, I'll continue with = this > >>>> approach. > >>> > >>> A bit hacky. > >>> > >>> I think it would be preferable to build it under SysBus by default, > >>> like spapr-rtc. Properties can be set on the device using -global (or > >>> -set, but -global is easier). > >> > >> If anyhow possible, I'd prefere to use "-device" for this instead, bec= ause > >> > >> a) it's easier to use for the user, for example you can simply use > >> "-device spapr-rng,?" to get the list of properties - this > >> does not seem to work with spapr-rtc (it has a "date" property > >> which does not show up in the help text?) > >=20 > > Actually, I don't think that's got anything to do with -device versus > > otherwise. "date" doesn't appear because it's an "object" property > > rather than a "qdev" property - that distinction is subtle and > > confusing, yes. >=20 > At least it is not very friendly for the user ... if a configuration > property does not show up in the help text, you've got to document it > somewhere else or nobody will be aware of it. Not arguing with that. In this case it happened because I just copied the setup code from mc146818rtc which also doesn't set a description. > >> b) unlike the rtc device which is always instantiated, the rng > >> device is rather optional, so it is IMHO more intuitive if > >> created via the -device option. > >=20 > > Hrm, that's true though. And.. we're back at the perrenial question > > of what "standard" devices should be constructed by default. And what > > "default" means. > >=20 > > It seems to me that while the random device is optional, it should be > > created by default. But with -device there's not really a way to do > > that. But then again if it's constructed internally there's not > > really a way to turn it off short of hacky machine options. Ugh. > >=20 > >> So I'd like to give it a try with the TYPE_VIO_SPAPR_DEVICE first ... = if > >> you then still don't like the patches at all, I can still rework them = to > >> use TYPE_SYS_BUS_DEVICE instead. > >=20 > > I still dislike putting it on the VIO "bus", since PAPR doesn't > > consider it a VIO device. >=20 > Hmm, that's also a valid point. >=20 > After doing some more research, I think I've found yet another > possibility (why isn't there a proper documentation/howto for all this > QOM stuff ... or did I just miss it?) : Tell me about it. The fact that there are apparently a whole bunch of conventions about how QOM things should be done that are neither obvious nor document is starting to really irritate me. > Instead of using a bus, simply set parent =3D TYPE_DEVICE, so that it is a > "bus-less" device. Seems to work fine at a first glance, so unless > somebody tells me that this is a very bad idea, I'll try to rework my > patches accordingly... =46rom agraf's comment, this seems like the way to go. I'm still pretty confused about where such a device sits in the composition tree. I had thought that SysBus was the root of the qdev tree, but apparently not. --=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 --IU5/I01NYhRvwH70 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJV9jA1AAoJEGw4ysog2bOSyRkQAIsBJtrychunEIRZSNkHNYsu ZcLBBdmtSpe9L2z8ObK6ybW0mLV/kLvBU0jbmPKbCBPKBlheueFOGpu9eZbz/LZG nTylU20P+HJbV+r1/jlgICzuujm7EzeVbllzZjc8jQGXzMj4HoFKEwEEr4/vOoVx fZaPQpZRrMWSM3gaCEZ/2ttQYMawwP9KNyz9b3LDrNccFIyfxldb8Hbulb+aoR1A DlwkxGz3ZHvNMoBv88DaxrcFo1cGanUqVuiBkxi2SVuXmiAWyK2RjRWdPgdE7raX rWaZYrqE+UNhpyOZGd0JkqyoRxqOA0wWW5x31+6REFZlMEIKVd+Zg+1eDvyxFy15 F1zcG6hhLzL/inGiBT0NI3VaxOFkW6+DRs9ZCLqDbbpJ09rXzo1BpjDOvj/lfTai 1QKXeSJajJcKxRgsAiZcx7dSTHvlOFjMMQOvvinTg2LI9okSp4iSvt0Drn049Zul cXDPptlcelWHCPYNG7LUNjZdokromcnH+4IWwVrU0TfhCxnGDt2UdIj2bRE/omkM YWICKa2cUlbbadUUTzVC5yZK1dcTwqk37oPJlyNY5cdyBnzVn3jJ7IK/vziKRkrV L/prPj2MH7AWXf84LLYC26zb3NMg5mOP1iWVGn/UhCtYcD1ZjiFMwNZX9cffku56 TSK1IsjQaOe4KeFPlxPb =O5dm -----END PGP SIGNATURE----- --IU5/I01NYhRvwH70--