Message ID | 1403793280-4353-1-git-send-email-marc.mari.barcelo@gmail.com |
---|---|
State | New |
Headers | show |
Il 26/06/2014 16:34, Marc Marí ha scritto: > +static void qvirtio_pci_foreach(uint16_t device_type, > + void (*func)(QVirtioDevice *d, void *data), void *data) > +{ > + QVirtioPCIForeachData d = { .func = func, > + .device_type = device_type, > + .user_data = data }; > + > + if (!bus) { > + bus = qpci_init_pc(); Why not pass this as an argument? .bus_foreach and .device_find need not be part of QVirtioBus. > + } > + > + qpci_device_foreach(bus, QVIRTIO_VENDOR_ID, -1, > + qvirtio_pci_foreach_callback, &d); > +} > + > +static QVirtioDevice *qvirtio_pci_device_find(uint16_t device_type) (Also here, qvirtio_pci_device_find would just forward the bus to qvirtio_pci_foreach). > + fprintf(stderr, "Device position: %x. Device type: %x\n", > + dev->pdev->devfn, dev->vdev.device_type); You can change these to assertions, and get your first testcase too. :) You can set the address in the qtest_start argument (like "-device virtio-blk-pci,...,addr=0x0a.0") so you know the expected value. Otherwise looks good! Paolo > +{ > + QVirtioPCIDevice *dev; > + > + dev = g_malloc0(sizeof(*dev)); > + qvirtio_pci_foreach(device_type, qvirtio_pci_assign_device, dev); > + > + return (QVirtioDevice *)dev; > +}
El Thu, 26 Jun 2014 17:09:12 +0200 Paolo Bonzini <pbonzini@redhat.com> escribió: > Il 26/06/2014 16:34, Marc Marí ha scritto: > > +static void qvirtio_pci_foreach(uint16_t device_type, > > + void (*func)(QVirtioDevice *d, void *data), void > > *data) +{ > > + QVirtioPCIForeachData d = { .func = func, > > + .device_type = device_type, > > + .user_data = data }; > > + > > + if (!bus) { > > + bus = qpci_init_pc(); > > Why not pass this as an argument? .bus_foreach and .device_find need > not be part of QVirtioBus. If these functions are just Virtio PCI specific, it makes sense to take them out. If they are common to other transports, in my opinion is better to leave them there. Of course, a good solution has to be found to just having a global bus variable. It is also interesting to see that the current implementation of qpci_device_foreach does only work in one bus, so probably is not so terrible to have it as global and unique. After reading Virtio specs about MMIO and CCW transports, I understand that in MMIO you somehow choose an address to map the device, and in CCW there is a device discovery mechanism, so probably in CCW is interesting to have those functions. Marc
Il 26/06/2014 17:27, Marc Marí ha scritto: > If these functions are just Virtio PCI specific, it makes sense to take > them out. If they are common to other transports, in my opinion is > better to leave them there. Of course, a good solution has to be found > to just having a global bus variable. > > It is also interesting to see that the current > implementation of qpci_device_foreach does only work in one bus, so > probably is not so terrible to have it as global and unique. qpci_device_foreach works for one bus only because libqos doesn't support PCI bridges. But it lets you specify the PCI bus of interest, and it seems a good idea to have this in the virtio-pci implementation. Paolo
Subject better have a component prefix "[PATCH] tests: Functions bus_foreach and device_find from libqos virtio API". See also "git log tests/". Fam On Thu, 06/26 16:34, Marc Marí wrote: > Virtio header has been changed to compile and work with a real device. > Functions bus_foreach and device_find have been implemented for PCI. > Virtio-blk test case now opens a fake device. > > Signed-off-by: Marc Marí <marc.mari.barcelo@gmail.com>
On Thu, Jun 26, 2014 at 04:34:40PM +0200, Marc Marí wrote: > +static QVirtioPCIDevice *qpcidevice_to_qvirtiodevice(QPCIDevice *pdev) > +{ > + QVirtioPCIDevice *vpcidev; > + vpcidev = g_malloc0(sizeof(*vpcidev)); > + > + if (pdev) { > + vpcidev->pdev = pdev; > + vpcidev->vdev.device_type = > + qpci_config_readw(vpcidev->pdev, PCI_SUBSYSTEM_ID); > + /* TODO: When QVirtQueue is defined, change for > + g_malloc0(sizeof(QVirtQueue)); */ > + vpcidev->vdev.vq = NULL; This doesn't quite work because devices can have multiple virtqueues. Please drop the vq field from this patch. Add it later when you actually implement virtqueues. > + } > + > + return vpcidev; > +} > + > +static void qvirtio_pci_foreach_callback( > + QPCIDevice *dev, int devfn, void *data) > +{ > + QVirtioPCIForeachData *d = data; > + QVirtioPCIDevice *vpcidev = qpcidevice_to_qvirtiodevice(dev); > + > + if (vpcidev->vdev.device_type == d->device_type) { > + d->func(&vpcidev->vdev, d->user_data); > + } > +} > + > +static void qvirtio_pci_assign_device(QVirtioDevice *d, void *data) > +{ > + QVirtioPCIDevice *vpcidev = data; > + vpcidev->pdev = ((QVirtioPCIDevice *)d)->pdev; > + vpcidev->vdev.device_type = ((QVirtioPCIDevice *)d)->vdev.device_type; > + vpcidev->vdev.vq = ((QVirtioPCIDevice *)d)->vdev.vq; > +} > + > +static void qvirtio_pci_notify(QVirtioDevice *d, uint16_t vector) > +{ > + > +} > + > +static void qvirtio_pci_get_config(QVirtioDevice *d, void *config) > +{ > + > +} > + > +static void qvirtio_pci_set_config(QVirtioDevice *d, void *config) > +{ > + > +} > + > +static uint32_t qvirtio_pci_get_features(QVirtioDevice *d) > +{ > + return 0; > +} > + > +static uint8_t qvirtio_pci_get_status(QVirtioDevice *d) > +{ > + return 0; > +} > + > +static void qvirtio_pci_set_status(QVirtioDevice *d, uint8_t val) > +{ > + > +} > + > +static void qvirtio_pci_reset(QVirtioDevice *d) > +{ > + > +} > + > +static uint8_t qvirtio_pci_query_isr(QVirtioDevice *d) > +{ > + return 0; > +} Patches should only include useful, working code. Please drop all these unimplemented functions. > + > +static void qvirtio_pci_foreach(uint16_t device_type, > + void (*func)(QVirtioDevice *d, void *data), void *data) > +{ > + QVirtioPCIForeachData d = { .func = func, > + .device_type = device_type, > + .user_data = data }; > + > + if (!bus) { > + bus = qpci_init_pc(); > + } > + > + qpci_device_foreach(bus, QVIRTIO_VENDOR_ID, -1, > + qvirtio_pci_foreach_callback, &d); > +} > + > +static QVirtioDevice *qvirtio_pci_device_find(uint16_t device_type) > +{ > + QVirtioPCIDevice *dev; > + > + dev = g_malloc0(sizeof(*dev)); > + qvirtio_pci_foreach(device_type, qvirtio_pci_assign_device, dev); > + > + return (QVirtioDevice *)dev; > +} > + > +const QVirtioBus qvirtio_pci = { > + .notify = qvirtio_pci_notify, > + .get_config = qvirtio_pci_get_config, > + .set_config = qvirtio_pci_set_config, > + .get_features = qvirtio_pci_get_features, > + .get_status = qvirtio_pci_get_status, > + .set_status = qvirtio_pci_set_status, > + .reset = qvirtio_pci_reset, > + .query_isr = qvirtio_pci_query_isr, > + .bus_foreach = qvirtio_pci_foreach, > + .device_find = qvirtio_pci_device_find, > +}; > + > diff --git a/tests/libqos/virtio-pci.h b/tests/libqos/virtio-pci.h > new file mode 100644 > index 0000000..e5812af > --- /dev/null > +++ b/tests/libqos/virtio-pci.h > @@ -0,0 +1,29 @@ > +/* > + * libqos virtio PCI definitions > + * > + * 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. > + */ > + > +#ifndef LIBQOS_VIRTIO_PCI_H > +#define LIBQOS_VIRTIO_PCI_H > + > +#include "libqos/virtio.h" > +#include "libqos/pci.h" > + > +typedef struct QVirtioPCIDevice { > + QVirtioDevice vdev; > + QPCIDevice *pdev; > +} QVirtioPCIDevice; > + > +typedef struct QVirtioPCIForeachData { > + void (*func)(QVirtioDevice *d, void *data); > + uint16_t device_type; > + void *user_data; > +} QVirtioPCIForeachData; > + > +extern const QVirtioBus qvirtio_pci; > + > +#endif > diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h > new file mode 100644 > index 0000000..43a0e73 > --- /dev/null > +++ b/tests/libqos/virtio.h > @@ -0,0 +1,64 @@ > +/* > + * libqos virtio definitions > + * > + * 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. > + */ > + > +#ifndef LIBQOS_VIRTIO_H > +#define LIBQOS_VIRTIO_H > + > +#define QVIRTQUEUE_MAX_SIZE 1024 > +#define QVIRTIO_VENDOR_ID 0x1AF4 > + > +#define QVIRTIO_NET_DEVICE_ID 0x1 > +#define QVIRTIO_BLK_DEVICE_ID 0x2 > + > +typedef struct QVirtioDevice QVirtioDevice; > +typedef struct QVirtQueue QVirtQueue; > +typedef struct QVRing QVRing; > + > +typedef struct QVirtioDevice { > + /* Device type */ > + uint16_t device_type; > + > + /* VQs associated with the device */ > + QVirtQueue *vq; > +} QVirtioDevice; > + > +typedef struct QVirtioBus { > + /* Send a notification IRQ to the device */ > + void (*notify)(QVirtioDevice *d, uint16_t vector); > + > + /* 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); > + > + /* Loop all devices, applying a function to all, or the one specified */ > + void (*bus_foreach)(uint16_t device_t, > + void (*func)(QVirtioDevice *d, void *data), void *data); > + > + /* Find and return a device */ > + QVirtioDevice *(*device_find)(uint16_t device_type); > +} QVirtioBus; Please drop all the unimplemented function pointers. Nothing calls them and there is no implementation, so we cannot review them or decide whether they make sense. > +static void pci_basic(void) > { > + QVirtioPCIDevice *dev; > + dev = (QVirtioPCIDevice *)qvirtio_pci.device_find(QVIRTIO_BLK_DEVICE_ID); > + g_assert_cmphex(dev->vdev.device_type, ==, QVIRTIO_BLK_DEVICE_ID); > + fprintf(stderr, "Device position: %x. Device type: %x\n", > + dev->pdev->devfn, dev->vdev.device_type); The test case should not produce output. Please drop the fprintf().
diff --git a/tests/Makefile b/tests/Makefile index 7e53d0d..028c462 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -292,6 +292,7 @@ libqos-obj-y += tests/libqos/i2c.o libqos-pc-obj-y = $(libqos-obj-y) tests/libqos/pci-pc.o libqos-pc-obj-y += tests/libqos/malloc-pc.o libqos-omap-obj-y = $(libqos-obj-y) tests/libqos/i2c-omap.o +libqos-virtio-obj-y = $(libqos-obj-y) $(libqos-pc-obj-y) tests/libqos/virtio-pci.o tests/rtc-test$(EXESUF): tests/rtc-test.o tests/m48t59-test$(EXESUF): tests/m48t59-test.o @@ -312,7 +313,7 @@ tests/eepro100-test$(EXESUF): tests/eepro100-test.o tests/vmxnet3-test$(EXESUF): tests/vmxnet3-test.o tests/ne2000-test$(EXESUF): tests/ne2000-test.o tests/virtio-balloon-test$(EXESUF): tests/virtio-balloon-test.o -tests/virtio-blk-test$(EXESUF): tests/virtio-blk-test.o +tests/virtio-blk-test$(EXESUF): tests/virtio-blk-test.o $(libqos-virtio-obj-y) tests/virtio-net-test$(EXESUF): tests/virtio-net-test.o tests/virtio-rng-test$(EXESUF): tests/virtio-rng-test.o tests/virtio-scsi-test$(EXESUF): tests/virtio-scsi-test.o diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c new file mode 100644 index 0000000..1c8c31a --- /dev/null +++ b/tests/libqos/virtio-pci.c @@ -0,0 +1,136 @@ +/* + * libqos virtio PCI driver + * + * 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. + */ + +#include <glib.h> +#include <stdio.h> +#include "libqtest.h" +#include "libqos/virtio.h" +#include "libqos/virtio-pci.h" +#include "libqos/pci.h" +#include "libqos/pci-pc.h" + +#include "hw/pci/pci_regs.h" + +/* TODO: Check possible problems with multiple busses */ +QPCIBus *bus = NULL; + +static QVirtioPCIDevice *qpcidevice_to_qvirtiodevice(QPCIDevice *pdev) +{ + QVirtioPCIDevice *vpcidev; + vpcidev = g_malloc0(sizeof(*vpcidev)); + + if (pdev) { + vpcidev->pdev = pdev; + vpcidev->vdev.device_type = + qpci_config_readw(vpcidev->pdev, PCI_SUBSYSTEM_ID); + /* TODO: When QVirtQueue is defined, change for + g_malloc0(sizeof(QVirtQueue)); */ + vpcidev->vdev.vq = NULL; + } + + return vpcidev; +} + +static void qvirtio_pci_foreach_callback( + QPCIDevice *dev, int devfn, void *data) +{ + QVirtioPCIForeachData *d = data; + QVirtioPCIDevice *vpcidev = qpcidevice_to_qvirtiodevice(dev); + + if (vpcidev->vdev.device_type == d->device_type) { + d->func(&vpcidev->vdev, d->user_data); + } +} + +static void qvirtio_pci_assign_device(QVirtioDevice *d, void *data) +{ + QVirtioPCIDevice *vpcidev = data; + vpcidev->pdev = ((QVirtioPCIDevice *)d)->pdev; + vpcidev->vdev.device_type = ((QVirtioPCIDevice *)d)->vdev.device_type; + vpcidev->vdev.vq = ((QVirtioPCIDevice *)d)->vdev.vq; +} + +static void qvirtio_pci_notify(QVirtioDevice *d, uint16_t vector) +{ + +} + +static void qvirtio_pci_get_config(QVirtioDevice *d, void *config) +{ + +} + +static void qvirtio_pci_set_config(QVirtioDevice *d, void *config) +{ + +} + +static uint32_t qvirtio_pci_get_features(QVirtioDevice *d) +{ + return 0; +} + +static uint8_t qvirtio_pci_get_status(QVirtioDevice *d) +{ + return 0; +} + +static void qvirtio_pci_set_status(QVirtioDevice *d, uint8_t val) +{ + +} + +static void qvirtio_pci_reset(QVirtioDevice *d) +{ + +} + +static uint8_t qvirtio_pci_query_isr(QVirtioDevice *d) +{ + return 0; +} + +static void qvirtio_pci_foreach(uint16_t device_type, + void (*func)(QVirtioDevice *d, void *data), void *data) +{ + QVirtioPCIForeachData d = { .func = func, + .device_type = device_type, + .user_data = data }; + + if (!bus) { + bus = qpci_init_pc(); + } + + qpci_device_foreach(bus, QVIRTIO_VENDOR_ID, -1, + qvirtio_pci_foreach_callback, &d); +} + +static QVirtioDevice *qvirtio_pci_device_find(uint16_t device_type) +{ + QVirtioPCIDevice *dev; + + dev = g_malloc0(sizeof(*dev)); + qvirtio_pci_foreach(device_type, qvirtio_pci_assign_device, dev); + + return (QVirtioDevice *)dev; +} + +const QVirtioBus qvirtio_pci = { + .notify = qvirtio_pci_notify, + .get_config = qvirtio_pci_get_config, + .set_config = qvirtio_pci_set_config, + .get_features = qvirtio_pci_get_features, + .get_status = qvirtio_pci_get_status, + .set_status = qvirtio_pci_set_status, + .reset = qvirtio_pci_reset, + .query_isr = qvirtio_pci_query_isr, + .bus_foreach = qvirtio_pci_foreach, + .device_find = qvirtio_pci_device_find, +}; + diff --git a/tests/libqos/virtio-pci.h b/tests/libqos/virtio-pci.h new file mode 100644 index 0000000..e5812af --- /dev/null +++ b/tests/libqos/virtio-pci.h @@ -0,0 +1,29 @@ +/* + * libqos virtio PCI definitions + * + * 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. + */ + +#ifndef LIBQOS_VIRTIO_PCI_H +#define LIBQOS_VIRTIO_PCI_H + +#include "libqos/virtio.h" +#include "libqos/pci.h" + +typedef struct QVirtioPCIDevice { + QVirtioDevice vdev; + QPCIDevice *pdev; +} QVirtioPCIDevice; + +typedef struct QVirtioPCIForeachData { + void (*func)(QVirtioDevice *d, void *data); + uint16_t device_type; + void *user_data; +} QVirtioPCIForeachData; + +extern const QVirtioBus qvirtio_pci; + +#endif diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h new file mode 100644 index 0000000..43a0e73 --- /dev/null +++ b/tests/libqos/virtio.h @@ -0,0 +1,64 @@ +/* + * libqos virtio definitions + * + * 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. + */ + +#ifndef LIBQOS_VIRTIO_H +#define LIBQOS_VIRTIO_H + +#define QVIRTQUEUE_MAX_SIZE 1024 +#define QVIRTIO_VENDOR_ID 0x1AF4 + +#define QVIRTIO_NET_DEVICE_ID 0x1 +#define QVIRTIO_BLK_DEVICE_ID 0x2 + +typedef struct QVirtioDevice QVirtioDevice; +typedef struct QVirtQueue QVirtQueue; +typedef struct QVRing QVRing; + +typedef struct QVirtioDevice { + /* Device type */ + uint16_t device_type; + + /* VQs associated with the device */ + QVirtQueue *vq; +} QVirtioDevice; + +typedef struct QVirtioBus { + /* Send a notification IRQ to the device */ + void (*notify)(QVirtioDevice *d, uint16_t vector); + + /* 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); + + /* Loop all devices, applying a function to all, or the one specified */ + void (*bus_foreach)(uint16_t device_t, + void (*func)(QVirtioDevice *d, void *data), void *data); + + /* Find and return a device */ + QVirtioDevice *(*device_find)(uint16_t device_type); +} QVirtioBus; + +#endif diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c index d53f875..572e80e 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,52 @@ #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 "libqos/virtio-pci.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"; + +static void pci_basic(void) { + QVirtioPCIDevice *dev; + dev = (QVirtioPCIDevice *)qvirtio_pci.device_find(QVIRTIO_BLK_DEVICE_ID); + g_assert_cmphex(dev->vdev.device_type, ==, QVIRTIO_BLK_DEVICE_ID); + fprintf(stderr, "Device position: %x. Device type: %x\n", + dev->pdev->devfn, dev->vdev.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); + + /* TODO: Start new machine (new block device) at each function */ + snprintf(test_start, 100, "-drive if=none,id=drive0,file=%s " + "-device virtio-blk-pci,drive=drive0", tmp_path); + qtest_start(test_start); ret = g_test_run(); qtest_end(); + /* Cleanup */ + unlink(tmp_path); + return ret; }
Virtio header has been changed to compile and work with a real device. Functions bus_foreach and device_find have been implemented for PCI. Virtio-blk test case now opens a fake device. Signed-off-by: Marc Marí <marc.mari.barcelo@gmail.com> --- tests/Makefile | 3 +- tests/libqos/virtio-pci.c | 136 +++++++++++++++++++++++++++++++++++++++++++++ tests/libqos/virtio-pci.h | 29 ++++++++++ tests/libqos/virtio.h | 64 +++++++++++++++++++++ tests/virtio-blk-test.c | 39 +++++++++++-- 5 files changed, 264 insertions(+), 7 deletions(-) create mode 100644 tests/libqos/virtio-pci.c create mode 100644 tests/libqos/virtio-pci.h create mode 100644 tests/libqos/virtio.h