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

[Daniel P. Berrange]

On Mon, Jun 25, 2012 at 07:54:18AM -0500, Anthony Liguori wrote:
> On 06/25/2012 07:30 AM, Daniel P. Berrange wrote:
> >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
> 
> Why not just do:
> 
> -device virtio-rng-pci[,file=/dev/urandom|chardev=foo]
> 
> ?
> 
> I think that's better than doing chardev + protocol argument.
> 
> Since the 'file=' option is actually usable by humans.

This is mixing up frontend + backend config again, which is what
I thought we were trying to get away from, and is uneccessarily
restricting the flexibility in config to just a plain file in
the raw mode.

If we want a 'simple' option, then rather than polluting -device
config, then shouldn't we setup an alias

  -rng virtio,file=/dev/urandom

which QEMU auto-expands to

  -device virtio-rng-pci,chardev=foo
  -chardev id=foo,file=/dev/urandom

as we do for other types of option in QEMU

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