Patchwork [RFC,1/3] virtio-bus : Introduce VirtioBus.

login
register
mail settings
Submitter fred.konrad@greensocs.com
Date Nov. 16, 2012, 3:35 p.m.
Message ID <1353080159-10536-2-git-send-email-fred.konrad@greensocs.com>
Download mbox | patch
Permalink /patch/199663/
State New
Headers show

Comments

fred.konrad@greensocs.com - Nov. 16, 2012, 3:35 p.m.
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
Peter Maydell - Nov. 19, 2012, 5:33 p.m.
On 16 November 2012 15:35,  <fred.konrad@greensocs.com> wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>

You forgot to CC enough people...

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

Unless you copied this code from an existing v2-only file, preferred
license for new code is v2-or-any-later-version.

> + */
> +
> +#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
> +
> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev);

Is this function necessary? I think your implementation
is doing the same as the default.

> +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);
> +    bus->busnr = next_virtio_bus++;

This looks suspicious to me -- is keeping a count of the global
bus number really the right way to do this?

> +    bus->info = info;
> +    /* no hotplug for the moment ? */
> +    bus->qbus.allow_hotplug = 0;

Correct -- we don't need to hotplug the virtio backend.

> +    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 looks wrong -- virtio_bind_device() is part of the
old-style transport API.

> +}
> +
> +/* This must be called to when the VirtIODevice init */
> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *vdev)
> +{
> +    if (bus->bus_in_use == true) {
> +        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;

This shouldn't be happening here, it should happen as
part of plugging the device into the bus.

> +    bus->info->init_cb(bus->qbus.parent);
> +
> +    bus->bus_in_use = true;
> +    return 0;
> +}
> +
> +/* This must be called when the VirtIODevice exit */
> +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;
> +}

These shouldn't be necessary -- the VirtioDevice should
have a pointer to the VirtioBus and can just invoke the
init/exit callbacks directly.

> +
> +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_

Leading underscores are reserved by the C standard. VIRTIO_BUS_H will
do fine.

> +
> +#include "qdev.h"
> +#include "sysemu.h"
> +#include "virtio.h"
> +
> +#define TYPE_VIRTIO_BUS "VIRTIO"

Type names are generally "virtio-bus" by convention.

> +#define BUS_NAME "virtio"

I don't think you want this.


> +typedef struct VirtioBus VirtioBus;
> +typedef struct VirtioBusInfo VirtioBusInfo;
> +
> +struct VirtioBusInfo {
> +    void (*init_cb)(DeviceState *dev);
> +    void (*exit_cb)(DeviceState *dev);
> +    VirtIOBindings virtio_bindings;

This doesn't look right -- VirtIOBindings is the
old-style way for the transport to specify a bunch
of function pointers for its specific implementation.
Those function pointers should probably just be in
the VirtioBus struct.

> +};

I was just talking to Anthony on IRC and he said SCSIBusInfo
is really kind of a historical accident. So we can just fold
this struct into VirtioBus. Sorry for misleading you earlier.

> +struct VirtioBus {
> +    BusState qbus;
> +    int busnr;

Why does the bus need to know what number it is?

> +    bool bus_in_use;
> +    uint16_t pci_device_id;
> +    uint16_t pci_class;

This shouldn't be here -- VirtioBus should be transport
independent, so no PCI related info.

> +    VirtIODevice *vdev;

This could use a comment saying that we only ever have one
child device on the bus.

> +    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
> --
> 1.7.11.7

-- PMM
Cornelia Huck - Nov. 20, 2012, 2:12 p.m.
On Mon, 19 Nov 2012 17:33:01 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 16 November 2012 15:35,  <fred.konrad@greensocs.com> wrote:
> > From: KONRAD Frederic <fred.konrad@greensocs.com>
> 
> You forgot to CC enough people...
> 
> > 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.

Can we specify the general purpose of the {init,exit}_cb a bit better?
Is it analogous to any of the functions performed by the transport
busses today?

> >
> >     * 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.
> 
> Unless you copied this code from an existing v2-only file, preferred
> license for new code is v2-or-any-later-version.
> 
> > + */
> > +
> > +#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
> > +
> > +static char *virtio_bus_get_fw_dev_path(DeviceState *dev);
> 
> Is this function necessary? I think your implementation
> is doing the same as the default.
> 
> > +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);
> > +    bus->busnr = next_virtio_bus++;
> 
> This looks suspicious to me -- is keeping a count of the global
> bus number really the right way to do this?

