Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3243 bytes --]

On Thu, Sep 10, 2015 at 02:13:26PM +0200, Alexander Graf wrote:
> 
> 
> > Am 10.09.2015 um 14:03 schrieb Thomas Huth <thuth@redhat.com>:
> > 
> >> 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 machine
> >>>>> 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=rng0,usekvm=0
> >>>> 
> >>>> Now this does not work when I use TYPE_SYS_BUS_DEVICE as parent class
> >>>> like it is done for spapr-rtc, since the user apparently can not plug
> >>>> 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 small
> >>> 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, because
> > 
> > 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?)
> > 
> > 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.
> > 
> > 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.
> 
> Please don't use sysbus. If the vio device approach turns ugly,
> create a new spapr hcall bus instead. We should have had that from
> the beginning really.

Ok.. why?

It's a system (pseudo-)device that doesn't have any common bus
infrastructure with anything else.  Isn't that what SysBus is for?

-- 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]