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

[David Gibson]

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.

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

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

pgpcxze3623Up.pgp
Description: PGP signature