If we want an unique number, we need to track usage of numbers, else we
end up with duplicates on wraparound.

> 
> > +    bus->info = info;
> > +    /* no hotplug for the moment ? */
> > +    bus->qbus.allow_hotplug = 0;
> 
> Correct -- we don't need to hotplug the virtio backend.
> 
> > +    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 looks wrong -- virtio_bind_device() is part of the
> old-style transport API.
> 
> > +}
> > +
> > +/* This must be called to when the VirtIODevice init */
> > +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *vdev)
> > +{
> > +    if (bus->bus_in_use == true) {
> > +        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;
> 
> This shouldn't be happening here, it should happen as
> part of plugging the device into the bus.
> 
> > +    bus->info->init_cb(bus->qbus.parent);
> > +
> > +    bus->bus_in_use = true;
> > +    return 0;
> > +}
> > +
> > +/* This must be called when the VirtIODevice exit */
> > +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;
> > +}
> 
> These shouldn't be necessary -- the VirtioDevice should
> have a pointer to the VirtioBus and can just invoke the
> init/exit callbacks directly.
> 
> > +
> > +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_
> 
> Leading underscores are reserved by the C standard. VIRTIO_BUS_H will
> do fine.
> 
> > +
> > +#include "qdev.h"
> > +#include "sysemu.h"
> > +#include "virtio.h"
> > +
> > +#define TYPE_VIRTIO_BUS "VIRTIO"
> 
> Type names are generally "virtio-bus" by convention.
> 
> > +#define BUS_NAME "virtio"
> 
> I don't think you want this.
> 
> 
> > +typedef struct VirtioBus VirtioBus;
> > +typedef struct VirtioBusInfo VirtioBusInfo;
> > +
> > +struct VirtioBusInfo {
> > +    void (*init_cb)(DeviceState *dev);
> > +    void (*exit_cb)(DeviceState *dev);
> > +    VirtIOBindings virtio_bindings;
> 
> This doesn't look right -- VirtIOBindings is the
> old-style way for the transport to specify a bunch
> of function pointers for its specific implementation.
> Those function pointers should probably just be in
> the VirtioBus struct.
> 
> > +};
> 
> I was just talking to Anthony on IRC and he said SCSIBusInfo
> is really kind of a historical accident. So we can just fold
> this struct into VirtioBus. Sorry for misleading you earlier.
> 
> > +struct VirtioBus {
> > +    BusState qbus;
> > +    int busnr;
> 
> Why does the bus need to know what number it is?
> 
> > +    bool bus_in_use;
> > +    uint16_t pci_device_id;
> > +    uint16_t pci_class;
> 
> This shouldn't be here -- VirtioBus should be transport
> independent, so no PCI related info.

Agreed - this should be a property of the bridge device.

> 
> > +    VirtIODevice *vdev;
> 
> This could use a comment saying that we only ever have one
> child device on the bus.
> 
> > +    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
> > --
> > 1.7.11.7
> 
> -- PMM
>
fred.konrad@greensocs.com - Nov. 20, 2012, 2:30 p.m.
On 20/11/2012 15:12, Cornelia Huck wrote:
> On Mon, 19 Nov 2012 17:33:01 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> On 16 November 2012 15:35,  <fred.konrad@greensocs.com> wrote:
>>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>> You forgot to CC enough people...
>>
>>> 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.
> Can we specify the general purpose of the {init,exit}_cb a bit better?
> Is it analogous to any of the functions performed by the transport
> busses today?
For example, look at the function static int 
virtio_blk_init_pci(PCIDevice *pci_dev) in virtio-pci.c,

it creates the VirtIODevice and then call virtio_init_pci(proxy, vdev);

It is what I tryed to reproduce, and abstract it from the new device.

eg : for the new virtio-blk I add, in the function static int 
virtio_blk_qdev_init(DeviceState *qdev) in virtio-blk.c :

it creates the VirtIODevice, create the virtiobus and call the 
virtio_bus_init_cb(bus, vdev)
  which call the virtio_init_pci callback when virtio-pci bridge is used.

it is the same thing for the exit.

Is it making sense ?

>
>>>      * 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.
>> Unless you copied this code from an existing v2-only file, preferred
>> license for new code is v2-or-any-later-version.
>>
>>> + */
>>> +
>>> +#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
>>> +
>>> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev);
>> Is this function necessary? I think your implementation
>> is doing the same as the default.
>>
>>> +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);
>>> +    bus->busnr = next_virtio_bus++;
>> This looks suspicious to me -- is keeping a count of the global
>> bus number really the right way to do this?
> If we want an unique number, we need to track usage of numbers, else we
> end up with duplicates on wraparound.
I put that because the number was all identical in "info qtree", is it 
the right thing to do ?

