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

[Greg Kurz]

On Wed, 30 Sep 2015 11:10:52 +0200
Thomas Huth <address@hidden> wrote:

> On 30/09/15 10:33, Greg Kurz wrote:
> > 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.
> 
> 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

pgpV0iTvfPZZj.pgp
Description: OpenPGP digital signature