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

["KONRAD Frédéric" <fred.konrad@greensocs.com>]

On 21/11/2012 14:04, Andreas Färber wrote:
> Am 16.11.2012 16:35, schrieb fred.konrad@greensocs.com:
>> 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  | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   hw/virtio-bus.h  |  49 ++++++++++++++++++++++++
>>   3 files changed, 161 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 af4ab0c..1b25d77 100644
>> --- a/hw/Makefile.objs
>> +++ b/hw/Makefile.objs
>> @@ -1,6 +1,7 @@
>>   common-obj-y = usb/ ide/
>>   common-obj-y += loader.o
>>   common-obj-$(CONFIG_VIRTIO) += virtio-console.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..b2e7e9c
>> --- /dev/null
>> +++ b/hw/virtio-bus.c
>> @@ -0,0 +1,111 @@
>> +/*
>> + * VirtioBus
>> + *
>> + *  Copyright (C) 2012 : GreenSocs Ltd
>> + *      http://www.greensocs.com/ , email: info@greensocs.com
>> + *
>> + *  Developed by :
>> + *  Frederic Konrad   <fred.konrad@greensocs.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>> + * the COPYING file in the top-level directory.
> Any chance to use GPLv2 "or (at your option) any later version"? If not,
> please mention in the commit message why.
>
Yes, I made the change.
>> + */
>> +
>> +#include "hw.h"
>> +#include "qemu-error.h"
>> +#include "qdev.h"
>> +#include "virtio-bus.h"
>> +#include "virtio.h"
>> +
>> +#define DEBUG_VIRTIO_BUS
>> +
>> +#ifdef DEBUG_VIRTIO_BUS
>> +
>> +#define DPRINTF(fmt, ...) \
>> +do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
>> +#else
>> +#define DPRINTF(fmt, ...) do {} while (0)
>> +#endif
> We recently had a discussion about bitrotting DPRINTF() statements where
> I suggested to use if (0) instead of a no-op macro like this that
> doesn't reference fmt and the varargs.
I don't understand what you suggested, can you point me to an example ?

>> +
>> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev);
>> +
>> +static void virtio_bus_class_init(ObjectClass *klass, void *data)
>> +{
>> +    BusClass *k = BUS_CLASS(klass);
>> +    k->get_fw_dev_path = virtio_bus_get_fw_dev_path;
>> +}
>> +
>> +static const TypeInfo virtio_bus_info = {
>> +    .name = TYPE_VIRTIO_BUS,
>> +    .parent = TYPE_BUS,
>> +    .instance_size = sizeof(VirtioBus),
>> +    .class_init = virtio_bus_class_init,
>> +};
>> +
>> +/* VirtioBus */
>> +
>> +static int next_virtio_bus;
>> +
>> +/* Create a virtio bus, and attach to transport.  */
>> +void virtio_bus_new(VirtioBus *bus, DeviceState *host,
>> +                    const VirtioBusInfo *info)
>> +{
>> +    /*
>> +     * Setting name to NULL return always "virtio.0"
>> +     * as bus name in info qtree.
>> +     */
>> +    char *bus_name = g_strdup_printf("%s.%d", BUS_NAME, next_virtio_bus);
>> +    qbus_create_inplace(&bus->qbus, TYPE_VIRTIO_BUS, host, bus_name);
> Accessing the qbus field directly is considered old-style. Please use
> the BUS() macro to cast to a new BusState* variable and pass that here...
Ok, I'll look.

>> +    bus->busnr = next_virtio_bus++;
>> +    bus->info = info;
>> +    /* no hotplug for the moment ? */
>> +    bus->qbus.allow_hotplug = 0;
> ...and access its fields through it. More cases below.
>
>> +    bus->bus_in_use = false;
>> +    DPRINTF("bus %s created\n", bus_name);
>> +}
>> +
>> +/* Bind the VirtIODevice to the VirtioBus. */
>> +void virtio_bus_bind_device(VirtioBus *bus)
>> +{
>> +    assert(bus != NULL);
>> +    assert(bus->vdev != NULL);
>> +    virtio_bind_device(bus->vdev, &(bus->info->virtio_bindings),
>> +                       bus->qbus.parent);
>> +}
>> +
>> +/* This must be called to when the VirtIODevice init */
> "called when the ...Device inits" or "called during ...Device init"
oops sorry for that.

>> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *vdev)
>> +{
>> +    if (bus->bus_in_use == true) {
> Even if bus_in_use is of bool type, doing an explicit "true" comparison
> is not a good idea in C since everything non-zero is to be considered
> true, not just the "true" constant.
sure.
>> +        error_report("%s in use.\n", bus->qbus.name);
>> +        return -1;
>> +    }
>> +    assert(bus->info->init_cb != NULL);
>> +    /* keep the VirtIODevice in the VirtioBus */
>> +    bus->vdev = vdev;
>> +    bus->info->init_cb(bus->qbus.parent);
>> +
>> +    bus->bus_in_use = true;
>> +    return 0;
>> +}
>> +
>> +/* This must be called when the VirtIODevice exit */
> Similar grammar issue as above.
>
>> +void virtio_bus_exit_cb(VirtioBus *bus)
>> +{
>> +    assert(bus->info->exit_cb != NULL);
>> +    bus->info->exit_cb(bus->qbus.parent);
>> +    bus->bus_in_use = false;
>> +}
>> +
>> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev)
>> +{
>> +    return g_strdup_printf("%s", qdev_fw_name(dev));
>> +}
>> +
>> +
>> +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..6ea5035
>> --- /dev/null
>> +++ b/hw/virtio-bus.h
>> @@ -0,0 +1,49 @@
>> +/*
>> + * VirtioBus
>> + *
>> + *  Copyright (C) 2012 : GreenSocs Ltd
>> + *      http://www.greensocs.com/ , email: info@greensocs.com
>> + *
>> + *  Developed by :
>> + *  Frederic Konrad   <fred.konrad@greensocs.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>> + * the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef _VIRTIO_BUS_H_
>> +#define _VIRTIO_BUS_H_
>> +
>> +#include "qdev.h"
>> +#include "sysemu.h"
>> +#include "virtio.h"
>> +
>> +#define TYPE_VIRTIO_BUS "VIRTIO"
> Is there a special reason for all-uppercase here? Is lowercase already
> taken?
No, there are no reasons, it was the case for scsi-bus.

>
> You should add QOM cast macros VIRTIO_BUS() et al. to access the
> VirtioBus fields.
>
>> +#define BUS_NAME "virtio"
>> +
>> +typedef struct VirtioBus VirtioBus;
>> +typedef struct VirtioBusInfo VirtioBusInfo;
>> +
>> +struct VirtioBusInfo {
>> +    void (*init_cb)(DeviceState *dev);
>> +    void (*exit_cb)(DeviceState *dev);
>> +    VirtIOBindings virtio_bindings;
>> +};
>> +
>> +struct VirtioBus {
>> +    BusState qbus;
> You could name this field parent_obj to break the old qbus pattern.
>
>> +    int busnr;
>> +    bool bus_in_use;
>> +    uint16_t pci_device_id;
>> +    uint16_t pci_class;
> pci_* in VirtioBus does not strike me as the best naming. Either this is
> specific to the PCI backend and doesn't belong here, or it's not a
> pci_device_id but a device_id.
it is specific to the PCI backend, and I removed it.

>
>> +    VirtIODevice *vdev;
>> +    const VirtioBusInfo *info;
>> +};
>> +
>> +void virtio_bus_new(VirtioBus *bus, DeviceState *host,
>> +                    const VirtioBusInfo *info);
>> +void virtio_bus_bind_device(VirtioBus *bus);
>> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *dev);
>> +void virtio_bus_exit_cb(VirtioBus *bus);
>> +
>> +#endif
>>
> Regards,
> Andreas
>
Thanks,

Fred