>>> +    bus->info = info;
>>> +    /* no hotplug for the moment ? */
>>> +    bus->qbus.allow_hotplug = 0;
>> Correct -- we don't need to hotplug the virtio backend.
>>
>>> +    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 looks wrong -- virtio_bind_device() is part of the
>> old-style transport API.
>>


>>> +}
>>> +
>>> +/* This must be called to when the VirtIODevice init */
>>> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *vdev)
>>> +{
>>> +    if (bus->bus_in_use == true) {
>>> +        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;
>> This shouldn't be happening here, it should happen as
>> part of plugging the device into the bus.
>>
>>> +    bus->info->init_cb(bus->qbus.parent);
>>> +
>>> +    bus->bus_in_use = true;
>>> +    return 0;
>>> +}
>>> +
>>> +/* This must be called when the VirtIODevice exit */
>>> +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;
>>> +}
>> These shouldn't be necessary -- the VirtioDevice should
>> have a pointer to the VirtioBus and can just invoke the
>> init/exit callbacks directly.
>>
>>> +
>>> +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_
>> Leading underscores are reserved by the C standard. VIRTIO_BUS_H will
>> do fine.
>>
>>> +
>>> +#include "qdev.h"
>>> +#include "sysemu.h"
>>> +#include "virtio.h"
>>> +
>>> +#define TYPE_VIRTIO_BUS "VIRTIO"
>> Type names are generally "virtio-bus" by convention.
>>
>>> +#define BUS_NAME "virtio"
>> I don't think you want this.
>>
>>
>>> +typedef struct VirtioBus VirtioBus;
>>> +typedef struct VirtioBusInfo VirtioBusInfo;
>>> +
>>> +struct VirtioBusInfo {
>>> +    void (*init_cb)(DeviceState *dev);
>>> +    void (*exit_cb)(DeviceState *dev);
>>> +    VirtIOBindings virtio_bindings;
>> This doesn't look right -- VirtIOBindings is the
>> old-style way for the transport to specify a bunch
>> of function pointers for its specific implementation.
>> Those function pointers should probably just be in
>> the VirtioBus struct.
>>
>>> +};
>> I was just talking to Anthony on IRC and he said SCSIBusInfo
>> is really kind of a historical accident. So we can just fold
>> this struct into VirtioBus. Sorry for misleading you earlier.
>>
>>> +struct VirtioBus {
>>> +    BusState qbus;
>>> +    int busnr;
>> Why does the bus need to know what number it is?
>>
>>> +    bool bus_in_use;
>>> +    uint16_t pci_device_id;
>>> +    uint16_t pci_class;
>> This shouldn't be here -- VirtioBus should be transport
>> independent, so no PCI related info.
> Agreed - this should be a property of the bridge device.
Yes, So I must add the virtiodevice identifier and put a switch case in 
the transport to set the right PCI IDs?
In that case, the transport won't be device independent.

>>> +    VirtIODevice *vdev;
>> This could use a comment saying that we only ever have one
>> child device on the bus.
>>
>> +    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
>> --
>> 1.7.11.7
>> -- PMM
>>
Cornelia Huck - Nov. 20, 2012, 4:15 p.m.
On Tue, 20 Nov 2012 15:30:35 +0100
KONRAD Frédéric <fred.konrad@greensocs.com> wrote:

> On 20/11/2012 15:12, Cornelia Huck wrote:
> > On Mon, 19 Nov 2012 17:33:01 +0000
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> >> On 16 November 2012 15:35,  <fred.konrad@greensocs.com> wrote:
> >>> From: KONRAD Frederic <fred.konrad@greensocs.com>
> >> You forgot to CC enough people...
> >>
> >>> 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.
> > Can we specify the general purpose of the {init,exit}_cb a bit better?
> > Is it analogous to any of the functions performed by the transport
> > busses today?
> For example, look at the function static int 
> virtio_blk_init_pci(PCIDevice *pci_dev) in virtio-pci.c,
> 
> it creates the VirtIODevice and then call virtio_init_pci(proxy, vdev);
> 
> It is what I tryed to reproduce, and abstract it from the new device.
> 
> eg : for the new virtio-blk I add, in the function static int 
> virtio_blk_qdev_init(DeviceState *qdev) in virtio-blk.c :
> 
> it creates the VirtIODevice, create the virtiobus and call the 
> virtio_bus_init_cb(bus, vdev)
>   which call the virtio_init_pci callback when virtio-pci bridge is used.
> 
> it is the same thing for the exit.
> 
> Is it making sense ?

Yes, I think I understand. It would probably make sense to add a
comment like "transport specific, device type independent
initialization" or so.

> 
> >
> >>>      * 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.
> >> Unless you copied this code from an existing v2-only file, preferred
> >> license for new code is v2-or-any-later-version.
> >>
> >>> + */
> >>> +
> >>> +#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
> >>> +
> >>> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev);
> >> Is this function necessary? I think your implementation
> >> is doing the same as the default.
> >>
> >>> +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);
> >>> +    bus->busnr = next_virtio_bus++;
> >> This looks suspicious to me -- is keeping a count of the global
> >> bus number really the right way to do this?
> > If we want an unique number, we need to track usage of numbers, else we
> > end up with duplicates on wraparound.
> I put that because the number was all identical in "info qtree", is it 
> the right thing to do ?

