[Qemu-devel] [RFC PATCH 0/3] Virtio refactoring.

[Qemu-devel] [RFC PATCH 0/3] Virtio refactoring.

[fred.konrad]

Hi,

I submit this RFC to be sure I'm doing the right thing about the VirtioBus.

I push the patchset here : 

git://git.greensocs.com/qemu_virtio.git virtio_refact

What I proposed to do :

    * Introduce a new VirtioBus ( same way as scsi-bus.c ), with VirtIODevice
      interface :
         -> callback to completely abstract the VirtioDevice from VirtioPCI.
         -> for the queue, load/save the queue/config, features, ..., other ?

    * Add a VirtioBus to the VirtioPCIProxy. ( virtio-pci.c ) :
         -> moving all to the newer callback.

    * For each of the virtio-device : ( virtio-x.c )
         -> making a separate class for virtio-x which is a VirtioDevice.
         -> making a virtio-x-pci which has a virtio-x.

    * Create virtio-mmio ( virtio-mmio.c ).

What it is done in this RFC :

    * introduce VirtioBus, which can be added to Virtio transports like
      virtio-pci or virtio-mmio. I think the interface must be improved.

    * add a VirtioBus to the existing VirtioPCIProxy ( virtio-pci ), and add a
      "virtio-pci" device which can be loaded without a VirtIODevice, and create
      a VirtioBus.

    * add a "virtio-blk" device which must be connected on a virtio-bus, to try
      the virtio-bus and virtio-pci.

The info qtree return this structure :

bus: main-system-bus
  type System
  dev: i440FX-pcihost, id ""
    bus: pci.0
      type PCI
      dev: virtio-pci, id ""
        bus: virtio.0
          type VIRTIO
          dev: virtio-blk, id ""

What I must improve :

    * the Virtio-PCI still need the VirtIODevice.
    * others devices.
    * spliting virtio-x-pci to virtio-x + virtio-pci.

Did I miss something ?

Thanks,

Fred.

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

[fred.konrad]

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

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

[Peter Maydell]

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

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

[Cornelia Huck]

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
> 

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

[KONRAD Frédéric]

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

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

[Cornelia Huck]

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

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

[KONRAD Frédéric]

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

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

[Cornelia Huck]

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.

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

[KONRAD Frédéric]

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

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

