Re: [Qemu-devel] [PATCH v2] rng-builtin: add an RNG backend that uses qemu_guest_getrandom()

[Markus Armbruster <armbru@redhat.com>]

Laurent Vivier <lvivier@redhat.com> writes:

> On 10/05/2019 14:27, Markus Armbruster wrote:
>> Laurent Vivier <lvivier@redhat.com> writes:
>>
>>> Add a new RNG backend using QEMU builtin getrandom function.
>>>
>>> It can be created and used with something like:
>>>
>>>      ... -object rng-builtin,id=rng0 -device virtio-rng,rng=rng0 ...
>>>
>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>> ---
>>>
>>> Notes:
>>>      This patch applies on top of
>>>      "[PATCH v5 00/24] Add qemu_getrandom and ARMv8.5-RNG etc"
>>>      Based-on: 20190510012458.22706-1-richard.henderson@linaro.org
>>>           v2: Update qemu-options.hx
>>>          describe the new backend and specify virtio-rng uses the
>>>          rng-random by default (do we want to change this?)
>>>
>>>   backends/Makefile.objs |  2 +-
>>>   backends/rng-builtin.c | 56 ++++++++++++++++++++++++++++++++++++++++++
>>>   qemu-options.hx        | 10 +++++++-
>>>   3 files changed, 66 insertions(+), 2 deletions(-)
>>>   create mode 100644 backends/rng-builtin.c
>>>
>>> diff --git a/backends/Makefile.objs b/backends/Makefile.objs
>>> index ff619d31b461..8da4a508d97b 100644
>>> --- a/backends/Makefile.objs
>>> +++ b/backends/Makefile.objs
>>> @@ -1,4 +1,4 @@
>>> -common-obj-y += rng.o rng-egd.o
>>> +common-obj-y += rng.o rng-egd.o rng-builtin.o
>>>   common-obj-$(CONFIG_POSIX) += rng-random.o
>>>     common-obj-$(CONFIG_TPM) += tpm.o
>>> diff --git a/backends/rng-builtin.c b/backends/rng-builtin.c
>>> new file mode 100644
>>> index 000000000000..b1264b745407
>>> --- /dev/null
>>> +++ b/backends/rng-builtin.c
>>> @@ -0,0 +1,56 @@
>>> +/*
>>> + * QEMU Builtin Random Number Generator Backend
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>>> + * See the COPYING file in the top-level directory.
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "sysemu/rng.h"
>>> +#include "qapi/error.h"
>>> +#include "qapi/qmp/qerror.h"
>>> +#include "qemu/main-loop.h"
>>> +#include "qemu/guest-random.h"
>>> +
>>> +#define TYPE_RNG_BUILTIN "rng-builtin"
>>> +#define RNG_BUILTIN(obj) OBJECT_CHECK(RngBuiltin, (obj), TYPE_RNG_BUILTIN)
>>> +
>>> +typedef struct RngBuiltin {
>>> +    RngBackend parent;
>>> +} RngBuiltin;
>>> +
>>> +static void rng_builtin_request_entropy(RngBackend *b, RngRequest *req)
>>> +{
>>> +    RngBuiltin *s = RNG_BUILTIN(b);
>>> +
>>> +    while (!QSIMPLEQ_EMPTY(&s->parent.requests)) {
>>> +        RngRequest *req = QSIMPLEQ_FIRST(&s->parent.requests);
>>> +
>>> +        qemu_guest_getrandom_nofail(req->data, req->size);
>>> +
>>> +        req->receive_entropy(req->opaque, req->data, req->size);
>>> +
>>> +        rng_backend_finalize_request(&s->parent, req);
>>> +    }
>>> +}
>>> +
>>> +static void rng_builtin_class_init(ObjectClass *klass, void *data)
>>> +{
>>> +    RngBackendClass *rbc = RNG_BACKEND_CLASS(klass);
>>> +
>>> +    rbc->request_entropy = rng_builtin_request_entropy;
>>> +}
>>> +
>>> +static const TypeInfo rng_builtin_info = {
>>> +    .name = TYPE_RNG_BUILTIN,
>>> +    .parent = TYPE_RNG_BACKEND,
>>> +    .instance_size = sizeof(RngBuiltin),
>>> +    .class_init = rng_builtin_class_init,
>>> +};
>>> +
>>> +static void register_types(void)
>>> +{
>>> +    type_register_static(&rng_builtin_info);
>>> +}
>>> +
>>> +type_init(register_types);
>>> diff --git a/qemu-options.hx b/qemu-options.hx
>>> index 0191ef8b1eb7..3e2a51c691b0 100644
>>> --- a/qemu-options.hx
>>> +++ b/qemu-options.hx
>>> @@ -4280,13 +4280,21 @@ other options.
>>>     The @option{share} boolean option is @var{on} by default with
>>> memfd.
>>>   +@item -object rng-builtin,id=@var{id}
>>> +
>>> +Creates a random number generator backend which obtains entropy from
>>> +QEMU builtin functions. The @option{id} parameter is a unique ID that
>>> +will be used to reference this entropy backend from the @option{virtio-rng}
>>> +device.
>>> +
>>>   @item -object rng-random,id=@var{id},filename=@var{/dev/random}
>>>     Creates a random number generator backend which obtains entropy
>>> from
>>>   a device on the host. The @option{id} parameter is a unique ID that
>>>   will be used to reference this entropy backend from the @option{virtio-rng}
>>>   device.
>>
>> There's also the "spapr-rng" device, I think.
>
> spapr-rng doesn't have default. You must specify one to be able to use it:
>    qemu-system-ppc64: -device spapr-rng: spapr-rng needs an RNG backend!

You're right.

>>>           The @option{filename} parameter specifies which file to obtain
>>> -entropy from and if omitted defaults to @option{/dev/random}.
>>> +entropy from and if omitted defaults to @option{/dev/random}. By default,
>>> +the @option{virtio-rng} device uses this RNG backend.
>>>     @item -object rng-egd,id=@var{id},chardev=@var{chardevid}
>>
>> Trivial conflict with Kashyap's "[PATCH v2] VirtIO-RNG: Update default
>> entropy source to `/dev/urandom`".
>>
>> virtio-rng indeed creates an rng-random backend when the user doesn't
>> specify one.  I consider having device model frontends create backends a
>> bad idea.  Not this patch's fault, of course.
>>
>> That said, would rng-builtin be a better default?  For starters, it's
>> available when !CONFIG_POSIX.  I suspect virtio-rng crashes when it
>> tries to create an rng-random that isn't available.
>
> I will send a v3 with rng-builtin as a default. Maintainer will be
> able to pick one of his choice, v2 or v3.
>
>>
>> The new rng-builtin is considerably simpler than both rng-random and
>> rng-egd.  Moreover, it just works, whereas rng-random is limited to
>> CONFIG_POSIX, and rng-egd needs egd running (which I suspect basically
>> nobody does).  Have we considered deprecating these two backends in
>> favor of rng-builtin?
>
> I have several bugzilla involving these backends: as there are
> blocking, the virtio-rng device in the guest can hang, or crash during
> hot-unplug. From my point of view, life would be easier without
> them...

Sounds like perfectly fine reasons for deprecating them.  Amit, what do
you think?