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

[Ian Molton]

Paul Brook wrote:
>> So, rather than bike-shedding, how about making some kind of decision?
> 
> Ok, let me make this simple.

Great!

> Features such as rate limiting and EGD protocol translation should not be 
> part 
> of individual device emulation. They are part of the host interface, not the 
> guest machine.  If you really want these in qemu then they should be 
> implemented as part of the chardev layer.

Well thats a bit more descriptive than a NAK at last. Thankyou.

So, before I bother commiting work time to this, will a patch to
implement rate limiting in the chardev layer be accepted in principle?

Is there going to be a fuss over what *kind* of rate limiting? Am I
going to have to implement it both for in coming AND outgoing streams?

Personally, I'm not interested in implementing more than basic rate
limiting, MAYBE with a burst limit as well, and only on incoming streams.

Is that going to be acceptable? If not, then what?

> It may be a bit more work to implement these properly. However I'm not going 
> to accept a half-assed implementation just because someone wrote it.

You cant realistically expect someone to implement a feature across an
entire (sub)system every time a single use-case is found.

I actually like the idea of putting it in the chardev layer, so I dont
mind doing the rejiggery to implement that, as long as I'm not going to
be forced to implement solutions for every corner case in creation (I
only have a small test case and less than infinite time)

> A direct result of this is that virtio-rng degenerates to a crippled virtual 
> serial port.

And a very small, very simple driver with near on nothing to go wrong.
The kernel interface already exists, and has been debugged (it used to
have a major alignment-based bug) by me. Its in use by other
hypervisors. It exists, and people want support for it. And I want to
write the support.

Just because *you* don't think its useful doesn't mean its inherently
bad - and its a device driver - if you don't like it, don't build it in,
just like I don't build the cirrus logic VGA driver in.

I could understand your POV if you were saying that (once the rate
limiting is moved to the chardev layer) the driver was actually broken,
but as it *isnt* and it supports a pre-existing kernel virtio device, I
don't see why you have a problem with it.

In fact, if no other users of the kernels virtio-rng driver existed,
then I'd (almost) agree with you. But they do, and with the rate
limiting moved into a chardev, the driver becomes simpler than the
serial drivers.

One last and quite important point - where should the EGD protocol
implementation go? really it needs to work as kind-of a line discipline,
but AFAICT thats not supported? it would be a mistake to put rate
limiting in the chardev layer and leave the EGD protocol implementation
in the driver.

Ideally, the layering would be

EGD -> rate limiting -> device driver

But I don't see quite how this will fit into the chardev layer cleanly.

TBH, for the sake of one very simple driver, and given that apparently
no other users in qemu seem to want rate-limiting, *I( think that you
are massively over-complicating matters right now. If more drivers need
rate limiting, perhaps, but that doesnt seem to be the case.

> We already have a virtual serial port implementation designed for 
> exactly this kind of application.

Except that it doesn't speak to the kernels virtio-rng implementation.
And that interface is not going to change just because you don't like
it. (Unless you'd like to rewrite the kernels hwrng core? feel free! I'm
sure it'd be appreciated - but if not, then don't complain)

> IMHO virtio-rng is a complete waste of time and space.

To you. Not everyone. Unfortunately, qemu is missing some infrastructure
(like the SIZE property and socket reconnects) right now that would make
it useful.

Could I ask that you review and apply the SIZE property patch and the
reconnect patch? I've modified both of those in line with earlier
criticism, and their acceptance should be orthogonal to the issues
surrounding virtio-rng. Both patches provide useful core functionality.
They were submitted as part of the same patchset, but I'll happily break
them out if required. I've seen no negative remarks about those patches
this time around, so it should be time to apply them.

-Ian