mbox series

[v7,0/4] rng-builtin: add an RNG backend that uses qemu_guest_getrandom()

Message ID 20190529143106.11789-1-lvivier@redhat.com
Headers show
Series rng-builtin: add an RNG backend that uses qemu_guest_getrandom() | expand

Message

Laurent Vivier May 29, 2019, 2:31 p.m. UTC
Add a new RNG backend using QEMU builtin getrandom function.

v7: rebase on master
    Make rng-builtin asynchronous with QEMUBH (removed existing R-b)

v6: remove "sysemu/rng-random.h" from virtio-rng.c
    rebase on qemu_getrandom v8

v5: PATCH 1 s/linux/Linux/
    remove superfluous includes from rng-builtin.c
    don't update rng-random documentation
    add a patch from Markus to keep the default backend out of VirtIORNGConf
    move TYPE_RNG_BUILTIN to sysemu/rng.h and remove sysemu/rng-builtin.h

v4: update PATCH 1 commit message

v3: Include Kashyap's patch in the series
    Add a patch to change virtio-rng default backend to rng-builtin

v2: Update qemu-options.hx
    describe the new backend and specify virtio-rng uses the
    rng-random by default

Kashyap Chamarthy (1):
  VirtIO-RNG: Update default entropy source to `/dev/urandom`

Laurent Vivier (2):
  rng-builtin: add an RNG backend that uses qemu_guest_getrandom()
  virtio-rng: change default backend to rng-builtin

Markus Armbruster (1):
  virtio-rng: Keep the default backend out of VirtIORNGConf

 backends/Makefile.objs         |  2 +-
 backends/rng-builtin.c         | 77 ++++++++++++++++++++++++++++++++++
 backends/rng-random.c          |  2 +-
 hw/virtio/virtio-rng.c         | 19 ++++-----
 include/hw/virtio/virtio-rng.h |  2 -
 include/sysemu/rng.h           |  2 +
 qemu-options.hx                |  9 +++-
 7 files changed, 97 insertions(+), 16 deletions(-)
 create mode 100644 backends/rng-builtin.c

Comments

Markus Armbruster June 5, 2019, 1:05 p.m. UTC | #1
Laurent Vivier <lvivier@redhat.com> writes:

> Add a new RNG backend using QEMU builtin getrandom function.
>
> v7: rebase on master
>     Make rng-builtin asynchronous with QEMUBH (removed existing R-b)

Pardon the ignorant question: why is that necessary?
Laurent Vivier June 5, 2019, 1:58 p.m. UTC | #2
On 05/06/2019 15:05, Markus Armbruster wrote:
> Laurent Vivier <lvivier@redhat.com> writes:
> 
>> Add a new RNG backend using QEMU builtin getrandom function.
>>
>> v7: rebase on master
>>     Make rng-builtin asynchronous with QEMUBH (removed existing R-b)
> 
> Pardon the ignorant question: why is that necessary?
> 

Because request_entropy() function is called while the request is not in
the requests queue, so the loop on !QSIMPLEQ_EMPTY(&s->parent.requests)
doens't process it. The request is added just after the call.

thanks,
Laurent
Markus Armbruster June 5, 2019, 5:56 p.m. UTC | #3
Laurent Vivier <lvivier@redhat.com> writes:

> On 05/06/2019 15:05, Markus Armbruster wrote:
>> Laurent Vivier <lvivier@redhat.com> writes:
>> 
>>> Add a new RNG backend using QEMU builtin getrandom function.
>>>
>>> v7: rebase on master
>>>     Make rng-builtin asynchronous with QEMUBH (removed existing R-b)
>> 
>> Pardon the ignorant question: why is that necessary?
>> 
>
> Because request_entropy() function is called while the request is not in
> the requests queue, so the loop on !QSIMPLEQ_EMPTY(&s->parent.requests)
> doens't process it. The request is added just after the call.

