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

[Greg Kurz]

On Tue, 29 Sep 2015 15:01:09 +1000
David Gibson <address@hidden> 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 <address@hidden>
> 
> 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

pgpt27fMvYgZL.pgp
Description: OpenPGP digital signature