Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available
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 : > >> 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
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available
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 pgp
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available
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 : > >>> > 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? -- 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 pgpF0UOoFop1o.pgp Description: PGP signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available
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 : >>> 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
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available
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
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available
On Thu, Sep 10, 2015 at 02:13:26PM +0200, Alexander Graf wrote: > > > > Am 10.09.2015 um 14:03 schrieb Thomas Huth : > > > >> 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? -- 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 pgpfCaFNbeF9n.pgp Description: PGP signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available
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 pgpcdYeK2iFSG.pgp Description: PGP signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available
> Am 10.09.2015 um 14:03 schrieb Thomas Huth : > >> 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
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available
On Wed, 9 Sep 2015 10:54:20 +1000 Sam Bobroff wrote: > On Tue, Sep 08, 2015 at 07:38:12AM +0200, Thomas Huth wrote: > > On 08/09/15 07:03, Sam Bobroff wrote: > > > On Tue, Sep 01, 2015 at 12:53:26PM +0200, Thomas Huth wrote: > > >> On 01/09/15 02:38, David Gibson wrote: > > >>> On Mon, Aug 31, 2015 at 08:46:01PM +0200, Thomas Huth wrote: > > From: Michael Ellerman > > > > Some powerpc systems have support for a hardware random number > > generator > > (hwrng). If such a hwrng is present the host kernel can provide access > > to it via the H_RANDOM hcall. > > > > The kernel advertises the presence of a hwrng with the > > KVM_CAP_PPC_HWRNG > > capability. If this is detected we add the appropriate device tree bits > > to advertise the presence of the hwrng to the guest kernel. > > > > Signed-off-by: Michael Ellerman > > [thuth: Refreshed patch so it applies to QEMU master branch] > > Signed-off-by: Thomas Huth > > >>> > > >>> So, I'm confused by one thing. > > >>> > > >>> I thought new kernel handled hcalls were supposed to be disabled by > > >>> default, but I don't see any calls to kvmppc_enable_hcall() to turn on > > >>> H_RANDOM. > > >> > > >> Michael's patch was from 2013, the kvmppc_enable_hcall() stuff seems to > > >> be from 2014 ... so the enablement is likely missing in this patch, > > >> indeed. I didn't test the in-kernel hypercall yet, just my QEMU > > >> implementation so far, that's why I did not notice this yet. > > >> > > >> Michael, do you want to rework your patch? Or shall I add an additional > > >> enablement patch to my queue? > > > > > > If I recall correctly, it's specifically not enabled: there was quite a > > > lot of > > > discussion about it when Michael posted the patches and I think the > > > consensus > > > was that it should only be enabled by QEMU, and only if the user could > > > decide > > > if it was used or not. > > > > Can you find this discussion in a mailing list archive somewhere? The > > only discussions I've found are basically these: > > > > http://lists.nongnu.org/archive/html/qemu-devel/2013-09/msg04233.html > > https://lkml.org/lkml/fancy/2013/10/1/49 > > > > ... and there it was only discussed that the call should be implemented > > in QEMU, too. I did not spot any discussion about making it switchable > > for the user? > > Sorry that part wasn't very well worded: I was trying to say that QEMU's > configuration should be able to choose how H_RANDOM handled, rather than > having > it automatically choose based on what was available in KVM and that's what > we're considering now anyway. (See David's comment elsewhere in this thread.) > > Sam. > > In this case, this patch isn't appropriate anymore: QEMU would advertise support for "ibm,random-v1" but guests would be returned H_FUNCTION. The DT bits should be in a later patch, after H_RANDOM is plugged in (either KVM or QEMU). Cheers. -- Greg
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available
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 signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available
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).
--
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
pgp_gN39bHorw.pgp
Description: PGP signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available
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.
Thomas
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available
On 08/09/15 07:15, David Gibson wrote: > On Tue, Sep 08, 2015 at 03:03:16PM +1000, Sam Bobroff wrote: >> On Tue, Sep 01, 2015 at 12:53:26PM +0200, Thomas Huth wrote: >>> On 01/09/15 02:38, David Gibson wrote: On Mon, Aug 31, 2015 at 08:46:01PM +0200, Thomas Huth wrote: > From: Michael Ellerman > > Some powerpc systems have support for a hardware random number generator > (hwrng). If such a hwrng is present the host kernel can provide access > to it via the H_RANDOM hcall. ... >> What if we set up another backend that just enables the hcall in KVM? > > I think that's basically the right approach. > > It can't quite be a "backend" as such, since the in-kernel hcall can > only supply H_RANDOM; it can't supply random for other purposes like > virtio-rng, which the general qemu rng backends can. > > So I'd suggest two options controlling H_RANDOM: > usekvm : boolean [default true] > Whether to enable the in-kernel implementation if > available > backend : ref to rng backend object [no default] > Backend to use if in-kernel implementation is > unavailable or disabled. > > 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. So I am currently not sure whether this is the right approach. Any recommendations? Or shall I stick with the machine option? Thomas signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available
On Tue, Sep 08, 2015 at 07:38:12AM +0200, Thomas Huth wrote: > On 08/09/15 07:03, Sam Bobroff wrote: > > On Tue, Sep 01, 2015 at 12:53:26PM +0200, Thomas Huth wrote: > >> On 01/09/15 02:38, David Gibson wrote: > >>> On Mon, Aug 31, 2015 at 08:46:01PM +0200, Thomas Huth wrote: > From: Michael Ellerman > > Some powerpc systems have support for a hardware random number generator > (hwrng). If such a hwrng is present the host kernel can provide access > to it via the H_RANDOM hcall. > > The kernel advertises the presence of a hwrng with the KVM_CAP_PPC_HWRNG > capability. If this is detected we add the appropriate device tree bits > to advertise the presence of the hwrng to the guest kernel. > > Signed-off-by: Michael Ellerman > [thuth: Refreshed patch so it applies to QEMU master branch] > Signed-off-by: Thomas Huth > >>> > >>> So, I'm confused by one thing. > >>> > >>> I thought new kernel handled hcalls were supposed to be disabled by > >>> default, but I don't see any calls to kvmppc_enable_hcall() to turn on > >>> H_RANDOM. > >> > >> Michael's patch was from 2013, the kvmppc_enable_hcall() stuff seems to > >> be from 2014 ... so the enablement is likely missing in this patch, > >> indeed. I didn't test the in-kernel hypercall yet, just my QEMU > >> implementation so far, that's why I did not notice this yet. > >> > >> Michael, do you want to rework your patch? Or shall I add an additional > >> enablement patch to my queue? > > > > If I recall correctly, it's specifically not enabled: there was quite a lot > > of > > discussion about it when Michael posted the patches and I think the > > consensus > > was that it should only be enabled by QEMU, and only if the user could > > decide > > if it was used or not. > > Can you find this discussion in a mailing list archive somewhere? The > only discussions I've found are basically these: > > http://lists.nongnu.org/archive/html/qemu-devel/2013-09/msg04233.html > https://lkml.org/lkml/fancy/2013/10/1/49 > > ... and there it was only discussed that the call should be implemented > in QEMU, too. I did not spot any discussion about making it switchable > for the user? Sorry that part wasn't very well worded: I was trying to say that QEMU's configuration should be able to choose how H_RANDOM handled, rather than having it automatically choose based on what was available in KVM and that's what we're considering now anyway. (See David's comment elsewhere in this thread.) Sam.
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available
On 08/09/15 07:03, Sam Bobroff wrote: > On Tue, Sep 01, 2015 at 12:53:26PM +0200, Thomas Huth wrote: >> On 01/09/15 02:38, David Gibson wrote: >>> On Mon, Aug 31, 2015 at 08:46:01PM +0200, Thomas Huth wrote: From: Michael Ellerman Some powerpc systems have support for a hardware random number generator (hwrng). If such a hwrng is present the host kernel can provide access to it via the H_RANDOM hcall. The kernel advertises the presence of a hwrng with the KVM_CAP_PPC_HWRNG capability. If this is detected we add the appropriate device tree bits to advertise the presence of the hwrng to the guest kernel. Signed-off-by: Michael Ellerman [thuth: Refreshed patch so it applies to QEMU master branch] Signed-off-by: Thomas Huth >>> >>> So, I'm confused by one thing. >>> >>> I thought new kernel handled hcalls were supposed to be disabled by >>> default, but I don't see any calls to kvmppc_enable_hcall() to turn on >>> H_RANDOM. >> >> Michael's patch was from 2013, the kvmppc_enable_hcall() stuff seems to >> be from 2014 ... so the enablement is likely missing in this patch, >> indeed. I didn't test the in-kernel hypercall yet, just my QEMU >> implementation so far, that's why I did not notice this yet. >> >> Michael, do you want to rework your patch? Or shall I add an additional >> enablement patch to my queue? > > If I recall correctly, it's specifically not enabled: there was quite a lot of > discussion about it when Michael posted the patches and I think the consensus > was that it should only be enabled by QEMU, and only if the user could decide > if it was used or not. Can you find this discussion in a mailing list archive somewhere? The only discussions I've found are basically these: http://lists.nongnu.org/archive/html/qemu-devel/2013-09/msg04233.html https://lkml.org/lkml/fancy/2013/10/1/49 ... and there it was only discussed that the call should be implemented in QEMU, too. I did not spot any discussion about making it switchable for the user? Thomas
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available
On Tue, Sep 08, 2015 at 03:03:16PM +1000, Sam Bobroff wrote: > On Tue, Sep 01, 2015 at 12:53:26PM +0200, Thomas Huth wrote: > > On 01/09/15 02:38, David Gibson wrote: > > > On Mon, Aug 31, 2015 at 08:46:01PM +0200, Thomas Huth wrote: > > >> From: Michael Ellerman > > >> > > >> Some powerpc systems have support for a hardware random number generator > > >> (hwrng). If such a hwrng is present the host kernel can provide access > > >> to it via the H_RANDOM hcall. > > >> > > >> The kernel advertises the presence of a hwrng with the KVM_CAP_PPC_HWRNG > > >> capability. If this is detected we add the appropriate device tree bits > > >> to advertise the presence of the hwrng to the guest kernel. > > >> > > >> Signed-off-by: Michael Ellerman > > >> [thuth: Refreshed patch so it applies to QEMU master branch] > > >> Signed-off-by: Thomas Huth > > > > > > So, I'm confused by one thing. > > > > > > I thought new kernel handled hcalls were supposed to be disabled by > > > default, but I don't see any calls to kvmppc_enable_hcall() to turn on > > > H_RANDOM. > > > > Michael's patch was from 2013, the kvmppc_enable_hcall() stuff seems to > > be from 2014 ... so the enablement is likely missing in this patch, > > indeed. I didn't test the in-kernel hypercall yet, just my QEMU > > implementation so far, that's why I did not notice this yet. > > > > Michael, do you want to rework your patch? Or shall I add an additional > > enablement patch to my queue? > > If I recall correctly, it's specifically not enabled: there was quite a lot of > discussion about it when Michael posted the patches and I think the consensus > was that it should only be enabled by QEMU, and only if the user could decide > if it was used or not. > > What if we set up another backend that just enables the hcall in KVM? I think that's basically the right approach. It can't quite be a "backend" as such, since the in-kernel hcall can only supply H_RANDOM; it can't supply random for other purposes like virtio-rng, which the general qemu rng backends can. So I'd suggest two options controlling H_RANDOM: usekvm : boolean [default true] Whether to enable the in-kernel implementation if available backend : ref to rng backend object [no default] Backend to use if in-kernel implementation is unavailable or disabled. 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). -- 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 pgprpLOz3x8qW.pgp Description: PGP signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available
On Tue, Sep 01, 2015 at 12:53:26PM +0200, Thomas Huth wrote: > On 01/09/15 02:38, David Gibson wrote: > > On Mon, Aug 31, 2015 at 08:46:01PM +0200, Thomas Huth wrote: > >> From: Michael Ellerman > >> > >> Some powerpc systems have support for a hardware random number generator > >> (hwrng). If such a hwrng is present the host kernel can provide access > >> to it via the H_RANDOM hcall. > >> > >> The kernel advertises the presence of a hwrng with the KVM_CAP_PPC_HWRNG > >> capability. If this is detected we add the appropriate device tree bits > >> to advertise the presence of the hwrng to the guest kernel. > >> > >> Signed-off-by: Michael Ellerman > >> [thuth: Refreshed patch so it applies to QEMU master branch] > >> Signed-off-by: Thomas Huth > > > > So, I'm confused by one thing. > > > > I thought new kernel handled hcalls were supposed to be disabled by > > default, but I don't see any calls to kvmppc_enable_hcall() to turn on > > H_RANDOM. > > Michael's patch was from 2013, the kvmppc_enable_hcall() stuff seems to > be from 2014 ... so the enablement is likely missing in this patch, > indeed. I didn't test the in-kernel hypercall yet, just my QEMU > implementation so far, that's why I did not notice this yet. > > Michael, do you want to rework your patch? Or shall I add an additional > enablement patch to my queue? If I recall correctly, it's specifically not enabled: there was quite a lot of discussion about it when Michael posted the patches and I think the consensus was that it should only be enabled by QEMU, and only if the user could decide if it was used or not. What if we set up another backend that just enables the hcall in KVM? This would answer the question about what to do if both are available as well: just do only what the user has asked for. (But I'm not sure what the default behaviour should be if the user doesn't specify anything.) (Oh also: I have tested the in-kernel hcall using a QEMU modified to enable it and it seems to work :-) Cheers, Sam.