[Andreas Färber]

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[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:
>> 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

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

[Andreas Färber]

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[KONRAD Frédéric]

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

[Qemu-devel] [RFC PATCH 2/3] virtio-pci : Add a virtio-bus interface

[fred.konrad]

From: KONRAD Frederic <fred.konrad@greensocs.com>

This patch add a VirtioBus in the VirtIOPCIProxy structure. It creates a new
device : "virtio-pci" which init the VirtioBus. Two callback are written :

    * void virtio_pci_init_cb(DeviceState *dev) to initialize the PCI interface
      after the VirtIODevice init, it is approximately the same as
      virtio_init_pci.

    * void virtio_pci_exit_cb(DeviceState *dev) to stop the ioevent before the
      VirtIODevice exit.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 hw/virtio-pci.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-pci.h |   6 +++
 2 files changed, 148 insertions(+)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 9603150..09bbb2a 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -22,6 +22,7 @@
 #include "virtio-net.h"
 #include "virtio-serial.h"
 #include "virtio-scsi.h"
+#include "virtio-bus.h"
 #include "pci.h"
 #include "qemu-error.h"
 #include "msi.h"
@@ -1039,13 +1040,154 @@ static TypeInfo virtio_scsi_info = {
     .class_init    = virtio_scsi_class_init,
 };
 
+/* The new virtio-pci device */
+
+void virtio_pci_init_cb(DeviceState *dev)
+{
+    PCIDevice *d = DO_UPCAST(PCIDevice, qdev, dev);
+    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, d);
+    uint8_t *config;
+    uint32_t size;
+    /* Put the PCI IDs */
+    pci_config_set_device_id(proxy->pci_dev.config,
+                             proxy->bus.pci_device_id);
+    pci_config_set_class(proxy->pci_dev.config, proxy->bus.pci_class);
+
+    /* virtio_init_pci code without bindings */
+
+    /* This should disappear */
+    proxy->vdev = proxy->bus.vdev;
+
+    config = proxy->pci_dev.config;
+
+    if (proxy->class_code) {
+        pci_config_set_class(config, proxy->class_code);
+    }
+    pci_set_word(config + PCI_SUBSYSTEM_VENDOR_ID,
+                 pci_get_word(config + PCI_VENDOR_ID));
+    pci_set_word(config + PCI_SUBSYSTEM_ID, proxy->bus.vdev->device_id);
+    config[PCI_INTERRUPT_PIN] = 1;
+
+    if (proxy->bus.vdev->nvectors &&
+        msix_init_exclusive_bar(&proxy->pci_dev, proxy->bus.vdev->nvectors,
+                                1)) {
+        proxy->bus.vdev->nvectors = 0;
+    }
+
+    proxy->pci_dev.config_write = virtio_write_config;
+
+    size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev)
+         + proxy->bus.vdev->config_len;
+    if (size & (size-1)) {
+        size = 1 << qemu_fls(size);
+    }
+
+    memory_region_init_io(&proxy->bar, &virtio_pci_config_ops, proxy,
+                          "virtio-pci", size);
+    pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
+                     &proxy->bar);
+
+    if (!kvm_has_many_ioeventfds()) {
+        proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
+    }
+
+    /* Bind the VirtIODevice to the VirtioBus. */
+    virtio_bus_bind_device(&(proxy->bus));
+
+    proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
+    proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
+    /* Should be modified */
+    proxy->host_features = proxy->bus.vdev->get_features(proxy->bus.vdev,
+                                                         proxy->host_features);
+}
+
+void virtio_pci_exit_cb(DeviceState *dev)
+{
+    PCIDevice *d = DO_UPCAST(PCIDevice, qdev, dev);
+    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, d);
+    /* Put the PCI IDs */
+    pci_config_set_device_id(proxy->pci_dev.config, 0x0000);
+    pci_config_set_class(proxy->pci_dev.config, PCI_CLASS_OTHERS);
+
+    virtio_pci_stop_ioeventfd(proxy);
+}
+
+static const struct VirtioBusInfo virtio_bus_info = {
+    .init_cb = virtio_pci_init_cb,
+    .exit_cb = virtio_pci_exit_cb,
+
+    .virtio_bindings = {
+        .notify = virtio_pci_notify,
+        .save_config = virtio_pci_save_config,
+        .load_config = virtio_pci_load_config,
+        .save_queue = virtio_pci_save_queue,
+        .load_queue = virtio_pci_load_queue,
+        .get_features = virtio_pci_get_features,
+        .query_guest_notifiers = virtio_pci_query_guest_notifiers,
+        .set_host_notifier = virtio_pci_set_host_notifier,
+        .set_guest_notifiers = virtio_pci_set_guest_notifiers,
+        .vmstate_change = virtio_pci_vmstate_change,
+    }
+};
+
+static int virtiopci_qdev_init(PCIDevice *dev)
+{
+    VirtIOPCIProxy *s = DO_UPCAST(VirtIOPCIProxy, pci_dev, dev);
+
+    /* create a virtio-bus and attach virtio-pci to it */
+    virtio_bus_new(&s->bus, &dev->qdev, &virtio_bus_info);
+
+    return 0;
+}
+
+static Property virtiopci_properties[] = {
+    /* TODO : Add the correct properties */
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void virtiopci_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    PCIDeviceClass *pc = PCI_DEVICE_CLASS(oc);
+
+    pc->init = virtiopci_qdev_init;
+    pc->exit = virtio_exit_pci;
+    pc->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
+    pc->revision = VIRTIO_PCI_ABI_VERSION;
+    pc->class_id = PCI_CLASS_OTHERS;
+    /* TODO : Add the correct device information below */
+    /* pc->exit =
+     * pc->device_id =
+     * pc->subsystem_vendor_id =
+     * pc->subsystem_id =
+     * dc->reset =
+     * dc->vmsd =
+     */
+    dc->props = virtiopci_properties;
+    dc->desc = "virtio-pci transport.";
+}
+
+static const TypeInfo virtio_pci_info = {
+    .name  = "virtio-pci",
+    .parent = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(VirtIOPCIProxy),
+    .class_init = virtiopci_class_init,
+};
+
+
+/************************************/
+
+
 static void virtio_pci_register_types(void)
 {
+    /* This should disappear */
     type_register_static(&virtio_blk_info);
     type_register_static(&virtio_net_info);
     type_register_static(&virtio_serial_info);
     type_register_static(&virtio_balloon_info);
     type_register_static(&virtio_scsi_info);
+    /* new virtio-pci device */
+    type_register_static(&virtio_pci_info);
 }
 
 type_init(virtio_pci_register_types)
diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h
index ac9d522..d3ce579 100644
--- a/hw/virtio-pci.h
+++ b/hw/virtio-pci.h
@@ -19,6 +19,7 @@
 #include "virtio-net.h"
 #include "virtio-serial.h"
 #include "virtio-scsi.h"
+#include "virtio-bus.h"
 
 /* Performance improves when virtqueue kick processing is decoupled from the
  * vcpu thread using ioeventfd for some devices. */