Not sure whether we need a unique name for each bus as each proxy
device will have only one bus beneath it - but if we need it, it must
stay unique when hot(un)plugging devices, even after walking the int
space once.

> 
> >>> +    bus->info = info;
> >>> +    /* no hotplug for the moment ? */
> >>> +    bus->qbus.allow_hotplug = 0;
> >> Correct -- we don't need to hotplug the virtio backend.
> >>
> >>> +    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 looks wrong -- virtio_bind_device() is part of the
> >> old-style transport API.
> >>
> 
> 
> >>> +}
> >>> +
> >>> +/* This must be called to when the VirtIODevice init */
> >>> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *vdev)
> >>> +{
> >>> +    if (bus->bus_in_use == true) {
> >>> +        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;
> >> This shouldn't be happening here, it should happen as
> >> part of plugging the device into the bus.
> >>
> >>> +    bus->info->init_cb(bus->qbus.parent);
> >>> +
> >>> +    bus->bus_in_use = true;
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +/* This must be called when the VirtIODevice exit */
> >>> +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;
> >>> +}
> >> These shouldn't be necessary -- the VirtioDevice should
> >> have a pointer to the VirtioBus and can just invoke the
> >> init/exit callbacks directly.
> >>
> >>> +
> >>> +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_
> >> Leading underscores are reserved by the C standard. VIRTIO_BUS_H will
> >> do fine.
> >>
> >>> +
> >>> +#include "qdev.h"
> >>> +#include "sysemu.h"
> >>> +#include "virtio.h"
> >>> +
> >>> +#define TYPE_VIRTIO_BUS "VIRTIO"
> >> Type names are generally "virtio-bus" by convention.
> >>
> >>> +#define BUS_NAME "virtio"
> >> I don't think you want this.
> >>
> >>
> >>> +typedef struct VirtioBus VirtioBus;
> >>> +typedef struct VirtioBusInfo VirtioBusInfo;
> >>> +
> >>> +struct VirtioBusInfo {
> >>> +    void (*init_cb)(DeviceState *dev);
> >>> +    void (*exit_cb)(DeviceState *dev);
> >>> +    VirtIOBindings virtio_bindings;
> >> This doesn't look right -- VirtIOBindings is the
> >> old-style way for the transport to specify a bunch
> >> of function pointers for its specific implementation.
> >> Those function pointers should probably just be in
> >> the VirtioBus struct.
> >>
> >>> +};
> >> I was just talking to Anthony on IRC and he said SCSIBusInfo
> >> is really kind of a historical accident. So we can just fold
> >> this struct into VirtioBus. Sorry for misleading you earlier.
> >>
> >>> +struct VirtioBus {
> >>> +    BusState qbus;
> >>> +    int busnr;
> >> Why does the bus need to know what number it is?
> >>
> >>> +    bool bus_in_use;
> >>> +    uint16_t pci_device_id;
> >>> +    uint16_t pci_class;
> >> This shouldn't be here -- VirtioBus should be transport
> >> independent, so no PCI related info.
> > Agreed - this should be a property of the bridge device.
> Yes, So I must add the virtiodevice identifier and put a switch case in 
> the transport to set the right PCI IDs?
> In that case, the transport won't be device independent.

So these are values depending upon the virtio device type?
Can you calculate these from the virtio device?

> 
> >>> +    VirtIODevice *vdev;
> >> This could use a comment saying that we only ever have one
> >> child device on the bus.
> >>
> >> +    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
> >> --
> >> 1.7.11.7
> >> -- PMM
> >>
>
fred.konrad@greensocs.com - Nov. 20, 2012, 4:45 p.m.
On 20/11/2012 17:15, Cornelia Huck wrote:
> On Tue, 20 Nov 2012 15:30:35 +0100
> KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
>
>> On 20/11/2012 15:12, Cornelia Huck wrote:
>>> On Mon, 19 Nov 2012 17:33:01 +0000
>>> Peter Maydell <peter.maydell@linaro.org> wrote:
>>>
>>>> On 16 November 2012 15:35,  <fred.konrad@greensocs.com> wrote:
>>>>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>>> You forgot to CC enough people...
>>>>
>>>>> 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.
>>> Can we specify the general purpose of the {init,exit}_cb a bit better?
>>> Is it analogous to any of the functions performed by the transport
>>> busses today?
>> For example, look at the function static int
>> virtio_blk_init_pci(PCIDevice *pci_dev) in virtio-pci.c,
>>
>> it creates the VirtIODevice and then call virtio_init_pci(proxy, vdev);
>>
>> It is what I tryed to reproduce, and abstract it from the new device.
>>
>> eg : for the new virtio-blk I add, in the function static int
>> virtio_blk_qdev_init(DeviceState *qdev) in virtio-blk.c :
>>
>> it creates the VirtIODevice, create the virtiobus and call the
>> virtio_bus_init_cb(bus, vdev)
>>    which call the virtio_init_pci callback when virtio-pci bridge is used.
>>
>> it is the same thing for the exit.
>>
>> Is it making sense ?
> Yes, I think I understand. It would probably make sense to add a
> comment like "transport specific, device type independent
> initialization" or so.
Ok, good idea I'll add that.

>>>>>       * 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.
>>>> Unless you copied this code from an existing v2-only file, preferred
>>>> license for new code is v2-or-any-later-version.
>>>>
>>>>> + */
>>>>> +
>>>>> +#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
>>>>> +
>>>>> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev);
>>>> Is this function necessary? I think your implementation
>>>> is doing the same as the default.
>>>>
>>>>> +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);
>>>>> +    bus->busnr = next_virtio_bus++;
>>>> This looks suspicious to me -- is keeping a count of the global
>>>> bus number really the right way to do this?
>>> If we want an unique number, we need to track usage of numbers, else we
>>> end up with duplicates on wraparound.
>> I put that because the number was all identical in "info qtree", is it
>> the right thing to do ?
> Not sure whether we need a unique name for each bus as each proxy
> device will have only one bus beneath it - but if we need it, it must
> stay unique when hot(un)plugging devices, even after walking the int
> space once.
Ok, it isn't necessary as we don't allow hotplugging.