In rng_backend_request_entropy().  I see.  Any particular reason for
this order?  "I don't know" is an acceptable answer :)
Laurent Vivier June 5, 2019, 6:36 p.m. UTC | #4
On 05/06/2019 19:56, Markus Armbruster wrote:
> Laurent Vivier <lvivier@redhat.com> writes:
> 
>> On 05/06/2019 15:05, Markus Armbruster wrote:
>>> Laurent Vivier <lvivier@redhat.com> writes:
>>>
>>>> Add a new RNG backend using QEMU builtin getrandom function.
>>>>
>>>> v7: rebase on master
>>>>     Make rng-builtin asynchronous with QEMUBH (removed existing R-b)
>>>
>>> Pardon the ignorant question: why is that necessary?
>>>
>>
>> Because request_entropy() function is called while the request is not in
>> the requests queue, so the loop on !QSIMPLEQ_EMPTY(&s->parent.requests)
>> doens't process it. The request is added just after the call.
> 
> In rng_backend_request_entropy().  I see.  Any particular reason for
> this order?  "I don't know" is an acceptable answer :)
> 

Yes...

and there is a reason:

in rng_random_request_entropy(), QSIMPLEQ_EMPTY() is used to know if we
have to register an fd handler with qemu_set_fd_handler().

For me, it seemed easier to use QEMUBH rather than to change the
existing algorithm, as the backend has been thought to be asynchronous.

Thanks,
Laurent
Markus Armbruster June 6, 2019, 4:45 a.m. UTC | #5
Laurent Vivier <lvivier@redhat.com> writes:

> On 05/06/2019 19:56, Markus Armbruster wrote:
>> Laurent Vivier <lvivier@redhat.com> writes:
>> 
>>> On 05/06/2019 15:05, Markus Armbruster wrote:
>>>> Laurent Vivier <lvivier@redhat.com> writes:
>>>>
>>>>> Add a new RNG backend using QEMU builtin getrandom function.
>>>>>
>>>>> v7: rebase on master
>>>>>     Make rng-builtin asynchronous with QEMUBH (removed existing R-b)
>>>>
>>>> Pardon the ignorant question: why is that necessary?
>>>>
>>>
>>> Because request_entropy() function is called while the request is not in
>>> the requests queue, so the loop on !QSIMPLEQ_EMPTY(&s->parent.requests)
>>> doens't process it. The request is added just after the call.
>> 
>> In rng_backend_request_entropy().  I see.  Any particular reason for
>> this order?  "I don't know" is an acceptable answer :)
>> 
>
> Yes...
>
> and there is a reason:
>
> in rng_random_request_entropy(), QSIMPLEQ_EMPTY() is used to know if we
> have to register an fd handler with qemu_set_fd_handler().
>
> For me, it seemed easier to use QEMUBH rather than to change the
> existing algorithm, as the backend has been thought to be asynchronous.

In your shoes, I'd be tempted to explore whether changing the order
simplifies things overall.  I'm not asking you to do that; your patch is
okay as is.

Thanks!
Laurent Vivier June 11, 2019, 8:42 a.m. UTC | #6
Michael,

Could you pick this series in the next virtio pull request?

If you disagree with some of my patches, could you take at least the
first one (from Kashyap)?

Thanks,
Laurent

