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

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

[Laurent Vivier]

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. 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}
 
-- 
2.20.1

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

[Daniel P. Berrangé]

On Fri, May 10, 2019 at 12:26:37PM +0200, Laurent Vivier wrote:
> 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?)

Yeah, I think we could change the default backend, as it won't affect
migration in any way

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

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Markus Armbruster]

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.

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

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?

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

[Laurent Vivier]

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!

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

Thanks,
Laurent

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

[Markus Armbruster]

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?

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

[Daniel P. Berrangé]

On Fri, May 10, 2019 at 02:37:41PM +0200, Laurent Vivier wrote:
> On 10/05/2019 14:27, Markus Armbruster wrote:
> > Laurent Vivier <lvivier@redhat.com> writes:
> > 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...

Are you sure about that ?

The EGD impl looks like it is requesting entropy in an async manner.

Any problem with rng-random would also affect rng-builtin, as depending
on platform / build options,  rng-builtin may just use /dev/urandom
directly.  It should only block with /dev/random really and that's only
with Linux's impl of /dev/random - some OS effectively have /dev/random
behave identically to /dev/urandom.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Laurent Vivier]

On 10/05/2019 17:32, Daniel P. Berrangé wrote:
> On Fri, May 10, 2019 at 02:37:41PM +0200, Laurent Vivier wrote:
>> On 10/05/2019 14:27, Markus Armbruster wrote:
>>> Laurent Vivier <lvivier@redhat.com> writes:
>>> 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...
> 
> Are you sure about that ?
> 
> The EGD impl looks like it is requesting entropy in an async manner.

The virtio-rng driver waits until it receives enough entropy from the 
RNG backend while a mutex is taken.

If the EGD daemon doesn't provide enough data to the RNG backend, 
virtio-rng driver can hang.

It's easy to have if we start EGD backend with a socket in server,nowait 
mode and no EGD daemon connects to the port.

Thanks,
Laurent

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

[Amit Shah]

On (Fri) 10 May 2019 [17:19:12], Markus Armbruster wrote:
> 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?

The egd backend wasn't too useful - so I don't mind deprecating it
using the usual deprecation notice.

The rng-random backend can stay with the multiple fallbacks as
discussed in the other threads - with getrandom() being the most
preferred one on Linux.  BSDs have /dev/srandom which is quite similar
to getrandom(), but that can be an additional backend that can come later.

Overall, I like the way these series turned out..


		Amit
-- 
http://amitshah.net/