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

[Thomas Huth]

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.

>> 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?) :
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...

 Thomas

signature.asc
Description: OpenPGP digital signature