On 29/05/2019 16:31, Laurent Vivier wrote:
> Add a new RNG backend using QEMU builtin getrandom function.
> 
> v7: rebase on master
>     Make rng-builtin asynchronous with QEMUBH (removed existing R-b)
> 
> v6: remove "sysemu/rng-random.h" from virtio-rng.c
>     rebase on qemu_getrandom v8
> 
> v5: PATCH 1 s/linux/Linux/
>     remove superfluous includes from rng-builtin.c
>     don't update rng-random documentation
>     add a patch from Markus to keep the default backend out of VirtIORNGConf
>     move TYPE_RNG_BUILTIN to sysemu/rng.h and remove sysemu/rng-builtin.h
> 
> v4: update PATCH 1 commit message
> 
> v3: Include Kashyap's patch in the series
>     Add a patch to change virtio-rng default backend to rng-builtin
> 
> v2: Update qemu-options.hx
>     describe the new backend and specify virtio-rng uses the
>     rng-random by default
> 
> Kashyap Chamarthy (1):
>   VirtIO-RNG: Update default entropy source to `/dev/urandom`
> 
> Laurent Vivier (2):
>   rng-builtin: add an RNG backend that uses qemu_guest_getrandom()
>   virtio-rng: change default backend to rng-builtin
> 
> Markus Armbruster (1):
>   virtio-rng: Keep the default backend out of VirtIORNGConf
> 
>  backends/Makefile.objs         |  2 +-
>  backends/rng-builtin.c         | 77 ++++++++++++++++++++++++++++++++++
>  backends/rng-random.c          |  2 +-
>  hw/virtio/virtio-rng.c         | 19 ++++-----
>  include/hw/virtio/virtio-rng.h |  2 -
>  include/sysemu/rng.h           |  2 +
>  qemu-options.hx                |  9 +++-
>  7 files changed, 97 insertions(+), 16 deletions(-)
>  create mode 100644 backends/rng-builtin.c
>
Laurent Vivier June 17, 2019, 12:09 p.m. UTC | #7
On 11/06/2019 10:42, Laurent Vivier wrote:
> Michael,
> 
> Could you pick this series in the next virtio pull request?

Or perhaps Amit?

Thanks,
Laurent

> 
> If you disagree with some of my patches, could you take at least the
> first one (from Kashyap)?
> 
> Thanks,
> Laurent
> 
> On 29/05/2019 16:31, Laurent Vivier wrote:
>> Add a new RNG backend using QEMU builtin getrandom function.
>>
>> v7: rebase on master
>>     Make rng-builtin asynchronous with QEMUBH (removed existing R-b)
>>
>> v6: remove "sysemu/rng-random.h" from virtio-rng.c
>>     rebase on qemu_getrandom v8
>>
>> v5: PATCH 1 s/linux/Linux/
>>     remove superfluous includes from rng-builtin.c
>>     don't update rng-random documentation
>>     add a patch from Markus to keep the default backend out of VirtIORNGConf
>>     move TYPE_RNG_BUILTIN to sysemu/rng.h and remove sysemu/rng-builtin.h
>>
>> v4: update PATCH 1 commit message
>>
>> v3: Include Kashyap's patch in the series
>>     Add a patch to change virtio-rng default backend to rng-builtin
>>
>> v2: Update qemu-options.hx
>>     describe the new backend and specify virtio-rng uses the
>>     rng-random by default
>>
>> Kashyap Chamarthy (1):
>>   VirtIO-RNG: Update default entropy source to `/dev/urandom`
>>
>> Laurent Vivier (2):
>>   rng-builtin: add an RNG backend that uses qemu_guest_getrandom()
>>   virtio-rng: change default backend to rng-builtin
>>
>> Markus Armbruster (1):
>>   virtio-rng: Keep the default backend out of VirtIORNGConf
>>
>>  backends/Makefile.objs         |  2 +-
>>  backends/rng-builtin.c         | 77 ++++++++++++++++++++++++++++++++++
>>  backends/rng-random.c          |  2 +-
>>  hw/virtio/virtio-rng.c         | 19 ++++-----
>>  include/hw/virtio/virtio-rng.h |  2 -
>>  include/sysemu/rng.h           |  2 +
>>  qemu-options.hx                |  9 +++-
>>  7 files changed, 97 insertions(+), 16 deletions(-)
>>  create mode 100644 backends/rng-builtin.c
>>
> 
>
Michael S. Tsirkin July 2, 2019, 1:21 p.m. UTC | #8
On Wed, May 29, 2019 at 04:31:02PM +0200, Laurent Vivier wrote:
> Add a new RNG backend using QEMU builtin getrandom function.
> 
> v7: rebase on master
>     Make rng-builtin asynchronous with QEMUBH (removed existing R-b)
> 
> v6: remove "sysemu/rng-random.h" from virtio-rng.c
>     rebase on qemu_getrandom v8
> 
> v5: PATCH 1 s/linux/Linux/
>     remove superfluous includes from rng-builtin.c
>     don't update rng-random documentation
>     add a patch from Markus to keep the default backend out of VirtIORNGConf
>     move TYPE_RNG_BUILTIN to sysemu/rng.h and remove sysemu/rng-builtin.h
> 
> v4: update PATCH 1 commit message
> 
> v3: Include Kashyap's patch in the series
>     Add a patch to change virtio-rng default backend to rng-builtin
> 
> v2: Update qemu-options.hx
>     describe the new backend and specify virtio-rng uses the
>     rng-random by default


Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

