Re: [Qemu-devel] [RFC PATCH v2 1/3] virtio-bus : Introduce VirtioBus.

[Anthony Liguori]

address@hidden writes:

> From: KONRAD Frederic <address@hidden>
>
> This patch create a new VirtioBus, which can be added to Virtio transports 
> like
> virtio-pci, virtio-mmio,...
>
> One VirtIODevice can be connected to this device, like virtio-blk in the 3rd
> patch.
>
> The VirtioBus shares through a VirtioBusInfo structure :
>
>     * two callbacks with the transport : init_cb and exit_cb, which must be
>       called by the VirtIODevice, after the initialization and before the
>       destruction, to put the right PCI IDs and/or stop the event fd.
>
>     * a VirtIOBindings structure.
>
> Signed-off-by: KONRAD Frederic <address@hidden>
> ---
>  hw/Makefile.objs |   1 +
>  hw/virtio-bus.c  | 148 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/virtio-bus.h  |  58 ++++++++++++++++++++++
>  3 files changed, 207 insertions(+)
>  create mode 100644 hw/virtio-bus.c
>  create mode 100644 hw/virtio-bus.h
>
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index ea46f81..bd14d1b 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -2,6 +2,7 @@ common-obj-y = usb/ ide/
>  common-obj-y += loader.o
>  common-obj-$(CONFIG_VIRTIO) += virtio-console.o
>  common-obj-$(CONFIG_VIRTIO) += virtio-rng.o
> +common-obj-$(CONFIG_VIRTIO) += virtio-bus.o
>  common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
>  common-obj-y += fw_cfg.o
>  common-obj-$(CONFIG_PCI) += pci.o pci_bridge.o pci_bridge_dev.o
> diff --git a/hw/virtio-bus.c b/hw/virtio-bus.c
> new file mode 100644
> index 0000000..991b6f5
> --- /dev/null
> +++ b/hw/virtio-bus.c
> @@ -0,0 +1,148 @@
> +/*
> + * VirtioBus
> + *
> + *  Copyright (C) 2012 : GreenSocs Ltd
> + *      http://www.greensocs.com/ , email: address@hidden
> + *
> + *  Developed by :
> + *  Frederic Konrad   <address@hidden>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "hw.h"
> +#include "qemu-error.h"
> +#include "qdev.h"
> +#include "virtio-bus.h"
> +#include "virtio.h"
> +
> +#define DEBUG_VIRTIO_BUS 1
> +
> +#define DPRINTF(fmt, ...) if (DEBUG_VIRTIO_BUS) {                        \
> +                            printf("virtio_bus: " fmt , ## __VA_ARGS__); \
> +                          }

#ifdef DEBUG_VIRTIO_BUS
#define DPRINTF(fmt, ...) ...
#else
#define DPRINTF(fmt, ...) do { } while (0)
#endif

You're leaving a dangling if clause which can do very strange things if
used before an else statement.

> +
> +static void virtio_bus_init_cb(VirtioBus *bus);
> +static int virtio_bus_reset(BusState *qbus);

You should rearrange the code to avoid forward declarations of static functions.

> +
> +static void virtio_bus_class_init(ObjectClass *klass, void *data)
> +{
> +    BusClass *k = BUS_CLASS(klass);
> +    k->reset = virtio_bus_reset;
> +}
> +
> +static TypeInfo virtio_bus_info = {
> +    .name = TYPE_VIRTIO_BUS,
> +    .parent = TYPE_BUS,
> +    .instance_size = sizeof(VirtioBus),
> +    .class_init = virtio_bus_class_init,
> +};
> +
> +/* Reset the bus */
> +static int virtio_bus_reset(BusState *qbus)
> +{
> +    VirtioBus *bus = VIRTIO_BUS(qbus);
> +    if (bus->bus_in_use) {
> +        virtio_reset(bus->vdev);
> +    }
> +    return 1;
> +}
> +
> +/* Plug the VirtIODevice */
> +int virtio_bus_plug_device(VirtioBus *bus, VirtIODevice *vdev)
> +{
> +    BusState *qbus = BUS(bus);
> +    /*
> +     * This is a problem, when bus= option is not set, the last created
> +     * virtio-bus is used. So it give the following error.
> +     */
> +    DPRINTF("plug device into %s.\n", qbus->name);
> +    if (bus->bus_in_use) {
> +        error_report("%s in use.\n", qbus->name);
> +        return -1;
> +    }
> +    bus->bus_in_use = true;
> +
> +    /* keep the VirtIODevice in the VirtioBus. */
> +    bus->vdev = vdev;
> +
> +    /* call the "transport" callback. */
> +    virtio_bus_init_cb(bus);
> +    return 0;
> +}

I think the desired semantics here could be achieved by modifying
qbus_find_recursive() to take an optional argument which then causes it
to only return a "!full" bus.  You can then add a member to BusState to
indicate whether the bus is full or not.

You can then set the parameter qdev_device_add() and it should Just Work.

