| Message ID | 20150928101346.23919.3988.stgit@bahia.huguette.org |
|---|---|
| State | New |
| Headers | show |
Cc'ing qemu-ppc@ On Mon, 28 Sep 2015 12:13:47 +0200 Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > A recent patch by Thomas Huth brought a new spapr-rng pseudo-device to > provide high-quality random numbers to guests. The device may either be > backed by a "RngBackend" or the in-kernel implementation of the H_RANDOM > hypercall. > > Since modern POWER8 based servers always provide a hardware rng, it makes > sense to create a spapr-rng device with use-kvm=true by default when it > is available. > > Of course we want the user to have full control on how the rng is handled. > The default device WILL NOT be created in the following cases: > - the -nodefaults option was passed > - a spapr-rng device was already passed on the command line > > The default device is created at reset time to ensure devices specified on > the command line have been created. > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > --- > hw/ppc/spapr.c | 17 +++++++++++++++++ > hw/ppc/spapr_rng.c | 2 +- > target-ppc/kvm.c | 9 +++++---- > target-ppc/kvm_ppc.h | 6 ++++++ > 4 files changed, 29 insertions(+), 5 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 7f4f196e53e5..ee048ecffd0c 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1059,6 +1059,14 @@ static int spapr_check_htab_fd(sPAPRMachineState *spapr) > return rc; > } > > +static void spapr_rng_create(void) > +{ > + Object *rng = object_new(TYPE_SPAPR_RNG); > + > + object_property_set_bool(rng, true, "use-kvm", &error_abort); > + object_property_set_bool(rng, true, "realized", &error_abort); > +} > + > static void ppc_spapr_reset(void) > { > sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > @@ -1082,6 +1090,15 @@ static void ppc_spapr_reset(void) > spapr->rtas_addr = rtas_limit - RTAS_MAX_SIZE; > spapr->fdt_addr = spapr->rtas_addr - FDT_MAX_SIZE; > > + /* Create a rng device if the user did not provide it already and > + * KVM has hwrng support. > + */ > + if (defaults_enabled() && > + kvmppc_hwrng_present() && > + !object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)) { > + spapr_rng_create(); > + } > + > /* Load the fdt */ > spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr, > spapr->rtas_size); > diff --git a/hw/ppc/spapr_rng.c b/hw/ppc/spapr_rng.c > index ed43d5e04221..ee5af302bd4d 100644 > --- a/hw/ppc/spapr_rng.c > +++ b/hw/ppc/spapr_rng.c > @@ -114,7 +114,7 @@ static void spapr_rng_realize(DeviceState *dev, Error **errp) > sPAPRRngState *rngstate = SPAPR_RNG(dev); > > if (rngstate->use_kvm) { > - if (kvmppc_enable_hwrng() == 0) { > + if (kvmppc_hwrng_present() && kvmppc_enable_hwrng() == 0) { > return; > } > /* > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c > index e641680fb146..084bb034f1fd 100644 > --- a/target-ppc/kvm.c > +++ b/target-ppc/kvm.c > @@ -2490,11 +2490,12 @@ int kvm_arch_msi_data_to_gsi(uint32_t data) > return data & 0xffff; > } > > -int kvmppc_enable_hwrng(void) > +bool kvmppc_hwrng_present(void) > { > - if (!kvm_enabled() || !kvm_check_extension(kvm_state, KVM_CAP_PPC_HWRNG)) { > - return -1; > - } > + return kvm_enabled() && kvm_check_extension(kvm_state, KVM_CAP_PPC_HWRNG); > +} > > +int kvmppc_enable_hwrng(void) > +{ > return kvmppc_enable_hcall(kvm_state, H_RANDOM); > } > diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h > index 470f6d62f7bb..a76338c9aa16 100644 > --- a/target-ppc/kvm_ppc.h > +++ b/target-ppc/kvm_ppc.h > @@ -54,6 +54,7 @@ void kvmppc_hash64_free_pteg(uint64_t token); > void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong pte_index, > target_ulong pte0, target_ulong pte1); > bool kvmppc_has_cap_fixup_hcalls(void); > +bool kvmppc_hwrng_present(void); > int kvmppc_enable_hwrng(void); > > #else > @@ -252,6 +253,11 @@ static inline bool kvmppc_has_cap_fixup_hcalls(void) > abort(); > } > > +static inline bool kvmppc_hwrng_present(void) > +{ > + return false; > +} > + > static inline int kvmppc_enable_hwrng(void) > { > return -1; > >
On Mon, Sep 28, 2015 at 12:13:47PM +0200, Greg Kurz wrote: > A recent patch by Thomas Huth brought a new spapr-rng pseudo-device to > provide high-quality random numbers to guests. The device may either be > backed by a "RngBackend" or the in-kernel implementation of the H_RANDOM > hypercall. > > Since modern POWER8 based servers always provide a hardware rng, it makes > sense to create a spapr-rng device with use-kvm=true by default when it > is available. > > Of course we want the user to have full control on how the rng is handled. > The default device WILL NOT be created in the following cases: > - the -nodefaults option was passed > - a spapr-rng device was already passed on the command line > > The default device is created at reset time to ensure devices specified on > the command line have been created. > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> So, I think the concept is ok, but.. > --- > hw/ppc/spapr.c | 17 +++++++++++++++++ > hw/ppc/spapr_rng.c | 2 +- > target-ppc/kvm.c | 9 +++++---- > target-ppc/kvm_ppc.h | 6 ++++++ > 4 files changed, 29 insertions(+), 5 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 7f4f196e53e5..ee048ecffd0c 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1059,6 +1059,14 @@ static int spapr_check_htab_fd(sPAPRMachineState *spapr) > return rc; > } > > +static void spapr_rng_create(void) > +{ > + Object *rng = object_new(TYPE_SPAPR_RNG); > + > + object_property_set_bool(rng, true, "use-kvm", &error_abort); > + object_property_set_bool(rng, true, "realized", &error_abort); > +} > + > static void ppc_spapr_reset(void) > { > sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > @@ -1082,6 +1090,15 @@ static void ppc_spapr_reset(void) > spapr->rtas_addr = rtas_limit - RTAS_MAX_SIZE; > spapr->fdt_addr = spapr->rtas_addr - FDT_MAX_SIZE; > > + /* Create a rng device if the user did not provide it already and > + * KVM has hwrng support. > + */ > + if (defaults_enabled() && > + kvmppc_hwrng_present() && > + !object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)) { > + spapr_rng_create(); > + } > + Constructing the RNG at reset time is just wrong. Using defaults_enabled() is ugly at the best of times, using it at reset, after construction of the qom tree is generally complete, is just hideous.
On Tue, 29 Sep 2015 15:01:09 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Mon, Sep 28, 2015 at 12:13:47PM +0200, Greg Kurz wrote: > > A recent patch by Thomas Huth brought a new spapr-rng pseudo-device to > > provide high-quality random numbers to guests. The device may either be > > backed by a "RngBackend" or the in-kernel implementation of the H_RANDOM > > hypercall. > > > > Since modern POWER8 based servers always provide a hardware rng, it makes > > sense to create a spapr-rng device with use-kvm=true by default when it > > is available. > > > > Of course we want the user to have full control on how the rng is handled. > > The default device WILL NOT be created in the following cases: > > - the -nodefaults option was passed > > - a spapr-rng device was already passed on the command line > > > > The default device is created at reset time to ensure devices specified on > > the command line have been created. > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > So, I think the concept is ok, but.. > Just to be sure about the concept. The goal is to free users from having to explicitely pass -device spapr-rng,use-kvm=true ... when ALL the following conditions are met: 1) KVM is used and advertises KVM_CAP_PPC_HWRNG 2) -nodefaults HAS NOT been passed on the cmdline 3) -device spapr-rng HAS NOT been passed on the cmdline > > --- > > hw/ppc/spapr.c | 17 +++++++++++++++++ > > hw/ppc/spapr_rng.c | 2 +- > > target-ppc/kvm.c | 9 +++++---- > > target-ppc/kvm_ppc.h | 6 ++++++ > > 4 files changed, 29 insertions(+), 5 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 7f4f196e53e5..ee048ecffd0c 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -1059,6 +1059,14 @@ static int spapr_check_htab_fd(sPAPRMachineState *spapr) > > return rc; > > } > > > > +static void spapr_rng_create(void) > > +{ > > + Object *rng = object_new(TYPE_SPAPR_RNG); > > + > > + object_property_set_bool(rng, true, "use-kvm", &error_abort); > > + object_property_set_bool(rng, true, "realized", &error_abort); > > +} > > + > > static void ppc_spapr_reset(void) > > { > > sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > > @@ -1082,6 +1090,15 @@ static void ppc_spapr_reset(void) > > spapr->rtas_addr = rtas_limit - RTAS_MAX_SIZE; > > spapr->fdt_addr = spapr->rtas_addr - FDT_MAX_SIZE; > > > > + /* Create a rng device if the user did not provide it already and > > + * KVM has hwrng support. > > + */ > > + if (defaults_enabled() && > > + kvmppc_hwrng_present() && > > + !object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)) { > > + spapr_rng_create(); > > + } > > + > > Constructing the RNG at reset time is just wrong. Using > defaults_enabled() is ugly at the best of times, using it at reset, > after construction of the qom tree is generally complete, is just > hideous. > Yeah I ended up with this hack because I could not figure out how to give priority to a spapr-rng device specified on the cmdline over the automatic one... poor QOM skills :\ If you have a suggestion to handle this case in a more appropriate way, and it is worth the pain compared to the gain, please advice. Thanks. -- Greg
On 30/09/15 10:33, Greg Kurz wrote: > On Tue, 29 Sep 2015 15:01:09 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > >> On Mon, Sep 28, 2015 at 12:13:47PM +0200, Greg Kurz wrote: >>> A recent patch by Thomas Huth brought a new spapr-rng pseudo-device to >>> provide high-quality random numbers to guests. The device may either be >>> backed by a "RngBackend" or the in-kernel implementation of the H_RANDOM >>> hypercall. >>> >>> Since modern POWER8 based servers always provide a hardware rng, it makes >>> sense to create a spapr-rng device with use-kvm=true by default when it >>> is available. >>> >>> Of course we want the user to have full control on how the rng is handled. >>> The default device WILL NOT be created in the following cases: >>> - the -nodefaults option was passed >>> - a spapr-rng device was already passed on the command line >>> >>> The default device is created at reset time to ensure devices specified on >>> the command line have been created. >>> >>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> >> >> So, I think the concept is ok, but.. >> > > Just to be sure about the concept. > > The goal is to free users from having to explicitely pass > > -device spapr-rng,use-kvm=true > > ... when ALL the following conditions are met: > > 1) KVM is used and advertises KVM_CAP_PPC_HWRNG > 2) -nodefaults HAS NOT been passed on the cmdline > 3) -device spapr-rng HAS NOT been passed on the cmdline > >>> --- >>> hw/ppc/spapr.c | 17 +++++++++++++++++ >>> hw/ppc/spapr_rng.c | 2 +- >>> target-ppc/kvm.c | 9 +++++---- >>> target-ppc/kvm_ppc.h | 6 ++++++ >>> 4 files changed, 29 insertions(+), 5 deletions(-) >>> >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >>> index 7f4f196e53e5..ee048ecffd0c 100644 >>> --- a/hw/ppc/spapr.c >>> +++ b/hw/ppc/spapr.c >>> @@ -1059,6 +1059,14 @@ static int spapr_check_htab_fd(sPAPRMachineState *spapr) >>> return rc; >>> } >>> >>> +static void spapr_rng_create(void) >>> +{ >>> + Object *rng = object_new(TYPE_SPAPR_RNG); >>> + >>> + object_property_set_bool(rng, true, "use-kvm", &error_abort); >>> + object_property_set_bool(rng, true, "realized", &error_abort); >>> +} >>> + >>> static void ppc_spapr_reset(void) >>> { >>> sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); >>> @@ -1082,6 +1090,15 @@ static void ppc_spapr_reset(void) >>> spapr->rtas_addr = rtas_limit - RTAS_MAX_SIZE; >>> spapr->fdt_addr = spapr->rtas_addr - FDT_MAX_SIZE; >>> >>> + /* Create a rng device if the user did not provide it already and >>> + * KVM has hwrng support. >>> + */ >>> + if (defaults_enabled() && >>> + kvmppc_hwrng_present() && >>> + !object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)) { >>> + spapr_rng_create(); >>> + } >>> + >> >> Constructing the RNG at reset time is just wrong. Using >> defaults_enabled() is ugly at the best of times, using it at reset, >> after construction of the qom tree is generally complete, is just >> hideous. >> > > Yeah I ended up with this hack because I could not figure out how > to give priority to a spapr-rng device specified on the cmdline > over the automatic one... poor QOM skills :\ > > If you have a suggestion to handle this case in a more appropriate way, > and it is worth the pain compared to the gain, please advice. Not sure whether this might be an acceptable solution, but maybe you could use qemu_opts_foreach(qemu_find_opts("device"), ...) to check whether a "spapr-rng" device has been specified at the command line? Thomas
On Wed, 30 Sep 2015 11:10:52 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 30/09/15 10:33, Greg Kurz wrote: > > On Tue, 29 Sep 2015 15:01:09 +1000 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > >> On Mon, Sep 28, 2015 at 12:13:47PM +0200, Greg Kurz wrote: > >>> A recent patch by Thomas Huth brought a new spapr-rng pseudo-device to > >>> provide high-quality random numbers to guests. The device may either be > >>> backed by a "RngBackend" or the in-kernel implementation of the H_RANDOM > >>> hypercall. > >>> > >>> Since modern POWER8 based servers always provide a hardware rng, it makes > >>> sense to create a spapr-rng device with use-kvm=true by default when it > >>> is available. > >>> > >>> Of course we want the user to have full control on how the rng is handled. > >>> The default device WILL NOT be created in the following cases: > >>> - the -nodefaults option was passed > >>> - a spapr-rng device was already passed on the command line > >>> > >>> The default device is created at reset time to ensure devices specified on > >>> the command line have been created. > >>> > >>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > >> > >> So, I think the concept is ok, but.. > >> > > > > Just to be sure about the concept. > > > > The goal is to free users from having to explicitely pass > > > > -device spapr-rng,use-kvm=true > > > > ... when ALL the following conditions are met: > > > > 1) KVM is used and advertises KVM_CAP_PPC_HWRNG > > 2) -nodefaults HAS NOT been passed on the cmdline > > 3) -device spapr-rng HAS NOT been passed on the cmdline > > > >>> --- > >>> hw/ppc/spapr.c | 17 +++++++++++++++++ > >>> hw/ppc/spapr_rng.c | 2 +- > >>> target-ppc/kvm.c | 9 +++++---- > >>> target-ppc/kvm_ppc.h | 6 ++++++ > >>> 4 files changed, 29 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > >>> index 7f4f196e53e5..ee048ecffd0c 100644 > >>> --- a/hw/ppc/spapr.c > >>> +++ b/hw/ppc/spapr.c > >>> @@ -1059,6 +1059,14 @@ static int spapr_check_htab_fd(sPAPRMachineState *spapr) > >>> return rc; > >>> } > >>> > >>> +static void spapr_rng_create(void) > >>> +{ > >>> + Object *rng = object_new(TYPE_SPAPR_RNG); > >>> + > >>> + object_property_set_bool(rng, true, "use-kvm", &error_abort); > >>> + object_property_set_bool(rng, true, "realized", &error_abort); > >>> +} > >>> + > >>> static void ppc_spapr_reset(void) > >>> { > >>> sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > >>> @@ -1082,6 +1090,15 @@ static void ppc_spapr_reset(void) > >>> spapr->rtas_addr = rtas_limit - RTAS_MAX_SIZE; > >>> spapr->fdt_addr = spapr->rtas_addr - FDT_MAX_SIZE; > >>> > >>> + /* Create a rng device if the user did not provide it already and > >>> + * KVM has hwrng support. > >>> + */ > >>> + if (defaults_enabled() && > >>> + kvmppc_hwrng_present() && > >>> + !object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)) { > >>> + spapr_rng_create(); > >>> + } > >>> + > >> > >> Constructing the RNG at reset time is just wrong. Using > >> defaults_enabled() is ugly at the best of times, using it at reset, > >> after construction of the qom tree is generally complete, is just > >> hideous. > >> > > > > Yeah I ended up with this hack because I could not figure out how > > to give priority to a spapr-rng device specified on the cmdline > > over the automatic one... poor QOM skills :\ > > > > If you have a suggestion to handle this case in a more appropriate way, > > and it is worth the pain compared to the gain, please advice. > > Not sure whether this might be an acceptable solution, but maybe you > could use qemu_opts_foreach(qemu_find_opts("device"), ...) to check > whether a "spapr-rng" device has been specified at the command line? > Yes it would allow, at least, to create the device at init time... then I don't know if it is good practice, considering that: $ grep -r qemu_opts_foreach hw/ hw/core/qdev-properties-system.c: qemu_opts_foreach(qemu_find_opts("global"), $ Cheers. -- Greg
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 7f4f196e53e5..ee048ecffd0c 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1059,6 +1059,14 @@ static int spapr_check_htab_fd(sPAPRMachineState *spapr) return rc; } +static void spapr_rng_create(void) +{ + Object *rng = object_new(TYPE_SPAPR_RNG); + + object_property_set_bool(rng, true, "use-kvm", &error_abort); + object_property_set_bool(rng, true, "realized", &error_abort); +} + static void ppc_spapr_reset(void) { sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); @@ -1082,6 +1090,15 @@ static void ppc_spapr_reset(void) spapr->rtas_addr = rtas_limit - RTAS_MAX_SIZE; spapr->fdt_addr = spapr->rtas_addr - FDT_MAX_SIZE; + /* Create a rng device if the user did not provide it already and + * KVM has hwrng support. + */ + if (defaults_enabled() && + kvmppc_hwrng_present() && + !object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)) { + spapr_rng_create(); + } + /* Load the fdt */ spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr, spapr->rtas_size); diff --git a/hw/ppc/spapr_rng.c b/hw/ppc/spapr_rng.c index ed43d5e04221..ee5af302bd4d 100644 --- a/hw/ppc/spapr_rng.c +++ b/hw/ppc/spapr_rng.c @@ -114,7 +114,7 @@ static void spapr_rng_realize(DeviceState *dev, Error **errp) sPAPRRngState *rngstate = SPAPR_RNG(dev); if (rngstate->use_kvm) { - if (kvmppc_enable_hwrng() == 0) { + if (kvmppc_hwrng_present() && kvmppc_enable_hwrng() == 0) { return; } /* diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index e641680fb146..084bb034f1fd 100644 --- a/target-ppc/kvm.c +++ b/target-ppc/kvm.c @@ -2490,11 +2490,12 @@ int kvm_arch_msi_data_to_gsi(uint32_t data) return data & 0xffff; } -int kvmppc_enable_hwrng(void) +bool kvmppc_hwrng_present(void) { - if (!kvm_enabled() || !kvm_check_extension(kvm_state, KVM_CAP_PPC_HWRNG)) { - return -1; - } + return kvm_enabled() && kvm_check_extension(kvm_state, KVM_CAP_PPC_HWRNG); +} +int kvmppc_enable_hwrng(void) +{ return kvmppc_enable_hcall(kvm_state, H_RANDOM); } diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h index 470f6d62f7bb..a76338c9aa16 100644 --- a/target-ppc/kvm_ppc.h +++ b/target-ppc/kvm_ppc.h @@ -54,6 +54,7 @@ void kvmppc_hash64_free_pteg(uint64_t token); void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong pte_index, target_ulong pte0, target_ulong pte1); bool kvmppc_has_cap_fixup_hcalls(void); +bool kvmppc_hwrng_present(void); int kvmppc_enable_hwrng(void); #else @@ -252,6 +253,11 @@ static inline bool kvmppc_has_cap_fixup_hcalls(void) abort(); } +static inline bool kvmppc_hwrng_present(void) +{ + return false; +} + static inline int kvmppc_enable_hwrng(void) { return -1;
A recent patch by Thomas Huth brought a new spapr-rng pseudo-device to provide high-quality random numbers to guests. The device may either be backed by a "RngBackend" or the in-kernel implementation of the H_RANDOM hypercall. Since modern POWER8 based servers always provide a hardware rng, it makes sense to create a spapr-rng device with use-kvm=true by default when it is available. Of course we want the user to have full control on how the rng is handled. The default device WILL NOT be created in the following cases: - the -nodefaults option was passed - a spapr-rng device was already passed on the command line The default device is created at reset time to ensure devices specified on the command line have been created. Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> --- hw/ppc/spapr.c | 17 +++++++++++++++++ hw/ppc/spapr_rng.c | 2 +- target-ppc/kvm.c | 9 +++++---- target-ppc/kvm_ppc.h | 6 ++++++ 4 files changed, 29 insertions(+), 5 deletions(-)