feel free to merge.

> Kashyap Chamarthy (1):
>   VirtIO-RNG: Update default entropy source to `/dev/urandom`
> 
> Laurent Vivier (2):
>   rng-builtin: add an RNG backend that uses qemu_guest_getrandom()
>   virtio-rng: change default backend to rng-builtin
> 
> Markus Armbruster (1):
>   virtio-rng: Keep the default backend out of VirtIORNGConf
> 
>  backends/Makefile.objs         |  2 +-
>  backends/rng-builtin.c         | 77 ++++++++++++++++++++++++++++++++++
>  backends/rng-random.c          |  2 +-
>  hw/virtio/virtio-rng.c         | 19 ++++-----
>  include/hw/virtio/virtio-rng.h |  2 -
>  include/sysemu/rng.h           |  2 +
>  qemu-options.hx                |  9 +++-
>  7 files changed, 97 insertions(+), 16 deletions(-)
>  create mode 100644 backends/rng-builtin.c
> 
> -- 
> 2.20.1
Laurent Vivier July 2, 2019, 4:48 p.m. UTC | #9
On 02/07/2019 15:21, Michael S. Tsirkin wrote:
> On Wed, May 29, 2019 at 04:31:02PM +0200, Laurent Vivier wrote:
>> Add a new RNG backend using QEMU builtin getrandom function.
>>
>> v7: rebase on master
>>     Make rng-builtin asynchronous with QEMUBH (removed existing R-b)
>>
>> v6: remove "sysemu/rng-random.h" from virtio-rng.c
>>     rebase on qemu_getrandom v8
>>
>> v5: PATCH 1 s/linux/Linux/
>>     remove superfluous includes from rng-builtin.c
>>     don't update rng-random documentation
>>     add a patch from Markus to keep the default backend out of VirtIORNGConf
>>     move TYPE_RNG_BUILTIN to sysemu/rng.h and remove sysemu/rng-builtin.h
>>
>> v4: update PATCH 1 commit message
>>
>> v3: Include Kashyap's patch in the series
>>     Add a patch to change virtio-rng default backend to rng-builtin
>>
>> v2: Update qemu-options.hx
>>     describe the new backend and specify virtio-rng uses the
>>     rng-random by default
> 
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> 
> feel free to merge.

Thank you Michael.

I've already included PATCH 1 in a pull-request for trivial patches branch.

I'm not sure the other patches are good candidates for trivial patches
branch, but is there any other maintainer that can include them in a
pull request (before the freeze)?

Amit?
[Do you want I manage a virtio-rng pull-request for you?]

