Re: [Qemu-devel] [PATCH] spapr: add a default rng device

[David Gibson]

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 <address@hidden>

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.

-- 
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

pgpcTpkVSyO7s.pgp
Description: PGP signature