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

[Anthony Liguori <anthony@codemonkey.ws>]

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<anthony@codemonkey.ws>    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.
>>>
>>>>                                                        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.
>
>> 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.

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

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

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

Regards,

Anthony Liguori

> Daniel