@@ -32,6 +33,7 @@ typedef struct {
 
 typedef struct {
     PCIDevice pci_dev;
+    /* This should be removed */
     VirtIODevice *vdev;
     MemoryRegion bar;
     uint32_t flags;
@@ -49,10 +51,14 @@ typedef struct {
     bool ioeventfd_disabled;
     bool ioeventfd_started;
     VirtIOIRQFD *vector_irqfd;
+    /* VirtIOBus to connect the VirtIODevice */
+    VirtioBus bus;
 } VirtIOPCIProxy;
 
 void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev);
 void virtio_pci_reset(DeviceState *d);
+void virtio_pci_init_cb(DeviceState *dev);
+void virtio_pci_exit_cb(DeviceState *dev);
 
 /* Virtio ABI version, if we increment this, we break the guest driver. */
 #define VIRTIO_PCI_ABI_VERSION          0
-- 
1.7.11.7

[Qemu-devel] [RFC PATCH 3/3] virtio-blk : add the virtio-blk device.

[fred.konrad]

From: KONRAD Frederic <fred.konrad@greensocs.com>

This patch just add the virtio-blk device which can connect on a Virtio-Bus. The
initialization fail if no free VirtioBus are present.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 hw/virtio-blk.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-blk.h |  7 +++++
 2 files changed, 91 insertions(+)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index e25cc96..0fa2ab9 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -17,6 +17,8 @@
 #include "hw/block-common.h"
 #include "blockdev.h"
 #include "virtio-blk.h"
+#include "virtio-bus.h"
+#include "hw/pci.h"         /* for PCI IDs */
 #include "scsi-defs.h"
 #ifdef __linux__
 # include <scsi/sg.h>
@@ -656,3 +658,85 @@ void virtio_blk_exit(VirtIODevice *vdev)
     blockdev_mark_auto_del(s->bs);
     virtio_cleanup(vdev);
 }
+
+static int virtio_blk_qdev_init(DeviceState *qdev)
+{
+    VirtIODevice *vdev;
+    VirtIOBLKState *s = DO_UPCAST(VirtIOBLKState, qdev, qdev);
+    VirtioBus *bus = DO_UPCAST(VirtioBus, qbus, s->qdev.parent_bus);
+
+    vdev = virtio_blk_init(qdev, &s->blk);
+    if (!vdev) {
+        return -1;
+    }
+
+    /* Set the PCI IDs :
+     * What if we add both IDs in VirtIODevice ?
+     */
+    bus->pci_device_id = PCI_DEVICE_ID_VIRTIO_BLOCK;
+    bus->pci_class = PCI_CLASS_STORAGE_SCSI;
+
+    /* Call the init callback of the transport device eg: virtio-pci */
+    if (virtio_bus_init_cb(bus, vdev) != 0) {
+        /* this can happen when the bus is not free */
+        return -1;
+    }
+
+    return 0;
+}
+
+static int virtio_blk_qdev_exit(DeviceState *qdev)
+{
+    VirtioBus *bus = DO_UPCAST(VirtioBus, qbus, qdev->parent_bus);
+    virtio_bus_exit_cb(bus);
+    virtio_blk_exit(bus->vdev);
+    return 0;
+}
+
+static Property virtio_blk_properties[] = {
+    DEFINE_BLOCK_PROPERTIES(VirtIOBLKState, blk.conf),
+    DEFINE_BLOCK_CHS_PROPERTIES(VirtIOBLKState, blk.conf),
+    DEFINE_PROP_STRING("serial", VirtIOBLKState, blk.serial),
+#ifdef __linux__
+    DEFINE_PROP_BIT("scsi", VirtIOBLKState, blk.scsi, 0, true),
+#endif
+    DEFINE_PROP_BIT("config-wce", VirtIOBLKState, blk.config_wce, 0, true),
+    /*
+     * Theses one are PCI related.
+     * DEFINE_PROP_BIT("ioeventfd", VirtIOBLKState, flags,
+     *                 VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
+     * DEFINE_PROP_UINT32("vectors", VirtIOBLKState, nvectors, 2),
+     */
+    DEFINE_VIRTIO_BLK_FEATURES(VirtIOBLKState, host_features),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void virtio_blk_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *k = DEVICE_CLASS(klass);
+    k->bus_type = TYPE_VIRTIO_BUS;
+    k->init = virtio_blk_qdev_init;
+    k->exit = virtio_blk_qdev_exit;
+    /*
+     * k->unplug = ?
+     */
+    k->props = virtio_blk_properties;
+}
+
+static TypeInfo virtio_blk_info = {
+    .name = "virtio-blk",
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(VirtIOBLKState),
+    /*
+     * .abstract = true,
+     * .class_size = sizeof(?),
+     */
+    .class_init = virtio_blk_class_init,
+};
+
+static void virtio_blk_register_types(void)
+{
+    type_register_static(&virtio_blk_info);
+}
+
+type_init(virtio_blk_register_types)
diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
index f0740d0..9e4bd27 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -16,6 +16,7 @@
 
 #include "virtio.h"
 #include "hw/block-common.h"
+#include "virtio-bus.h"
 
 /* from Linux's linux/virtio_blk.h */
 
@@ -107,6 +108,12 @@ struct VirtIOBlkConf
     uint32_t config_wce;
 };
 
+typedef struct {
+    DeviceState qdev;
+    uint32_t host_features;
+    VirtIOBlkConf blk;
+} VirtIOBLKState;
+
 #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
         DEFINE_VIRTIO_COMMON_FEATURES(_state, _field), \
         DEFINE_PROP_BIT("config-wce", _state, _field, VIRTIO_BLK_F_CONFIG_WCE, true)
-- 
1.7.11.7