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

[Daniel P. Berrange]

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.

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