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:

> >>>+/* 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().

I remember vaguely I looked at it before, and can't recollect why I
didn't follow through with it.  Will re-look and note it.

> >>>+
> >>>+static void virtio_rng_save(QEMUFile *f, void *opaque)
> >>>+{
> >>>+    VirtIORNG *vrng = opaque;
> >>>+
> >>>+    virtio_save(&vrng->vdev, f);
> >>>+
> >>>+    qemu_put_byte(f, vrng->popped);
> >>>+    if (vrng->popped) {
> >>>+        qemu_put_buffer(f, (unsigned char *)&vrng->elem,
> >>>+                        sizeof(vrng->elem));
> >>>+    }
> >>
> >>Okay, new rule: if you copy a struct verbatim to savevm, your next 5
> >>patches will be automatically nacked.
> >>
> >>Seriously, this is an awful thing to do.  I don't care if we do it
> >>in other places in the code.  It's never the right thing to do.]
> 
> Err, this came in with virtio-scsi too apparently.
> 
> Paolo/Stefan, please fix this in virtio-scsi before the 1.2 release.
> This is really not something we want to have to maintain long term.
> 
> >>This is an unpacked unaligned structure with non-fixed sized types
> >>in it. that are greater than a byte.  This breaks across endianness,
> >>compiler versions, etc.
> >
> >Any ideas how to get it done?  (CC'ed Juan).
> 
> Just save the individual fields of the structure using the
> appropriate accessors.  Then you don't have to worry about alignment
> and endianness.

I think we need an accessor for the whole for VirtQueueElem.  That
keeps individual devices agnostic from struct changes.

Then, we would need a way to deal with the individual elements in that
struct, which is tricky.  There are pointers there.

> We can't change the existing devices (sans virtio-scsi) because it's
> now part of the ABI but we can avoid making the same mistakes again.

Yup.

                Amit