Re: [Qemu-devel] [PATCH 2/2] VirtIO RNG

[Ian Molton]

Paul Brook wrote:
>> Paul Brook wrote:

>> So now everything that looks like a stream of bytes has to use the
>> virtio-serial code...
> 
> IMO, yes. That's the whole point of virtio-serial.
> 
> You can handle it however you like in your in your favourite guest OS, but I 
> object to qemu having multiple virtio devices that look and smell exactly 
> like 
> a serial port.

I dont think it looks and smells exactly like a serial port. The code is
simpler to begin with, nor does it support any out-of-band signalling.

>> Why? Its not like it'll make the rng device any simpler, smaller,
>> faster, or reduce its dependencies. Virtio is simple enough to begin with!
> 
> I disagree. Past experience shows that it's more than possible to completely 
> screw up something as simple as a serial port.

Straw man. Serial ports might be fairly simple things, but my device is
simpler still than most of them, and you're talking about emulating a
serial port, where you have control lines, line disciplines, etc. to
emulate, all of which need to be right. None of that is a concern in my
driver.

> If everything that looks like a 
> serial port provides their own implementation then the chances are they'll 
> all 
> be missing something, and broken in different ways.

Better never write any code then, its garunteesd to have a bug in it.

I *know* what you're getting at - I removed hundreds of lines of common
code copied and randomly, brokenly, modified from the various ASoC
kernel drivers a few months back.

But this bit of code doesnt really share enough in common with anything
else to make it worth handling your way. Alternatively, prove me wrong!

> Take rate limiting as an example: Your implementation has at least one 
> outright bug (using gettimeofday),

Arguably a bug. I'd disagree that it causes any problems in practice
(qemu_gettimeofday, btw - which I was specifically requested to use
during an earlier review of this patch...)

> and various questionable characteristics (is a 1-second token bucket 
> appropriate).

So what? if people dont like the default policy, or its too limited,
they can feel free to contribute patches. Its hardly like this is core
code that minute changes will require months of review to ratify!

At least I'm doing something about the
total-lack-of-any-support-whatsoever situation.

> The EGD protocol bits are another example. You aren't exposing any of the 
> interesting bits of this protocol to the guest, so it seems entirely 
> reasonable to connect this to a serial port.

Except that then we need to have a little daemon to feed rate limited
EGD protocol data to said serial port.

So now you've got the situation of the USB-serial entropy key, feeding
the (potentially) proprietary egd-compatible-but-not-rate-limited daemon
 that feeds a rate limiting daemon into a serial port on qemu that feeds
the guest OS.

So this is running in a server farm with 64 or 128 guests sat on it.
each one with its own silly rate limiting daemon / serial port mux from
the egd daemon the USB stick feeds. What happens when you apt-get
upgrade and the egd daemon gets replaced? you really want to notify and
restart all 128 noddy mux daemons and reconnect qemu to them? Why dont
we just kill the guests and reboot them all too?

My patchset handled this case without any problems at all (the reader
would simply (and without blocking) reconect to the server).

The point is that you can take 'generic support' too far. Sure you CAN
model this as a bunch of serial ports, and feed them from rate limiting
daemons, which in turn read from some egd-speaking daemon or hardware,
but EGD protocol is *SO* simple. Why on earth go to these lengths when
it can be implemented and embedded in a handful of lines of code?

The chardev reconnect stuff was in the generic qemu chardev layer and
thus reuseable by other parts of qemu that need to maintain a persistent
state even when connected to other processes via sockets, pipes, fifos etc.

> That allows it to be used by guests that don't have a virtio-rng driver.

Thats already supported by use of TCP tunnels. but thats 1) not
efficient and 2) not very secure - few sources of entropy provide crypto
to garuntee that the data source hasnt been frobbed.   3) much more
vulnerable to attack from processes running on the guest (TCP is much
more exposde than the rng-core internals)

So, rather than bike-shedding, how about making some kind of decision?

Either:

1) Dictate how the code is going to be written (in which case, since you
know it so well, write it yourself)

or:

2) Accept that not everyone likes your (idea for a) implementation, that
prior interfaces exist on the guest side, and that a perfectly good and
non-intrusive way of doing it on qemu has beeen written for you.
(including a couple of other nice features / fixes along the way, like
adding the missing SIZE_ property, socket reconnect support, etc.)

Bear in mind that its rather unfair to (as has been in this case) have a
patch reviewed, modified based on criticism, and then rejected after
effort has been wasted working on it.

-Ian