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

[Alexander Graf <agraf@suse.de>]

On 14.09.15 04:27, David Gibson wrote:
> On Fri, Sep 11, 2015 at 11:43:02AM +0200, Alexander Graf wrote:
>>
>>
>> On 11.09.15 02:46, David Gibson wrote:
>>> 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?
>>
>> No, sysbus means "A device that has MMIO and/or PIO connected via a bus
>> I'm too lazy to model" really. These devices have neither.
> 
> Oh.
> 
> So.. where is one supposed to find that out?

You could ask the same about any bus really. It's more or less common
sense / collective knowledge / call it what you want.

Just check out the sysbus code files and you'll see that 90% of them are
about handling mmio / pio and irqs. Do you need that logic? No? Then
sysbus is not for you :).

> 
>> Back in the days before QOM, sysbus was our lowest common denominator,
>> but now that we have TYPE_DEVICE and can branch off of that, we really
>> shouldn't abuse sysbus devices for things they aren't.
> 
> So what actually is the root of the qdev tree then?

qdev is legacy, qom is new :). In qdev sysbus was the root bus, in qom
it's not. For details on what exactly is the root for qom, please just
poke Andreas - I keep having a hard time to wrap my head around the qom
topology. I'm not even sure it has a root in the traditional sense.


Alex