Thanks,
Laurent
Amit Shah July 3, 2019, 2:23 p.m. UTC | #10
On Tue, 2019-07-02 at 18:48 +0200, Laurent Vivier wrote:
> On 02/07/2019 15:21, Michael S. Tsirkin wrote:
> > On Wed, May 29, 2019 at 04:31:02PM +0200, Laurent Vivier wrote:
> > > Add a new RNG backend using QEMU builtin getrandom function.
> > > 
> > > v7: rebase on master
> > >     Make rng-builtin asynchronous with QEMUBH (removed existing
> > > R-b)
> > > 
> > > v6: remove "sysemu/rng-random.h" from virtio-rng.c
> > >     rebase on qemu_getrandom v8
> > > 
> > > v5: PATCH 1 s/linux/Linux/
> > >     remove superfluous includes from rng-builtin.c
> > >     don't update rng-random documentation
> > >     add a patch from Markus to keep the default backend out of
> > > VirtIORNGConf
> > >     move TYPE_RNG_BUILTIN to sysemu/rng.h and remove sysemu/rng-
> > > builtin.h
> > > 
> > > v4: update PATCH 1 commit message
> > > 
> > > v3: Include Kashyap's patch in the series
> > >     Add a patch to change virtio-rng default backend to rng-
> > > builtin
> > > 
> > > v2: Update qemu-options.hx
> > >     describe the new backend and specify virtio-rng uses the
> > >     rng-random by default
> > 
> > 
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > feel free to merge.
> 
> Thank you Michael.
> 
> I've already included PATCH 1 in a pull-request for trivial patches
> branch.
> 
> I'm not sure the other patches are good candidates for trivial
> patches
> branch, but is there any other maintainer that can include them in a
> pull request (before the freeze)?
> 
> Amit?
> [Do you want I manage a virtio-rng pull-request for you?]

Hello Laurent,

Apologies as I haven't been around for a bit.

I don't mind you doing the pull req yourself if you have sufficient
reviews.  Do you also want to consider maintaining rng yourself?

Thanks,
Laurent Vivier July 3, 2019, 2:57 p.m. UTC | #11
On 03/07/2019 16:23, Amit Shah wrote:
> On Tue, 2019-07-02 at 18:48 +0200, Laurent Vivier wrote:
>> On 02/07/2019 15:21, Michael S. Tsirkin wrote:
>>> On Wed, May 29, 2019 at 04:31:02PM +0200, Laurent Vivier wrote:
>>>> Add a new RNG backend using QEMU builtin getrandom function.
>>>>
>>>> v7: rebase on master
>>>>     Make rng-builtin asynchronous with QEMUBH (removed existing
>>>> R-b)
>>>>
>>>> v6: remove "sysemu/rng-random.h" from virtio-rng.c
>>>>     rebase on qemu_getrandom v8
>>>>
>>>> v5: PATCH 1 s/linux/Linux/
>>>>     remove superfluous includes from rng-builtin.c
>>>>     don't update rng-random documentation
>>>>     add a patch from Markus to keep the default backend out of
>>>> VirtIORNGConf
>>>>     move TYPE_RNG_BUILTIN to sysemu/rng.h and remove sysemu/rng-
>>>> builtin.h
>>>>
>>>> v4: update PATCH 1 commit message
>>>>
>>>> v3: Include Kashyap's patch in the series
>>>>     Add a patch to change virtio-rng default backend to rng-
>>>> builtin
>>>>
>>>> v2: Update qemu-options.hx
>>>>     describe the new backend and specify virtio-rng uses the
>>>>     rng-random by default
>>>
>>>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>>
>>> feel free to merge.
>>
>> Thank you Michael.
>>
>> I've already included PATCH 1 in a pull-request for trivial patches
>> branch.
>>
>> I'm not sure the other patches are good candidates for trivial
>> patches
>> branch, but is there any other maintainer that can include them in a
>> pull request (before the freeze)?
>>
>> Amit?
>> [Do you want I manage a virtio-rng pull-request for you?]
> 
> Hello Laurent,
> 
> Apologies as I haven't been around for a bit.

No problem.

> I don't mind you doing the pull req yourself if you have sufficient
> reviews.

I think it's too late to push this in 4.1., so it this will wait next
release.

>   Do you also want to consider maintaining rng yourself?

It's up to you: if you think you don't have time to maintain virtio-rng,
I can manage this.

Thanks,
Laurent