Re: [Qemu-devel] [PATCH v3 1/1] virtio-rng: hardware random number generator device

[Amit Shah]

On (Fri) 22 Jun 2012 [08:44:52], Anthony Liguori wrote:
> On 06/22/2012 08:34 AM, Daniel P. Berrange wrote:
> >On Fri, Jun 22, 2012 at 07:58:53AM -0500, Anthony Liguori wrote:
> >>On 06/22/2012 07:31 AM, Daniel P. Berrange wrote:
> >>>On Fri, Jun 22, 2012 at 07:22:51AM -0500, Anthony Liguori wrote:
> >>>>On 06/22/2012 07:12 AM, Markus Armbruster wrote:
> >>>>>Anthony Liguori<address@hidden>    writes:
> >>>>>>Nack.
> >>>>>>
> >>>>>>Use a protocol.  This is not what QMP events are designed for!
> >>>>>>
> >>>>>>No human is going to launch nc to a unix domain socket to launch QEMU.
> >>>>>>That's a silly use-case to design for.
> >>>>>
> >>>>>To be honest, I'm a bit surprised to see working code that got an ACK
> >>>>>from the guys with the problem it solves rejected out of hand over
> >>>>>something that feels like artistic license to me.
> >>>>
> >>>>This is an ABI!  We have to support it for the rest of time.
> >>>>Everything else is a detail that is fixable but ABIs need to not
> >>>>suck from the beginning.
> >>>>
> >>>>And using a QMP event here is sucks.  It disappoints me that this is
> >>>>even something I need to explain.
> >>>>
> >>>>QMP events occur over a single socket.  If you trigger them from
> >>>>guest initiated activities (that have no intrinsic rate limit), you
> >>>>run into a situation where the guest could flood the management tool
> >>>>and/or queue infinite amounts of memory (because events have to be
> >>>>queued before they're sent).  So we have rate limiting for QMP
> >>>>events.
> >>>>
> >>>>That means QMP events (like this one) are unreliable.
> >>>
> >>>No it doesn't. As it stands currently, the only events that are
> >>>rate limited, are those where there is no state information to
> >>>loose. ie, the new event completely superceeds the old event
> >>>without loosing any information.

So I'm assuming you don't have a problem with this anymore.

> >>>>                                                       But since QMP
> >>>>events aren't acked, there's no way for the management tool to know
> >>>>whether a QMP event was dropped or not.  So you can run into the
> >>>>following scenario:
> >>>>
> >>>>- Guest sends randomness request for 10 bytes
> >>>>- QMP event gets sent for 10 bytes
> >>>>- Guest sends randomness request for 4 bytes
> >>>>- QMP is dropped
> >>>>
> >>>>Now what happens?  With the current virtio-rng, nothing.  It gets
> >>>>stuck in read() for ever.  Now what do we do?
> >>>
> >>>The RNG event will not be able to use the generic rate limiting
> >>>since it has state associated with it. The rate limiting of the
> >>>RNG QMP event will need to take account of this state, ie it
> >>>will have to accumulate the byte count of any events dropped for
> >>>rate limiting:
> >>>
> >>>   - Guest sends randomness request for 10 bytes
> >>>   - QMP event gets sent for 10 bytes
> >>>   - Guest sends randomness request for 4 bytes
> >>>   - QMP is dropped
> >>>   - Guest sends randomness request for 8 bytes
> >>>   - QMP event gets sent for 12 bytes
> >>
> >>BTW, in the current design, there's no way to tell *which*
> >>virtio-rng device needs entropy if you have multiple virtio-rng
> >>devices.
> >
> >Oh, that's a good point.

But easily fixed.

> >>All of these problems are naturally solved using a protocol over a 
> >>CharDriverState.
> >
> >Can we at least agree on merging a patch which just includes the
> >raw chardev backend support for virtio-rng ? ie drop the QMP
> >event for now, so we can make some step forward.
> 
> All we need to do to support EGD is instead of doing:
> 
> +    QObject *data;
> +
> +    data = qobject_from_jsonf("{ 'bytes': %" PRId64 " }",
> +                              size);
> +    monitor_protocol_event(QEVENT_ENTROPY_NEEDED, data);
> +    qobject_decref(data);
> 
> Do:
> 
>     while (size > 0) {
>         uint8_t partial_size = MIN(255, size);
>         uint8_t msg[2] = { 0x02, partial_size };
> 
>         qemu_chr_write(s->chr, msg, sizeof(msg));
> 
>         size -= partial_size;
>     }
> 
> And that's it.  It's an extremely simple protocol to support.  It
> would actually reduce the total size of the patch.

So I now get what your objection is, and that is because of an
assumption you're making which is incorrect.

An OS asking for 5038 bytes of entropy is doing just that: asking for
those bytes.  That doesn't mean a hardware device can provide it with
that much entropy.  So, the hardware device here will just provide
whatever is available, and the OS has to be happy with what it got.
If it got 200 bytes from the device, the OS is then free to demand
even 7638 bytes more, but it may not get anything for quite a while.

(This is exactly how things work with /dev/random and /dev/urandom
too, btw.  And /dev/urandom was invented for apps which can't wait for
all the data they're asking to come to them.)

All this is expected.  Keep in mind that the hardware-generated
entropy is read from /dev/hwrng, which again is a /dev/random-like
interface, and /dev/hwrng entropy can be fed into /dev/random by using
rngd.  All that is standard stuff.

So: the QMP event here is just a note to libvirt that the guest is
asking for data, and as an additional hint, it also mentions how much
data the guest wants right now.  No party is in any kind of contract
to provide exactly what's asked for.

> >As mentioned in the previous thread, I see no issue with
> >later implementing an alternate protocol on the chardev
> >backend eg as we have raw vs telnet mode for serial ports,
> >we ought to be able to have a choice of raw vs egd mode for
> >virtio-rng backends
> 
> I don't really understand how raw mode works other than reading as
> much from /dev/urandom as possible and filling the socket buffer
> buffer with it.

That's all that's needed for the simplest (while being as effective as
any other) implementation to work.

> I think the only two modes that make sense are EGD over a socket and
> direct open of /dev/urandom.

Directly wiring /dev/urandom via chardev is more flexible, and doesn't
involve anything else.  No chardev gimmicks as well.

> But I think the EGD mode is the more important of the two.

Why?  There's nothing standard about it.

                Amit