From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55544) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X1WM9-0005Yi-GU for qemu-devel@nongnu.org; Mon, 30 Jun 2014 03:50:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X1WM1-0000qJ-VV for qemu-devel@nongnu.org; Mon, 30 Jun 2014 03:50:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:64734) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X1WM1-0000oI-NP for qemu-devel@nongnu.org; Mon, 30 Jun 2014 03:50:21 -0400 Date: Mon, 30 Jun 2014 09:50:17 +0200 From: Stefan Hajnoczi Message-ID: <20140630075017.GA30969@stefanha-thinkpad.redhat.com> References: <1403793280-4353-1-git-send-email-marc.mari.barcelo@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="17pEHd4RhPHOinZp" Content-Disposition: inline In-Reply-To: <1403793280-4353-1-git-send-email-marc.mari.barcelo@gmail.com> Subject: Re: [Qemu-devel] [PATCH] Functions bus_foreach and device_find from libqos virtio API List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marc =?iso-8859-1?Q?Mar=ED?= Cc: Paolo Bonzini , qemu-devel@nongnu.org --17pEHd4RhPHOinZp Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jun 26, 2014 at 04:34:40PM +0200, Marc Mar=ED wrote: > +static QVirtioPCIDevice *qpcidevice_to_qvirtiodevice(QPCIDevice *pdev) > +{ > + QVirtioPCIDevice *vpcidev; > + vpcidev =3D g_malloc0(sizeof(*vpcidev)); > + > + if (pdev) { > + vpcidev->pdev =3D pdev; > + vpcidev->vdev.device_type =3D > + qpci_config_readw(vpcidev->pdev, PCI_SUBSYST= EM_ID); > + /* TODO: When QVirtQueue is defined, change for > + g_malloc0(sizeof(QVirtQueue)); */ > + vpcidev->vdev.vq =3D 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 =3D data; > + QVirtioPCIDevice *vpcidev =3D qpcidevice_to_qvirtiodevice(dev); > + > + if (vpcidev->vdev.device_type =3D=3D d->device_type) { > + d->func(&vpcidev->vdev, d->user_data); > + } > +} > + > +static void qvirtio_pci_assign_device(QVirtioDevice *d, void *data) > +{ > + QVirtioPCIDevice *vpcidev =3D data; > + vpcidev->pdev =3D ((QVirtioPCIDevice *)d)->pdev; > + vpcidev->vdev.device_type =3D ((QVirtioPCIDevice *)d)->vdev.device= _type; > + vpcidev->vdev.vq =3D ((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 =3D { .func =3D func, > + .device_type =3D device_type, > + .user_data =3D data }; > + > + if (!bus) { > + bus =3D 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 =3D g_malloc0(sizeof(*dev)); > + qvirtio_pci_foreach(device_type, qvirtio_pci_assign_device, dev); > + > + return (QVirtioDevice *)dev; > +} > + > +const QVirtioBus qvirtio_pci =3D { > + .notify =3D qvirtio_pci_notify, > + .get_config =3D qvirtio_pci_get_config, > + .set_config =3D qvirtio_pci_set_config, > + .get_features =3D qvirtio_pci_get_features, > + .get_status =3D qvirtio_pci_get_status, > + .set_status =3D qvirtio_pci_set_status, > + .reset =3D qvirtio_pci_reset, > + .query_isr =3D qvirtio_pci_query_isr, > + .bus_foreach =3D qvirtio_pci_foreach, > + .device_find =3D 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=ED > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or la= ter. > + * 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=ED > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or la= ter. > + * 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 specifie= d */ > + 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 =3D (QVirtioPCIDevice *)qvirtio_pci.device_find(QVIRTIO_BLK_DEVI= CE_ID); > + g_assert_cmphex(dev->vdev.device_type, =3D=3D, QVIRTIO_BLK_DEVICE_ID= ); > + fprintf(stderr, "Device position: %x. Device type: %x\n", > + dev->pdev->devfn, dev->vdev.device_t= ype); The test case should not produce output. Please drop the fprintf(). --17pEHd4RhPHOinZp Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJTsRa5AAoJEJykq7OBq3PI3SMH/A2tNbTFXtxfF5EC+vpOgOSb 03nsbmHunva1i993Kafo0SnT+C1w9hwtAbHqOOIu+92orbfFrhAeT8FFSoHrgBCd YyflpR3wuxGELP7NfXoiCE8sag/gBWr36wwo11MVWNQh3UPiqzXp9MGdT7EH7+CA ZgXejzmSKZbzGtZbavP0/14HFoC96EfYhN9/tqDntvy+TlDufqSIz07iYbMAO+iE sVsZOAhVUllC+6X8wBAedcXkP0k9Cq9vdreLbNG7bHNI7l3GIRJn+2xhGn8hh52l i4mv9se6LNHOwaTjN+Fo0/YmyZe3wODObwI60b25qNfV6qmmG2CdYhWkq4O8GKk= =VcVU -----END PGP SIGNATURE----- --17pEHd4RhPHOinZp--