Re: [Qemu-devel] [RFC PATCH 0/4] virtio-rng and RngBackend infrastructure (v2)

Re: [Qemu-devel] [RFC PATCH 0/4] virtio-rng and RngBackend infrastructure (v2)

[Paul Brook]

> This series depends on my QOM -object series that I just posted.
> 
> In Amit's thread on virtio-rng, danpb mentioned that we really ought to
> have a proper RNG backend infrastructure and of course he's correct on
> that.
> 
> Now that we have QOM, I wanted to demonstrate how we can use QOM to
> construct a complete backend without adding any new infrastructure.
> 
> I've now implemented a urandom and egd backend and tested them.  I think
> the first three patches are ready to go.

I never really understood why this exists in the first place.  It's a simple 
readonly charcter device.  IMHO you should be using virtio-serial.  This is 
virtio-console v.s. virtio-serial all over again.
The only thing close to a reason I've heard is that guest OS is incompetent 
and can't source random rata from a serial device.

Even accepting the pointless guest device, I see absolutely no reason to have 
special infrastructure for this within qemu.  Character devices do everything 
you need.  Creating annother "read stream of data" API is needless duplication 
and only going to reintroduce bugs we already fixed in the character device 
layer.

Paul

Re: [Qemu-devel] [RFC PATCH 0/4] virtio-rng and RngBackend infrastructure (v2)

[Amit Shah]

On (Sun) 01 Jul 2012 [23:06:36], Paul Brook wrote:
> > This series depends on my QOM -object series that I just posted.
> > 
> > In Amit's thread on virtio-rng, danpb mentioned that we really ought to
> > have a proper RNG backend infrastructure and of course he's correct on
> > that.
> > 
> > Now that we have QOM, I wanted to demonstrate how we can use QOM to
> > construct a complete backend without adding any new infrastructure.
> > 
> > I've now implemented a urandom and egd backend and tested them.  I think
> > the first three patches are ready to go.
> 
> I never really understood why this exists in the first place.  It's a simple 
> readonly charcter device.  IMHO you should be using virtio-serial.  This is 
> virtio-console v.s. virtio-serial all over again.
> The only thing close to a reason I've heard is that guest OS is incompetent 
> and can't source random rata from a serial device.

Linux has a virtio-rng driver for quite a while; some other hypervisor
implementation may have the corresponding device, I'm not aware of
it.  This series just adds a corresponding device to qemu.

You're right that it's just a chardev interface, and virtio-serial
would've been ideal for this, but given Linux has a driver already,
it's best to add the device to qemu.

> Even accepting the pointless guest device, I see absolutely no reason to have 
> special infrastructure for this within qemu.  Character devices do everything 
> you need.  Creating annother "read stream of data" API is needless duplication 
> and only going to reintroduce bugs we already fixed in the character device 
> layer.

Well I do agree with this qemu doesn't really need to bother about the
various ways of getting random numbers, just accept them over a
chardev and fwd them to the guest.

		Amit

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

[Amit Shah]

