On Wed, 30 Sep 2015 11:10:52 +0200 Thomas Huth <th...@redhat.com> wrote:
> On 30/09/15 10:33, Greg Kurz wrote: > > On Tue, 29 Sep 2015 15:01:09 +1000 > > David Gibson <da...@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 <gk...@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
pgp6NHLN_PudM.pgp
Description: OpenPGP digital signature