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

[David Gibson]

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 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?)
> > 
> > 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.
> 
> 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.
> > 
> > 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.
> > 
> > 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.
> > 
> >> 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.
> > 
> > I still dislike putting it on the VIO "bus", since PAPR doesn't
> > consider it a VIO device.
> 
> Hmm, that's also a valid point.
> 
> 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 = 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...

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

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

pgpk0lQIrInh2.pgp
Description: PGP signature