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

[Daniel P. Berrange]

On Mon, Jun 25, 2012 at 07:22:13AM -0500, Anthony Liguori wrote:
> On 06/25/2012 07:10 AM, Daniel P. Berrange wrote:
> >On Fri, Jun 22, 2012 at 02:59:13PM -0500, Anthony Liguori wrote:
> >>On 06/22/2012 01:50 PM, Amit Shah wrote:
> >>>On (Fri) 22 Jun 2012 [08:44:52], Anthony Liguori wrote:
> >>>>On 06/22/2012 08:34 AM, Daniel P. Berrange wrote:
> >>>>>
> >>>>>Oh, that's a good point.
> >>>
> >>>But easily fixed.
> >>
> >>Of course, except that now we have to maintain compatibility so some
> >>hideous hack goes in.
> >>
> >>This is what fundamentally makes using events a bad approach.  There
> >>are more problems lurking.  This is the fundamental complexity of
> >>having two non-synchronized communication channels when you're
> >>trying to synchronize some sort of activity.
> >>
> >>BTW, despite what danpb mentioned, you are rate limiting entropy
> >>requests in this patch series....
> >>
> >>>>>>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.)
> >>
> >>As it turns out, the EGD protocol also has a mechanism to ask for
> >>ask much entropy as is available at the current moment.  It also has
> >>a mechanism to query the amount of available entropy.
> >>
> >>And no, you're assertion that we don't care about how much entropy
> >>the guest is requesting is wrong.  If we lose entropy requests, then
> >>we never know if the guest is in a state where it's expecting
> >>entropy and we're not sending it.
> >>
> >>Consider:
> >>
> >>- Guest requests entropy (X bytes)
> >>- libvirt sees request
> >>- libvirt sends X bytes to guest
> >>- Guest requests entropy (Y bytes), QEMU filters due to rate limiting
> >>
> >>The guest will never request entropy again and libvirt will never
> >>send entropy again.  The device is effectively dead-locked.
> >
> >WRT the impl of rate limiting Amit has used in this patch, you
> >are correct, however, this is not how QEMU should be rate
> >limiting this event. As mentioned in an earlier reply in this
> >thread, any rate limited /must/ work as follows:
> >
> >    - 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
> >
> >ie, the byte count for any events which are dropped, must be added to
> >the byte count in the next emitted event. Also as Amit mentioned in his
> >reply to me, it should take account of multiple devices, or we should
> >limit QEMU to 1 single RNG device per guest, as we do for the balloon
> >driver.
> 
> QMP events are not meant to be used like this.  They are posted
> events and since we cannot know if there is something connected to
> the monitor, it's always possible for the backend to lose the
> information associated with an event.
> 
> Events really should just be used to indicate, "hey, you should go
> query information from QEMU now".
> 
> Even what you suggest above doesn't handle the case where a
> management application restarts.
> 
> If you were going to do this via QMP, you'd want to introduce a
> command to query the total outstanding requested entropy for a given
> device and then send a event whenever that value changes.
> 
> But again, it's silly to use QMP for this.  Using an inline protocol
> simplifies things quite a bit.

Ok, I agree that being rebust against mgmt layer restarts is a good
reason for not relying on QMP events for this, and that using the
EGD protocol would nicely solve this problem. So lets drop the QMP
event. I still think it is key to allow use of raw chardevs in
addition to EDGE though, so you can easily attached QEMU directly
to a /dev/urandom or an equivalent entropy source & just let it pull
as much as it likes.

Thus my suggestion for the syntax

  -chardev [regular chardev opts],mode=raw|egd

similar to how we allow 'telnet' protocol to be optionally turned on
for chardevs with serial/parallel ports


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|