On (Tue) 26 Jun 2012 [08:01:20], Anthony Liguori wrote:
> On 06/26/2012 05:48 AM, Amit Shah wrote:
> >On (Mon) 25 Jun 2012 [17:59:28], Anthony Liguori wrote:
> >>On 06/25/2012 05:46 PM, Anthony Liguori wrote:
> >>>From: Amit Shah<amit.shah@redhat.com>
> >
> >>>diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> >
> >>>+static void virtio_rng_class_init(ObjectClass *klass, void *data)
> >>>+{
> >>>+    DeviceClass *dc = DEVICE_CLASS(klass);
> >>>+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> >>>+
> >>>+    k->init = virtio_rng_init_pci;
> >>>+    k->exit = virtio_rng_exit_pci;
> >>>+    k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
> >>>+    k->device_id = PCI_DEVICE_ID_VIRTIO_RNG;
> >>>+    k->revision = VIRTIO_PCI_ABI_VERSION;
> >>>+    k->class_id = PCI_CLASS_OTHERS;
> >>
> >>WHQL tends to get very particular about PCI classes.  Do we
> >>understand the implications of making this CLASS_OTHERS and WHQL?
> >
> >I've not asked around; will update with info when I get it.
> 
> Thanks.

... and I heard back: PCI_CLASS_OTHERS is fine; no problem.

> >>>+/* Send data from a char device over to the guest */
> >>>+static void chr_read(void *opaque, const void *buf, size_t size)
> >>>+{
> >>>+    VirtIORNG *vrng = opaque;
> >>>+    size_t len;
> >>>+    int offset;
> >>>+
> >>>+    if (!is_guest_ready(vrng)) {
> >>>+        return;
> >>>+    }
> >>>+
> >>>+    offset = 0;
> >>>+    while (offset<   size) {
> >>>+        if (!pop_an_elem(vrng)) {
> >>>+            break;
> >>>+        }
> >>>+        len = iov_from_buf(vrng->elem.in_sg, vrng->elem.in_num,
> >>>+                           buf + offset, 0, size - offset);
> >>>+        offset += len;
> >>>+
> >>>+        virtqueue_push(vrng->vq,&vrng->elem, len);
> >>>+        vrng->popped = false;
> >>>+    }
> >>>+    virtio_notify(&vrng->vdev, vrng->vq);
> >>>+
> >>>+    /*
> >>>+     * Lastly, if we had multiple elems queued by the guest, and we
> >>>+     * didn't have enough data to fill them all, indicate we want more
> >>>+     * data.
> >>>+     */
> >>>+    len = pop_an_elem(vrng);
> >>>+    if (len) {
> >>>+        rng_backend_request_entropy(vrng->rng, size, chr_read, vrng);
> >>>+    }
> >>
> >>Because of this above while() loop, you won't see entropy requests
> >>for every request that comes from the guest depending on how data
> >>gets buffered in the socket.
> >
> >So the issue is we currently can't get the iov_size without popping
> >the elem from the vq.
> 
> I think we could split out some of the logic in virtqueue_pop to
> implement a virtqueue_peek().

Just sent out a series that adds virtqueue_get_avail_bytes() which
does this.  A rebased series on top of that eliminates the need for
popping and save/load of the elem.

		Amit

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

[Dor Laor]

On 07/06/2012 03:06 PM, Amit Shah wrote:
> On (Tue) 26 Jun 2012 [08:01:20], Anthony Liguori wrote:
>> >On 06/26/2012 05:48 AM, Amit Shah wrote:
>>> > >On (Mon) 25 Jun 2012 [17:59:28], Anthony Liguori wrote:
>>>> > >>On 06/25/2012 05:46 PM, Anthony Liguori wrote:
>>>>> > >>>From: Amit Shah<amit.shah@redhat.com>
>>> > >
>>>>> > >>>diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>>> > >
>>>>> > >>>+static void virtio_rng_class_init(ObjectClass *klass, void *data)
>>>>> > >>>+{
>>>>> > >>>+    DeviceClass *dc = DEVICE_CLASS(klass);
>>>>> > >>>+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>>>> > >>>+
>>>>> > >>>+    k->init = virtio_rng_init_pci;
>>>>> > >>>+    k->exit = virtio_rng_exit_pci;
>>>>> > >>>+    k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
>>>>> > >>>+    k->device_id = PCI_DEVICE_ID_VIRTIO_RNG;
>>>>> > >>>+    k->revision = VIRTIO_PCI_ABI_VERSION;
>>>>> > >>>+    k->class_id = PCI_CLASS_OTHERS;
>>>> > >>
>>>> > >>WHQL tends to get very particular about PCI classes.  Do we
>>>> > >>understand the implications of making this CLASS_OTHERS and WHQL?
>>> > >
>>> > >I've not asked around; will update with info when I get it.
>> >
>> >Thanks.
> ... and I heard back: PCI_CLASS_OTHERS is fine; no problem.
>

Unclassified device (PCI_CLASS_OTHERS) is the easiest way to pass M$ 
WHQL. It doesn't do any functional tests to the device itself, unlike 
NICs/StorPort devices. If M$ had a specific entropy device type it would 
be good to use it but I doubt such exist.