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

[Anthony Liguori]

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<address@hidden>
>
> > > 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.

> > > diff --git a/hw/virtio-rng.c b/hw/virtio-rng.c
> > > new file mode 100644
> > > index 0000000..a4c2ac1
> > > --- /dev/null
> > > +++ b/hw/virtio-rng.c
> > > @@ -0,0 +1,186 @@
> > > +/*
> > > + * A virtio device implementing a hardware random number generator.
> > > + *
> > > + * Copyright 2012 Red Hat, Inc.
> > > + * Copyright 2012 Amit Shah<address@hidden>
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > > + * (at your option) any later version.  See the COPYING file in the
> > > + * top-level directory.
> > > + */
> > > +
> > > +#include "iov.h"
> > > +#include "qdev.h"
> > > +#include "virtio.h"
> > > +#include "virtio-rng.h"
> > > +#include "qemu/rng.h"
> > > +
> > > +typedef struct VirtIORNG {
> > > +    VirtIODevice vdev;
> > > +
> > > +    DeviceState *qdev;
> > > +
> > > +    /* Only one vq - guest puts buffer(s) on it when it needs entropy */
> > > +    VirtQueue *vq;
> > > +    VirtQueueElement elem;
> > > +
> > > +    /* Config data for the device -- currently only chardev */
> > > +    VirtIORNGConf *conf;
> > > +
> > > +    /* Whether we've popped a vq element into 'elem' above */
> > > +    bool popped;
> > > +
> > > +    RngBackend *rng;
> > > +} VirtIORNG;
> > > +
> > > +static bool is_guest_ready(VirtIORNG *vrng)
> > > +{
> > > +    if (virtio_queue_ready(vrng->vq)
> > > +&&   (vrng->vdev.status&   VIRTIO_CONFIG_S_DRIVER_OK)) {
> > > +        return true;
> > > +    }
> > > +    return false;
> > > +}
> > > +
> > > +static size_t pop_an_elem(VirtIORNG *vrng)
> > > +{
> > > +    size_t size;
> > > +
> > > +    if (!vrng->popped&&   !virtqueue_pop(vrng->vq,&vrng->elem)) {
> > > +        return 0;
> > > +    }
> > > +    vrng->popped = true;
> > > +
> > > +    size = iov_size(vrng->elem.in_sg, vrng->elem.in_num);
> > > +    return size;
> > > +}
> > > +
> > > +/* 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().

> If we had that, we would just send out the
> request from handle_input(), and the save/load functions wouldn't have
> to bother with the elem, too.
>
> Given that we can't do that, restricting chr_read() to one iov seems
> fine.
>
> Note that there's a bug introduced in the
> rng_backend_request_entropy() call above - it should advertise 'len'
> instead of 'size' here.

Ah, good catch.  Thanks.

> > Things are simplified now because boundaries will be respected (they
> > always would be with qemu_chr_can_read too FWIW).  We should have
> > chr_read() process exactly one entropy request.  We should only have
> > one pending entropy request at a time too.
> >
> > That eliminates the need for looping here and should simplify the whole code.
> >
> > > +}
> > > +
> > > +static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
> > > +{
> > > +    VirtIORNG *vrng = DO_UPCAST(VirtIORNG, vdev, vdev);
> > > +    size_t size;
> > > +
> > > +    size = pop_an_elem(vrng);
> > > +    if (size) {
> > > +        rng_backend_request_entropy(vrng->rng, size, chr_read, vrng);
> > > +    }
> > > +}
> > > +
> > > +static uint32_t get_features(VirtIODevice *vdev, uint32_t f)
> > > +{
> > > +    return f;
> > > +}
> > > +
> > > +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.

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.

Regards,

Anthony Liguori

>                 Amit