| Message ID | 55F13241.8040004@redhat.com |
|---|---|
| State | New |
| Headers | show |
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: > > diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c > index c51eb8e..8e7f6b4 100644 > --- a/hw/ppc/spapr_vio.c > +++ b/hw/ppc/spapr_vio.c > @@ -99,6 +99,14 @@ static int vio_make_devnode(VIOsPAPRDevice *dev, > int vdevice_off, node_off, ret; > char *dt_name; > > + if (!pc->dt_name) { > + ret = 0; > + if (pc->devnode) { > + ret = (pc->devnode)(dev, fdt, -1); > + } > + return ret; > + } > + > vdevice_off = fdt_path_offset(fdt, "/vdevice"); > if (vdevice_off < 0) { > return vdevice_off; > > 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).
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. Thomas
> 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. Alex
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.
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?
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
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. 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. Alex
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.
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? > 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?
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
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c index c51eb8e..8e7f6b4 100644 --- a/hw/ppc/spapr_vio.c +++ b/hw/ppc/spapr_vio.c @@ -99,6 +99,14 @@ static int vio_make_devnode(VIOsPAPRDevice *dev, int vdevice_off, node_off, ret; char *dt_name; + if (!pc->dt_name) { + ret = 0; + if (pc->devnode) { + ret = (pc->devnode)(dev, fdt, -1); + } + return ret; + } + vdevice_off = fdt_path_offset(fdt, "/vdevice"); if (vdevice_off < 0) { return vdevice_off;