>>>>> +    bus->info = info;
>>>>> +    /* no hotplug for the moment ? */
>>>>> +    bus->qbus.allow_hotplug = 0;
>>>> Correct -- we don't need to hotplug the virtio backend.
>>>>
>>>>> +    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 looks wrong -- virtio_bind_device() is part of the
>>>> old-style transport API.
>>>>
>>
>>>>> +}
>>>>> +
>>>>> +/* This must be called to when the VirtIODevice init */
>>>>> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *vdev)
>>>>> +{
>>>>> +    if (bus->bus_in_use == true) {
>>>>> +        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;
>>>> This shouldn't be happening here, it should happen as
>>>> part of plugging the device into the bus.
>>>>
>>>>> +    bus->info->init_cb(bus->qbus.parent);
>>>>> +
>>>>> +    bus->bus_in_use = true;
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +/* This must be called when the VirtIODevice exit */
>>>>> +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;
>>>>> +}
>>>> These shouldn't be necessary -- the VirtioDevice should
>>>> have a pointer to the VirtioBus and can just invoke the
>>>> init/exit callbacks directly.
>>>>
>>>>> +
>>>>> +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_
>>>> Leading underscores are reserved by the C standard. VIRTIO_BUS_H will
>>>> do fine.
>>>>
>>>>> +
>>>>> +#include "qdev.h"
>>>>> +#include "sysemu.h"
>>>>> +#include "virtio.h"
>>>>> +
>>>>> +#define TYPE_VIRTIO_BUS "VIRTIO"
>>>> Type names are generally "virtio-bus" by convention.
>>>>
>>>>> +#define BUS_NAME "virtio"
>>>> I don't think you want this.
>>>>
>>>>
>>>>> +typedef struct VirtioBus VirtioBus;
>>>>> +typedef struct VirtioBusInfo VirtioBusInfo;
>>>>> +
>>>>> +struct VirtioBusInfo {
>>>>> +    void (*init_cb)(DeviceState *dev);
>>>>> +    void (*exit_cb)(DeviceState *dev);
>>>>> +    VirtIOBindings virtio_bindings;
>>>> This doesn't look right -- VirtIOBindings is the
>>>> old-style way for the transport to specify a bunch
>>>> of function pointers for its specific implementation.
>>>> Those function pointers should probably just be in
>>>> the VirtioBus struct.
>>>>
>>>>> +};
>>>> I was just talking to Anthony on IRC and he said SCSIBusInfo
>>>> is really kind of a historical accident. So we can just fold
>>>> this struct into VirtioBus. Sorry for misleading you earlier.
>>>>
>>>>> +struct VirtioBus {
>>>>> +    BusState qbus;
>>>>> +    int busnr;
>>>> Why does the bus need to know what number it is?
>>>>
>>>>> +    bool bus_in_use;
>>>>> +    uint16_t pci_device_id;
>>>>> +    uint16_t pci_class;
>>>> This shouldn't be here -- VirtioBus should be transport
>>>> independent, so no PCI related info.
>>> Agreed - this should be a property of the bridge device.
>> Yes, So I must add the virtiodevice identifier and put a switch case in
>> the transport to set the right PCI IDs?
>> In that case, the transport won't be device independent.
> So these are values depending upon the virtio device type?
> Can you calculate these from the virtio device?
Yes, it depends on the virtio device id, they are set for the virtio-x-pci
devices in the init class.

eg for virtio-block-pci in virtio-pci.c :
static void virtio_blk_class_init(ObjectClass *klass, void *data)
{
     k->device_id = PCI_DEVICE_ID_VIRTIO_BLOCK;
     ...
     k->class_id = PCI_CLASS_STORAGE_SCSI;
}

I think that the better solution is to put these value in a big switch 
case :

eg :

Adding that in virtio-bus.c :

/* Return the virtio device id of the plugged device. */
uint16_t get_virtio_device_id(VirtioBus *bus)
{
     return bus->vdev->device_id;
}

Using that in virtio-pci transport initialisation.

     switch (get_virtio_device_id(&proxy->bus))
     {
         case VIRTIO_ID_BLOCK:
             pci_config_set_device_id(proxy->pci_dev.config,
                                      PCI_DEVICE_ID_VIRTIO_BLOCK);
             pci_config_set_class(proxy->pci_dev.config, 
PCI_CLASS_STORAGE_SCSI);
         break;
         default:
             error_report("unknown device id\n");
         break;
     }

but the transport stay ( a little ) device dependent.

>>>>> +    VirtIODevice *vdev;
>>>> This could use a comment saying that we only ever have one
>>>> child device on the bus.
>>>>
>>>> +    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
>>>> --
>>>> 1.7.11.7
>>>> -- PMM
>>>>
Cornelia Huck - Nov. 20, 2012, 5:27 p.m.
On Tue, 20 Nov 2012 17:45:15 +0100
KONRAD Frédéric <fred.konrad@greensocs.com> wrote:


> eg for virtio-block-pci in virtio-pci.c :
> static void virtio_blk_class_init(ObjectClass *klass, void *data)
> {
>      k->device_id = PCI_DEVICE_ID_VIRTIO_BLOCK;
>      ...
>      k->class_id = PCI_CLASS_STORAGE_SCSI;
> }
> 
> I think that the better solution is to put these value in a big switch 
> case :
> 
> eg :
> 
> Adding that in virtio-bus.c :
> 
> /* Return the virtio device id of the plugged device. */
> uint16_t get_virtio_device_id(VirtioBus *bus)
> {
>      return bus->vdev->device_id;
> }

Yes, we'll need this for virtio-ccw as well (the id is used as the CU
model).

> 
> Using that in virtio-pci transport initialisation.
> 
>      switch (get_virtio_device_id(&proxy->bus))
>      {
>          case VIRTIO_ID_BLOCK:
>              pci_config_set_device_id(proxy->pci_dev.config,
>                                       PCI_DEVICE_ID_VIRTIO_BLOCK);
>              pci_config_set_class(proxy->pci_dev.config, 
> PCI_CLASS_STORAGE_SCSI);
>          break;
>          default:
>              error_report("unknown device id\n");
>          break;
>      }
> 
> but the transport stay ( a little ) device dependent.

Well, given that the device id _is_ device dependent, doing the device
id -> transport specific id transition in the transport code seems
sensible.
fred.konrad@greensocs.com - Nov. 21, 2012, 9:31 a.m.
On 19/11/2012 18:33, Peter Maydell wrote:
> On 16 November 2012 15:35,  <fred.konrad@greensocs.com> wrote:
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
> You forgot to CC enough people...
>
>> 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.
> Unless you copied this code from an existing v2-only file, preferred
> license for new code is v2-or-any-later-version.
>
>> + */
>> +
>> +#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
>> +
>> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev);
> Is this function necessary? I think your implementation
> is doing the same as the default.
>
>> +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);
>> +    bus->busnr = next_virtio_bus++;
> This looks suspicious to me -- is keeping a count of the global
> bus number really the right way to do this?
>
>> +    bus->info = info;
>> +    /* no hotplug for the moment ? */
>> +    bus->qbus.allow_hotplug = 0;
> Correct -- we don't need to hotplug the virtio backend.
>
>> +    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 looks wrong -- virtio_bind_device() is part of the
> old-style transport API.
it is part of the virtiodevice API, I don't think It is a good idea to 
modify it ?

