Re: [Qemu-devel] [RFC] Libqos virtio API

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3705 bytes --]

On Tue, Jun 17, 2014 at 11:02:48AM +0200, Marc Marí wrote:
> This is the draft for the libqos virtio API to create test cases for virtio
> devices. I'm looking forward to your comments.
> 
> Signed-off-by: Marc Marí <marc.mari.barcelo@gmail.com>
> ---
>  tests/libqos/virtio.h |  148 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 148 insertions(+)
>  create mode 100644 tests/libqos/virtio.h
> 
> diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h
> new file mode 100644
> index 0000000..03f28dc
> --- /dev/null
> +++ b/tests/libqos/virtio.h
> @@ -0,0 +1,148 @@
> +#define VIRTQUEUE_MAX_SIZE 1024
> +
> +struct QVirtioBus
> +{
> +    /* Send a notification IRQ to the device */
> +    void (*notify)(QVirtioDevice *d, uint16_t vector);

Guest->host notification is not an interrupt.  It's a "kick" that's
implemented by writing to a hardware register.

The argument should be the virtqueue number.

> +    
> +    /* Get configuration of the device */
> +    void (*get_config)(QVirtioDevice *d, void *config);
> +    
> +    /* Set configuration of the device */
> +    void (*set_config)(QVirtioDevice *d, void *config);
> +    
> +    /* Get features of the device */
> +    uint32_t (*get_features)(QVirtioDevice *d);
> +    
> +    /* Get status of the device */
> +    uint8_t (*get_status)(QVirtioDevice *d);
> +    
> +    /* Set status of the device  */
> +    void (*set_status)(QVirtioDevice *d, uint8_t val);
> +    
> +    /* Reset the device */
> +    void (*reset)(QVirtioDevice *d);
> +    
> +    /* Check pending IRQs */
> +    uint8_t (*query_isr)(QVirtioDevice *d);

Does this also clear the ISR?

> +    
> +    /* Loop all devices, applying a function to all, or the one specified */
> +    void (*device_foreach)(QVirtioBus *bus, uint16_t device_id, 
> +                void (*func)(QVirtioDevice *dev, void *data), void *data);
> +                
> +    /* Find and return a device */
> +    QVirtioDevice *(*device_find)(uint16_t device_id, int index);
> +};
> +
> +QVirtioBus *qvirtio_pci_init();
> +QVirtioBus *qvirtio_mmio_init();
> +QVirtioBus *qvirtio_ccw_init();

In C you should always give an arguments list.  Use void if there are no
arguments.  In C++ fn() means fn(void) but in C it means legacy K&R
behavior which no one uses due to lack of type-safety.

I don't think it's necessary to provide a function that instantiates a
QVirtioBus.  QVirtioBus is just a struct of function pointers.  It can
be declared static const:

static const QVirtioBus virtio_pci_bus = {
    .reset = virtio_pci_reset,
    ...
};

> +
> +struct QVirtioDevice
> +{
> +    /* Index in all devices of the same type in the bus */
> +    int index;
> +    
> +    /* Device type */
> +    uint16_t device_id;
> +    
> +    /* VQs associated with the device */
> +    QVirtQueue *vq;
> +};
> +
> +/*
> +I'm not sure what implementation of VirtQueue is better, QEMU's or Linux's. I
> +think QEMU implementation is better, because it will be easier to connect the
> +QEMU Virtqueue with the Libaos VirtQueue.
> +
> +The functions to use the VirtQueue are the ones I think necessary in Libqos, but
> +probably there are some missing and some others that are not necessary.
> +*/

Keep in mind that QEMU only cares about popping elements off the
available ring and pushing them onto the used ring.

The Linux guest driver implementation is the opposite.  It pushes
elements onto the available ring and pops them off the used ring.

If in doubt, follow the Linux implementation because you are
implementing the guest side of the interface, not the host side.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]