Re: [Qemu-devel] [RFC] Functions bus_foreach and device_find from libqos virtio API

[Stefan Hajnoczi]

On Mon, Jun 23, 2014 at 04:55:22PM +0200, Marc Marí wrote:
> +static QVirtioDevice *qpcidevice_to_qvirtiodevice(QPCIDevice *dpci)
> +{
> +    QVirtioDevice *dvirtio;
> +    dvirtio = g_malloc0(sizeof(*dvirtio));
> +
> +    if (dpci) {
> +        dvirtio->device_id = qpci_config_readw(dpci, PCI_DEVICE_ID);
> +        dvirtio->device_type = qpci_config_readw(dpci, PCI_SUBSYSTEM_ID);
> +        dvirtio->location = dpci->devfn;
> +        dvirtio->vq = g_malloc0(sizeof(QVirtQueue));

QVirtioDevice doesn't know about PCI, so device_id and location are not
appropriate - at least as public fields that callers are expected to
access.  It's fine to stash away the PCI-specific information, probably
by defining a QVirtioPCIDevice struct that embeds QVirtioDevice:

typedef struct {
    QVirtioDevice vdev;
    QPCIDevice *pdev;
} QVirtioPCIDevice;

Then virtio-pci functions can cast from the generic QVirtioDevice back
to the PCI-specific QVirtioPCIDevice:

static void qvirtio_pci_notify(QVirtioDevice *vdev)
{
    QVirtioPCIDevice *dev = (QVirtioPCIDevice*)vdev;

    qpci_io_writew(dev->pdev, ...);
}

> +void qvirtio_pci_foreach(uint16_t device_type,
> +                void (*func)(QVirtioDevice *d, void *data), void *data)
> +{
> +    QPCIBus *bus;
> +    QPCIDevice *dev;
> +    int slot;
> +    int fn;
> +
> +    bus = qpci_init_pc();

This might be a design flaw in qpci_init_pc().  Not sure it's a good
idea to have a local PCI bus for every qvirtio_pci_foreach() call.

Tests should be able to operate on different PCI busses, if the machine
has several, but devices on the same bus should probably share a QPCIBus
instance.

> +    for (slot = 0; slot < 32; slot++) {
> +        for (fn = 0; fn < 8; fn++) {
> +            dev = g_malloc0(sizeof(*dev));
> +            dev->bus = bus;
> +            dev->devfn = QPCI_DEVFN(slot, fn);
> +
> +            if (qpci_config_readw(dev, PCI_VENDOR_ID) == 0xFFFF) {
> +                g_free(dev);
> +                continue;
> +            }
> +
> +            if (device_type != (uint16_t)-1 &&
> +                qpci_config_readw(dev, PCI_SUBSYSTEM_ID) != device_type) {
> +                continue;
> +            }
> +
> +            func(qpcidevice_to_qvirtiodevice(dev), data);
> +        }
> +    }

Can you reuse qpci_device_foreach() instead of duplicating this code?

> +uint64_t qvring_desc_addr(uint64_t desc_pa, int i);
> +uint32_t qvring_desc_len(uint64_t desc_pa, int i);
> +uint16_t qvring_desc_flags(uint64_t desc_pa, int i);
> +uint16_t qvring_desc_next(uint64_t desc_pa, int i);
> +uint16_t qvring_avail_flags(QVirtQueue *vq);
> +uint16_t qvring_avail_idx(QVirtQueue *vq);
> +uint16_t qvring_avail_ring(QVirtQueue *vq, int i);
> +uint16_t qvring_used_event(QVirtQueue *vq);
> +void qvring_used_ring_id(QVirtQueue *vq, int i, uint32_t val);
> +void qvring_used_ring_len(QVirtQueue *vq, int i, uint32_t val);
> +uint16_t qvring_used_idx(QVirtQueue *vq);
> +void qvring_used_idx_set(QVirtQueue *vq, uint16_t val);
> +void qvring_used_flags_set_bit(QVirtQueue *vq, int mask);
> +void qvring_used_flags_unset_bit(QVirtQueue *vq, int mask);
> +void qvring_avail_event(QVirtQueue *vq, uint16_t val);
> +
> +void qvirtqueue_push(QVirtQueue *vq, const QVirtQueueElement *elem,
> +                                                        unsigned int len);
> +void qvirtqueue_flush(QVirtQueue *vq, unsigned int count);
> +void qvirtqueue_fill(QVirtQueue *vq, const QVirtQueueElement *elem,
> +                                        unsigned int len, unsigned int idx);
> +void qvirtqueue_pop(QVirtQueue *vq, QVirtQueueElement *elem);
> +void qvirtqueue_map_sg(/*struct iovec *sg,*/ uint64_t *addr,
> +    size_t num_sg, int is_write);
> +void qvirtio_notify(QVirtioDevice *vdev, QVirtQueue *vq);
> +int qvirtio_queue_ready(QVirtQueue *vq);
> +int qvirtio_queue_empty(QVirtQueue *vq);

Please drop all the unimplemented functions and unused structs from the
next patch revision.  Let's focus on one step at a time:
1. Finding devices
2. Device reset, setting status field bits, feature bit negotiation
3. Notification (qtest->QEMU kick, QEMU->qtest interrupt)
4. Virtqueues
5. Simple virtio-blk tests (e.g. read a block from the disk) to prove it
   works

I'm suggesting you send the next patch revision without RFC and just
with a test case for finding devices (no unimplemented stuff).  That way
you can send new patches each week and have them merged incrementally,
rather than pushing along one big series that keeps growing until the
very end.

> +
> +#endif
> diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
> index d53f875..4f5b1e0 100644
> --- a/tests/virtio-blk-test.c
> +++ b/tests/virtio-blk-test.c
> @@ -2,6 +2,7 @@
>   * QTest testcase for VirtIO Block Device
>   *
>   * Copyright (c) 2014 SUSE LINUX Products GmbH
> + * Copyright (c) 2014 Marc Marí
>   *
>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
>   * See the COPYING file in the top-level directory.
> @@ -9,26 +10,51 @@
>  
>  #include <glib.h>
>  #include <string.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <stdio.h>
>  #include "libqtest.h"
> -#include "qemu/osdep.h"
> +#include "libqos/virtio.h"
> +/*#include "qemu/osdep.h"*/
>  
> -/* Tests only initialization so far. TODO: Replace with functional tests */
> -static void pci_nop(void)
> +#define TEST_IMAGE_SIZE (64 * 1024 * 1024)
> +
> +static char tmp_path[] = "/tmp/qtest.XXXXXX";
> +extern QVirtioBus qvirtio_pci;
> +
> +static void pci_basic(void)
>  {
> +    QVirtioDevice *dev;
> +    dev = qvirtio_pci.device_find(VIRTIO_BLK_DEVICE_ID);
> +    fprintf(stderr, "Device: %x %x %x\n",
> +                            dev->device_id, dev->location, dev->device_type);
>  }
>  
>  int main(int argc, char **argv)
>  {
>      int ret;
> +    int fd;
> +    char test_start[100];
>  
>      g_test_init(&argc, &argv, NULL);
> -    qtest_add_func("/virtio/blk/pci/nop", pci_nop);
> +    qtest_add_func("/virtio/blk/pci/basic", pci_basic);
>  
> -    qtest_start("-drive id=drv0,if=none,file=/dev/null "
> -                "-device virtio-blk-pci,drive=drv0");
> +    /* Create a temporary raw image */
> +    fd = mkstemp(tmp_path);
> +    g_assert_cmpint(fd, >=, 0);
> +    ret = ftruncate(fd, TEST_IMAGE_SIZE);
> +    g_assert_cmpint(ret, ==, 0);
> +    close(fd);
> +
> +    sprintf(test_start, "-drive if=none,id=drive0,file=%s "
> +                    "-device virtio-blk-pci,drive=drive0", tmp_path);
> +    qtest_start(test_start);

Every test case should probably start a fresh QEMU with a fresh disk
image.  That way no device register state or disk image state can affect
later test cases.

pgpdgUWQ1GLWz.pgp
Description: PGP signature