>> +}
>> +
>> +/* This must be called to when the VirtIODevice init */
>> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *vdev)
>> +{
>> +    if (bus->bus_in_use == true) {
>> +        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;
> This shouldn't be happening here, it should happen as
> part of plugging the device into the bus.
What do you mean by plugging the device into the bus ?

I think that the device is already plugged when I call this function, I 
don't find an
other idea to set maximum device per bus to 1.

>
>> +    bus->info->init_cb(bus->qbus.parent);
>> +
>> +    bus->bus_in_use = true;
>> +    return 0;
>> +}
>> +
>> +/* This must be called when the VirtIODevice exit */
>> +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;
>> +}
> These shouldn't be necessary -- the VirtioDevice should
> have a pointer to the VirtioBus and can just invoke the
> init/exit callbacks directly.
>
>> +
>> +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_
> Leading underscores are reserved by the C standard. VIRTIO_BUS_H will
> do fine.
>
>> +
>> +#include "qdev.h"
>> +#include "sysemu.h"
>> +#include "virtio.h"
>> +
>> +#define TYPE_VIRTIO_BUS "VIRTIO"
> Type names are generally "virtio-bus" by convention.
>
>> +#define BUS_NAME "virtio"
> I don't think you want this.
>
>
>> +typedef struct VirtioBus VirtioBus;
>> +typedef struct VirtioBusInfo VirtioBusInfo;
>> +
>> +struct VirtioBusInfo {
>> +    void (*init_cb)(DeviceState *dev);
>> +    void (*exit_cb)(DeviceState *dev);
>> +    VirtIOBindings virtio_bindings;
> This doesn't look right -- VirtIOBindings is the
> old-style way for the transport to specify a bunch
> of function pointers for its specific implementation.
> Those function pointers should probably just be in
> the VirtioBus struct.
>
>> +};
> I was just talking to Anthony on IRC and he said SCSIBusInfo
> is really kind of a historical accident. So we can just fold
> this struct into VirtioBus. Sorry for misleading you earlier.
>
>> +struct VirtioBus {
>> +    BusState qbus;
>> +    int busnr;
> Why does the bus need to know what number it is?
>
>> +    bool bus_in_use;
>> +    uint16_t pci_device_id;
>> +    uint16_t pci_class;
> This shouldn't be here -- VirtioBus should be transport
> independent, so no PCI related info.
>
>> +    VirtIODevice *vdev;
> This could use a comment saying that we only ever have one
> child device on the bus.
>
>> +    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
>> --
>> 1.7.11.7
> -- PMM
Andreas Färber - Nov. 21, 2012, 1:04 p.m.
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.

> + */
> +
> +#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.

> +
> +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...

> +    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"

> +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.

> +        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?

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.

> +    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
fred.konrad@greensocs.com - Nov. 21, 2012, 2:05 p.m.
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
Andreas Färber - Nov. 21, 2012, 2:13 p.m.
Am 21.11.2012 15:05, schrieb KONRAD Frédéric:
> On 21/11/2012 14:04, Andreas Färber wrote:
>> Am 16.11.2012 16:35, schrieb fred.konrad@greensocs.com:
>>> +#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 ?

I don't have a link at hand, maybe Evgeny does. It was along the lines of:

#define DEBUG_VIRTIO_BUS 0

#define DPRINTF(fmt, ...) if (DEBUG_VIRTIO_BUS) { \
        printf("virtio_bus: " fmt , ## __VA_ARGS__); \
    }

The officially preferred alternative is to use tracepoints. ;)

