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

[Stefan Hajnoczi]

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().

pgpo1AenQDWeS.pgp
Description: PGP signature