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

[Konrad Frederic <fred.konrad@greensocs.com>]

On 26/11/2012 15:33, Anthony Liguori wrote:
> fred.konrad@greensocs.com writes:
>
>> From: KONRAD Frederic<fred.konrad@greensocs.com>
>>
>> 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<fred.konrad@greensocs.com>
>> ---
>>   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: info@greensocs.com
>> + *
>> + *  Developed by :
>> + *  Frederic Konrad<fred.konrad@greensocs.com>
>> + *
>> + * 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.
Ok, so I need to modify qdev_monitor.c to add an option ( in command 
line ) to set this optional argument ?

>> +
>> +/* 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.
Really ? If I use qbus_create(TYPE_VIRTIO_BUS, host, NULL) and I add two 
virtio-pci in qemu monitor, I have two virtio-bus named "virtio-bus.0".

Did I miss something ?
>> +
>> +    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: info@greensocs.com
>> + *
>> + *  Developed by :
>> + *  Frederic Konrad<fred.konrad@greensocs.com>
>> + *
>> + * 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).
Ok, so I can put all the virtio_bindings "methods" in this VirtioBusProxy ?

>
> Regards,
>
> Anthony Liguori
Thanks,

Fred

>
>> +};
>> +
>> +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