Cheers,
Andreas
fred.konrad@greensocs.com - Nov. 21, 2012, 2:18 p.m.
On 21/11/2012 15:13, Andreas Färber wrote:
> Am 21.11.2012 15:05, schrieb KONRAD Frédéric:
>> On 21/11/2012 14:04, Andreas Färber wrote:
>>> Am 16.11.2012 16:35, schrieb fred.konrad@greensocs.com:
>>>> +#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 ?
> I don't have a link at hand, maybe Evgeny does. It was along the lines of:
>
> #define DEBUG_VIRTIO_BUS 0
>
> #define DPRINTF(fmt, ...) if (DEBUG_VIRTIO_BUS) { \
>          printf("virtio_bus: " fmt , ## __VA_ARGS__); \
>      }
>
> The officially preferred alternative is to use tracepoints. ;)
>
> Cheers,
> Andreas
>
ok, thanks.

Fred

Patch

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.
+ */
+
+#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
+
+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);
+    bus->busnr = next_virtio_bus++;
+    bus->info = info;
+    /* no hotplug for the moment ? */
+    bus->qbus.allow_hotplug = 0;
+    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 */
+int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *vdev)
+{
+    if (bus->bus_in_use == true) {
+        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 */
+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"
+#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;
+    int busnr;
+    bool bus_in_use;
+    uint16_t pci_device_id;
+    uint16_t pci_class;
+    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