> +
> +/* Create a virtio bus.  */
> +VirtioBus *virtio_bus_new(DeviceState *host, const VirtioBusInfo *info)
> +{
> +    /*
> +     * This is needed, as we want to have different names for each 
> virtio-bus.
> +     * If we don't do that, we can't add more than one VirtIODevice.
> +     */
> +    static int next_virtio_bus;
> +    char *bus_name = g_strdup_printf("virtio-bus.%d",
> next_virtio_bus++);

It's not needed..  qdev will do this automagically as it turns out.

> +
> +    BusState *qbus = qbus_create(TYPE_VIRTIO_BUS, host, bus_name);
> +    VirtioBus *bus = VIRTIO_BUS(qbus);
> +    bus->info = info;
> +    qbus->allow_hotplug = 0;
> +    bus->bus_in_use = false;
> +    DPRINTF("%s bus created\n", bus_name);
> +    return bus;
> +}
> +
> +/* Bind the VirtIODevice to the VirtioBus. */
> +void virtio_bus_bind_device(VirtioBus *bus)
> +{
> +    BusState *qbus = BUS(bus);
> +    assert(bus != NULL);
> +    assert(bus->vdev != NULL);
> +    virtio_bind_device(bus->vdev, &(bus->info->virtio_bindings), 
> qbus->parent);
> +}
> +
> +/*
> + * Transport independent init.
> + * This must be called after VirtIODevice initialization.
> + */
> +static void virtio_bus_init_cb(VirtioBus *bus)
> +{
> +    BusState *qbus = BUS(bus);
> +    assert(bus->info->init_cb != NULL);
> +    bus->info->init_cb(qbus->parent);
> +}
> +
> +/*
> + * Transport independent exit.
> + * This must be called by the VirtIODevice before destroying it.
> + */
> +void virtio_bus_exit_cb(VirtioBus *bus)
> +{
> +    BusState *qbus = BUS(bus);
> +    assert(bus->info->exit_cb != NULL);
> +    bus->info->exit_cb(qbus->parent);
> +    bus->bus_in_use = false;
> +}
> +
> +/* Return the virtio device id of the plugged device. */
> +uint16_t get_virtio_device_id(VirtioBus *bus)
> +{
> +    return bus->vdev->device_id;
> +}
> +
> +static void virtio_register_types(void)
> +{
> +    type_register_static(&virtio_bus_info);
> +}
> +
> +type_init(virtio_register_types)
> diff --git a/hw/virtio-bus.h b/hw/virtio-bus.h
> new file mode 100644
> index 0000000..31aad53
> --- /dev/null
> +++ b/hw/virtio-bus.h
> @@ -0,0 +1,58 @@
> +/*
> + * VirtioBus
> + *
> + *  Copyright (C) 2012 : GreenSocs Ltd
> + *      http://www.greensocs.com/ , email: address@hidden
> + *
> + *  Developed by :
> + *  Frederic Konrad   <address@hidden>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#ifndef VIRTIO_BUS_H
> +#define VIRTIO_BUS_H
> +
> +#include "qdev.h"
> +#include "sysemu.h"
> +#include "virtio.h"
> +
> +#define TYPE_VIRTIO_BUS "virtio-bus"
> +#define VIRTIO_BUS(obj) OBJECT_CHECK(VirtioBus, (obj), TYPE_VIRTIO_BUS)
> +
> +typedef struct VirtioBus VirtioBus;
> +typedef struct VirtioBusInfo VirtioBusInfo;
> +
> +struct VirtioBusInfo {
> +    void (*init_cb)(DeviceState *dev);
> +    void (*exit_cb)(DeviceState *dev);

Don't suffix methods with '_cb'.

VirtioBusInfo is not a great name.  This is a proxy class that allows
for a device to implement the virtio bus interface.

This could be done as an interface but since nothing else uses
interfaces, I'm okay with something like this.  But the first argument
ought to be an opaque for all methods.

But it should be called something like VirtioBusProxy.

BTW, comments describing the role of this struct would be very helpful.

> +    VirtIOBindings virtio_bindings;

Everything that's in VirtIOBindings should be methods of the
VirtioBusClass (which needs to be defined here).

Regards,

Anthony Liguori

> +};
> +
> +struct VirtioBus {
> +    BusState qbus;
> +    bool bus_in_use;
> +    /* Only one VirtIODevice can be plugged on the bus. */
> +    VirtIODevice *vdev;
> +    const VirtioBusInfo *info;
> +};
> +
> +VirtioBus *virtio_bus_new(DeviceState *host, const VirtioBusInfo *info);
> +void virtio_bus_bind_device(VirtioBus *bus);
> +int virtio_bus_plug_device(VirtioBus *bus, VirtIODevice *vdev);
> +void virtio_bus_exit_cb(VirtioBus *bus);
> +uint16_t get_virtio_device_id(VirtioBus *bus);
> +
> +#endif /* VIRTIO_BUS_H */
> -- 
> 1.7.11.7