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

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

[fred.konrad]

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

Hi,

I made the changes you suggest in the last RFC. 

There are still two issues with the command line :

    * When I use ./qemu* -device virtio-blk -device virtio-pci
      It is said that no virtio-bus are present.
    * The virtio-blk is plugged in the last created virtio-bus if no "bus="
      option is present. It's an issue as we can only plug one virtio-device.

The first problem is a more general issue as it is the case for the SCSI bus and
can be fixed later.

Changes v1 -> v2:

    * All the little fix you suggest ( License, Debug printf, naming convention,
      ...)
    * Added get_virtio_device_id(), and remove the pci_id* from the VirtioBus
      structure.
    * Added virtio_bus_reset().
    * Added cast macros VIRTIO_BUS.
    * Added virtio_bus_plug_device.
    * Replaced the old-style "bus->qbus" by BUS() macro.

git available here :
    git://git.greensocs.com/qemu_virtio.git virtio_refactoring_2
or with http :
    http://git.greensocs.com/?p=qemu_virtio.git;a=shortlog; \
    h=refs/heads/virtio_refactoring_2

Fred

KONRAD Frederic (3):
  virtio-bus : Introduce VirtioBus.
  virtio-pci : add a virtio-bus interface
  virtio-blk : add the virtio-blk device.

 hw/Makefile.objs |   1 +
 hw/virtio-blk.c  |  82 ++++++++++++++++++++++++++++++
 hw/virtio-blk.h  |  10 ++++
 hw/virtio-bus.c  | 148 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-bus.h  |  58 +++++++++++++++++++++
 hw/virtio-pci.c  | 152 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-pci.h  |   6 +++
 7 files changed, 457 insertions(+)
 create mode 100644 hw/virtio-bus.c
 create mode 100644 hw/virtio-bus.h

-- 
1.7.11.7

[Qemu-devel] [RFC PATCH v2 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  | 148 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-bus.h  |  58 ++++++++++++++++++++++
 3 files changed, 207 insertions(+)
 create mode 100644 hw/virtio-bus.c
 create mode 100644 hw/virtio-bus.h

diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index ea46f81..bd14d1b 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -2,6 +2,7 @@ common-obj-y = usb/ ide/
 common-obj-y += loader.o
 common-obj-$(CONFIG_VIRTIO) += virtio-console.o
 common-obj-$(CONFIG_VIRTIO) += virtio-rng.o
+common-obj-$(CONFIG_VIRTIO) += virtio-bus.o
 common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
 common-obj-y += fw_cfg.o
 common-obj-$(CONFIG_PCI) += pci.o pci_bridge.o pci_bridge_dev.o
diff --git a/hw/virtio-bus.c b/hw/virtio-bus.c
new file mode 100644
index 0000000..991b6f5
--- /dev/null
+++ b/hw/virtio-bus.c
@@ -0,0 +1,148 @@
+/*
+ * VirtioBus
+ *
+ *  Copyright (C) 2012 : GreenSocs Ltd
+ *      http://www.greensocs.com/ , email: info@greensocs.com
+ *
+ *  Developed by :
+ *  Frederic Konrad   <fred.konrad@greensocs.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "hw.h"
+#include "qemu-error.h"
+#include "qdev.h"
+#include "virtio-bus.h"
+#include "virtio.h"
+
+#define DEBUG_VIRTIO_BUS 1
+
+#define DPRINTF(fmt, ...) if (DEBUG_VIRTIO_BUS) {                        \
+                            printf("virtio_bus: " fmt , ## __VA_ARGS__); \
+                          }
+
+static void virtio_bus_init_cb(VirtioBus *bus);
+static int virtio_bus_reset(BusState *qbus);
+
+static void virtio_bus_class_init(ObjectClass *klass, void *data)
+{
+    BusClass *k = BUS_CLASS(klass);
+    k->reset = virtio_bus_reset;
+}
+
+static TypeInfo virtio_bus_info = {
+    .name = TYPE_VIRTIO_BUS,
+    .parent = TYPE_BUS,
+    .instance_size = sizeof(VirtioBus),
+    .class_init = virtio_bus_class_init,
+};
+
+/* Reset the bus */
+static int virtio_bus_reset(BusState *qbus)
+{
+    VirtioBus *bus = VIRTIO_BUS(qbus);
+    if (bus->bus_in_use) {
+        virtio_reset(bus->vdev);
+    }
+    return 1;
+}
+
+/* Plug the VirtIODevice */
+int virtio_bus_plug_device(VirtioBus *bus, VirtIODevice *vdev)
+{
+    BusState *qbus = BUS(bus);
+    /*
+     * This is a problem, when bus= option is not set, the last created
+     * virtio-bus is used. So it give the following error.
+     */
+    DPRINTF("plug device into %s.\n", qbus->name);
+    if (bus->bus_in_use) {
+        error_report("%s in use.\n", qbus->name);
+        return -1;
+    }
+    bus->bus_in_use = true;
+
+    /* keep the VirtIODevice in the VirtioBus. */
+    bus->vdev = vdev;
+
+    /* call the "transport" callback. */
+    virtio_bus_init_cb(bus);
+    return 0;
+}
+
+/* Create a virtio bus.  */
+VirtioBus *virtio_bus_new(DeviceState *host, const VirtioBusInfo *info)
+{
+    /*
+     * This is needed, as we want to have different names for each virtio-bus.
+     * If we don't do that, we can't add more than one VirtIODevice.
+     */
+    static int next_virtio_bus;
+    char *bus_name = g_strdup_printf("virtio-bus.%d", next_virtio_bus++);
+
+    BusState *qbus = qbus_create(TYPE_VIRTIO_BUS, host, bus_name);
+    VirtioBus *bus = VIRTIO_BUS(qbus);
+    bus->info = info;
+    qbus->allow_hotplug = 0;
+    bus->bus_in_use = false;
+    DPRINTF("%s bus created\n", bus_name);
+    return bus;
+}
+
+/* Bind the VirtIODevice to the VirtioBus. */
+void virtio_bus_bind_device(VirtioBus *bus)
+{
+    BusState *qbus = BUS(bus);
+    assert(bus != NULL);
+    assert(bus->vdev != NULL);
+    virtio_bind_device(bus->vdev, &(bus->info->virtio_bindings), qbus->parent);
+}
+
+/*
+ * Transport independent init.
+ * This must be called after VirtIODevice initialization.
+ */
+static void virtio_bus_init_cb(VirtioBus *bus)
+{
+    BusState *qbus = BUS(bus);
+    assert(bus->info->init_cb != NULL);
+    bus->info->init_cb(qbus->parent);
+}
+
+/*
+ * Transport independent exit.
+ * This must be called by the VirtIODevice before destroying it.
+ */
+void virtio_bus_exit_cb(VirtioBus *bus)
+{
+    BusState *qbus = BUS(bus);
+    assert(bus->info->exit_cb != NULL);
+    bus->info->exit_cb(qbus->parent);
+    bus->bus_in_use = false;
+}
+
+/* Return the virtio device id of the plugged device. */
+uint16_t get_virtio_device_id(VirtioBus *bus)
+{
+    return bus->vdev->device_id;
+}
+
+static void virtio_register_types(void)
+{
+    type_register_static(&virtio_bus_info);
+}
+
+type_init(virtio_register_types)
diff --git a/hw/virtio-bus.h b/hw/virtio-bus.h
new file mode 100644
index 0000000..31aad53
--- /dev/null
+++ b/hw/virtio-bus.h
@@ -0,0 +1,58 @@
+/*
+ * VirtioBus
+ *
+ *  Copyright (C) 2012 : GreenSocs Ltd
+ *      http://www.greensocs.com/ , email: info@greensocs.com
+ *
+ *  Developed by :
+ *  Frederic Konrad   <fred.konrad@greensocs.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#ifndef VIRTIO_BUS_H
+#define VIRTIO_BUS_H
+
+#include "qdev.h"
+#include "sysemu.h"
+#include "virtio.h"
+
+#define TYPE_VIRTIO_BUS "virtio-bus"
+#define VIRTIO_BUS(obj) OBJECT_CHECK(VirtioBus, (obj), TYPE_VIRTIO_BUS)
+
+typedef struct VirtioBus VirtioBus;
+typedef struct VirtioBusInfo VirtioBusInfo;
+
+struct VirtioBusInfo {
+    void (*init_cb)(DeviceState *dev);
+    void (*exit_cb)(DeviceState *dev);
+    VirtIOBindings virtio_bindings;
+};
+
+struct VirtioBus {
+    BusState qbus;
+    bool bus_in_use;
+    /* Only one VirtIODevice can be plugged on the bus. */
+    VirtIODevice *vdev;
+    const VirtioBusInfo *info;
+};
+
+VirtioBus *virtio_bus_new(DeviceState *host, const VirtioBusInfo *info);
+void virtio_bus_bind_device(VirtioBus *bus);
+int virtio_bus_plug_device(VirtioBus *bus, VirtIODevice *vdev);
+void virtio_bus_exit_cb(VirtioBus *bus);
+uint16_t get_virtio_device_id(VirtioBus *bus);
+
+#endif /* VIRTIO_BUS_H */
-- 
1.7.11.7

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

[Cornelia Huck]

On Thu, 22 Nov 2012 15:50:50 +0100
fred.konrad@greensocs.com wrote:


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

This still has the overflow/id-reuse problem, hasn't it?

> +
> +    BusState *qbus = qbus_create(TYPE_VIRTIO_BUS, host, bus_name);
> +    VirtioBus *bus = VIRTIO_BUS(qbus);
> +    bus->info = info;
> +    qbus->allow_hotplug = 0;
> +    bus->bus_in_use = false;
> +    DPRINTF("%s bus created\n", bus_name);
> +    return bus;
> +}

Don't you need a way to destroy the bus again when the proxy device is
hotunplugged?

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

[Konrad Frederic]

On 23/11/2012 13:08, Cornelia Huck wrote:
> On Thu, 22 Nov 2012 15:50:50 +0100
> fred.konrad@greensocs.com wrote:
>
>
>> +/* Create a virtio bus.  */
>> +VirtioBus *virtio_bus_new(DeviceState *host, const VirtioBusInfo *info)
>> +{
>> +    /*
>> +     * This is needed, as we want to have different names for each virtio-bus.
>> +     * If we don't do that, we can't add more than one VirtIODevice.
>> +     */
>> +    static int next_virtio_bus;
>> +    char *bus_name = g_strdup_printf("virtio-bus.%d", next_virtio_bus++);
> This still has the overflow/id-reuse problem, hasn't it?
What do you mean by overflow problem ?

>
>> +
>> +    BusState *qbus = qbus_create(TYPE_VIRTIO_BUS, host, bus_name);
>> +    VirtioBus *bus = VIRTIO_BUS(qbus);
>> +    bus->info = info;
>> +    qbus->allow_hotplug = 0;
>> +    bus->bus_in_use = false;
>> +    DPRINTF("%s bus created\n", bus_name);
>> +    return bus;
>> +}
> Don't you need a way to destroy the bus again when the proxy device is
> hotunplugged?
>
Yes, you're right I must add a way to destroy the bus, and the devices 
when the proxy is hot unplugged!

Thanks,

Fred

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

[Cornelia Huck]

On Fri, 23 Nov 2012 15:12:45 +0100
Konrad Frederic <fred.konrad@greensocs.com> wrote:

> On 23/11/2012 13:08, Cornelia Huck wrote:
> > On Thu, 22 Nov 2012 15:50:50 +0100
> > fred.konrad@greensocs.com wrote:
> >
> >
> >> +/* Create a virtio bus.  */
> >> +VirtioBus *virtio_bus_new(DeviceState *host, const VirtioBusInfo *info)
> >> +{
> >> +    /*
> >> +     * This is needed, as we want to have different names for each virtio-bus.
> >> +     * If we don't do that, we can't add more than one VirtIODevice.
> >> +     */
> >> +    static int next_virtio_bus;
> >> +    char *bus_name = g_strdup_printf("virtio-bus.%d", next_virtio_bus++);
> > This still has the overflow/id-reuse problem, hasn't it?
> What do you mean by overflow problem ?

If you do a lot of hotplugs (and hotunplugs), at some point
next_virtio_bus will overflow and the code will start to create
virtio-bus.<existing number>. It's a bit of a pathological case, but I
can see it happening on machines with lots of devices that change
rapidly (such as somebody running a test script doing
device_add/device_del).

Maybe use something like the ida stuff the virtio kernel code is using
(don't know whether something similar exists in qemu).

> 
> >
> >> +
> >> +    BusState *qbus = qbus_create(TYPE_VIRTIO_BUS, host, bus_name);
> >> +    VirtioBus *bus = VIRTIO_BUS(qbus);
> >> +    bus->info = info;
> >> +    qbus->allow_hotplug = 0;
> >> +    bus->bus_in_use = false;
> >> +    DPRINTF("%s bus created\n", bus_name);
> >> +    return bus;
> >> +}
> > Don't you need a way to destroy the bus again when the proxy device is
> > hotunplugged?
> >
> Yes, you're right I must add a way to destroy the bus, and the devices 
> when the proxy is hot unplugged!
> 
> Thanks,
> 
> Fred
> 

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

[Konrad Frederic]

On 23/11/2012 13:08, Cornelia Huck wrote:
> On Thu, 22 Nov 2012 15:50:50 +0100
> fred.konrad@greensocs.com wrote:
>
>
>> +/* Create a virtio bus.  */
>> +VirtioBus *virtio_bus_new(DeviceState *host, const VirtioBusInfo *info)
>> +{
>> +    /*
>> +     * This is needed, as we want to have different names for each virtio-bus.
>> +     * If we don't do that, we can't add more than one VirtIODevice.
>> +     */
>> +    static int next_virtio_bus;
>> +    char *bus_name = g_strdup_printf("virtio-bus.%d", next_virtio_bus++);
> This still has the overflow/id-reuse problem, hasn't it?
>
>> +
>> +    BusState *qbus = qbus_create(TYPE_VIRTIO_BUS, host, bus_name);
>> +    VirtioBus *bus = VIRTIO_BUS(qbus);
>> +    bus->info = info;
>> +    qbus->allow_hotplug = 0;
>> +    bus->bus_in_use = false;
>> +    DPRINTF("%s bus created\n", bus_name);
>> +    return bus;
>> +}
> Don't you need a way to destroy the bus again when the proxy device is
> hotunplugged?
>
Is the virtio-pci-* proxy currently supporting hotunplugging ?

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

[Cornelia Huck]

On Mon, 26 Nov 2012 14:55:56 +0100
Konrad Frederic <fred.konrad@greensocs.com> wrote:

> On 23/11/2012 13:08, Cornelia Huck wrote:
> > On Thu, 22 Nov 2012 15:50:50 +0100
> > fred.konrad@greensocs.com wrote:
> >
> >
> >> +/* Create a virtio bus.  */
> >> +VirtioBus *virtio_bus_new(DeviceState *host, const VirtioBusInfo *info)
> >> +{
> >> +    /*
> >> +     * This is needed, as we want to have different names for each virtio-bus.
> >> +     * If we don't do that, we can't add more than one VirtIODevice.
> >> +     */
> >> +    static int next_virtio_bus;
> >> +    char *bus_name = g_strdup_printf("virtio-bus.%d", next_virtio_bus++);
> > This still has the overflow/id-reuse problem, hasn't it?
> >
> >> +
> >> +    BusState *qbus = qbus_create(TYPE_VIRTIO_BUS, host, bus_name);
> >> +    VirtioBus *bus = VIRTIO_BUS(qbus);
> >> +    bus->info = info;
> >> +    qbus->allow_hotplug = 0;
> >> +    bus->bus_in_use = false;
> >> +    DPRINTF("%s bus created\n", bus_name);
> >> +    return bus;
> >> +}
> > Don't you need a way to destroy the bus again when the proxy device is
> > hotunplugged?
> >
> Is the virtio-pci-* proxy currently supporting hotunplugging ?
> 

No idea about virtio-pci devices, but since virtio-ccw will support
hotunplugging, we'll certainly need an interface for that.

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

[Stefan Hajnoczi]

On Thu, Nov 22, 2012 at 03:50:50PM +0100, fred.konrad@greensocs.com wrote:
> +/* Bind the VirtIODevice to the VirtioBus. */
> +void virtio_bus_bind_device(VirtioBus *bus)
> +{
> +    BusState *qbus = BUS(bus);
> +    assert(bus != NULL);
> +    assert(bus->vdev != NULL);
> +    virtio_bind_device(bus->vdev, &(bus->info->virtio_bindings), qbus->parent);
> +}

Should plug and bind be together in a single function?  Binding the
device seems like an internal step that the bus takes when you plug the
device.

> +struct VirtioBusInfo {

This is defining an ad-hoc interface.  QOM has support for interfaces so
that a virtio-pci adapter brovides a VirtioBindingInterface which
VirtioBus can talk to intead of using VirtioBusInfo.

> +    void (*init_cb)(DeviceState *dev);
> +    void (*exit_cb)(DeviceState *dev);

Can _cb be dropped from the name?  Structs with function pointers always
provide "callbacks" so the _cb is unnecessary.  For example, QOM methods
like BusClass->reset() don't include _cb either.

> +    VirtIOBindings virtio_bindings;
> +};
> +
> +struct VirtioBus {
> +    BusState qbus;
> +    bool bus_in_use;

Should bus_in_use basically be bus->vdev != NULL?

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

[Konrad Frederic]

On 23/11/2012 13:23, Stefan Hajnoczi wrote:
> On Thu, Nov 22, 2012 at 03:50:50PM +0100, fred.konrad@greensocs.com wrote:
>> +/* Bind the VirtIODevice to the VirtioBus. */
>> +void virtio_bus_bind_device(VirtioBus *bus)
>> +{
>> +    BusState *qbus = BUS(bus);
>> +    assert(bus != NULL);
>> +    assert(bus->vdev != NULL);
>> +    virtio_bind_device(bus->vdev,&(bus->info->virtio_bindings), qbus->parent);
>> +}
> Should plug and bind be together in a single function?  Binding the
> device seems like an internal step that the bus takes when you plug the
> device.
Maybe we can, but we must init the "transport" device before bind the 
device.

>
>> +struct VirtioBusInfo {
> This is defining an ad-hoc interface.  QOM has support for interfaces so
> that a virtio-pci adapter brovides a VirtioBindingInterface which
> VirtioBus can talk to intead of using VirtioBusInfo.
Can you point me to example in the tree to see how this QOM interfaces 
work ?

>> +    void (*init_cb)(DeviceState *dev);
>> +    void (*exit_cb)(DeviceState *dev);
> Can _cb be dropped from the name?  Structs with function pointers always
> provide "callbacks" so the _cb is unnecessary.  For example, QOM methods
> like BusClass->reset() don't include _cb either.
Ok.
>> +    VirtIOBindings virtio_bindings;
>> +};
>> +
>> +struct VirtioBus {
>> +    BusState qbus;
>> +    bool bus_in_use;
> Should bus_in_use basically be bus->vdev != NULL?
Yes I can do that. :).

Thanks,

Fred

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

[Stefan Hajnoczi]

On Fri, Nov 23, 2012 at 03:21:30PM +0100, Konrad Frederic wrote:
> On 23/11/2012 13:23, Stefan Hajnoczi wrote:
> >On Thu, Nov 22, 2012 at 03:50:50PM +0100, fred.konrad@greensocs.com wrote:
> >>+struct VirtioBusInfo {
> >This is defining an ad-hoc interface.  QOM has support for interfaces so
> >that a virtio-pci adapter brovides a VirtioBindingInterface which
> >VirtioBus can talk to intead of using VirtioBusInfo.
> Can you point me to example in the tree to see how this QOM
> interfaces work ?

hw/stream.h and hw/stream.c

A StreamSlave has one method:

    void (*push)(StreamSlave *obj, unsigned char *buf,
                 size_t len, uint32_t *app);

The xlnx.axi-ethernet device in hw/xilinx_axienet.c is an example of a
class that implements the StreamSlave interface.

Stefan

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

[Andreas Färber]

Am 22.11.2012 15:50, schrieb fred.konrad@greensocs.com:
> diff --git a/hw/virtio-bus.c b/hw/virtio-bus.c
> new file mode 100644
> index 0000000..991b6f5
> --- /dev/null
> +++ b/hw/virtio-bus.c
[...]
> +#define DEBUG_VIRTIO_BUS 1

We probably want to disable debug output by default as done elsewhere?

> +
> +#define DPRINTF(fmt, ...) if (DEBUG_VIRTIO_BUS) {                        \
> +                            printf("virtio_bus: " fmt , ## __VA_ARGS__); \
> +                          }
> +
> +static void virtio_bus_init_cb(VirtioBus *bus);
> +static int virtio_bus_reset(BusState *qbus);
> +
> +static void virtio_bus_class_init(ObjectClass *klass, void *data)
> +{
> +    BusClass *k = BUS_CLASS(klass);
> +    k->reset = virtio_bus_reset;
> +}
> +
> +static TypeInfo virtio_bus_info = {

Somehow you lost "const" here since v1.

> +    .name = TYPE_VIRTIO_BUS,
> +    .parent = TYPE_BUS,
> +    .instance_size = sizeof(VirtioBus),
> +    .class_init = virtio_bus_class_init,
> +};

The BUS()-related changes look good, thanks!

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 v2 1/3] virtio-bus : Introduce VirtioBus.

[Anthony Liguori]

fred.konrad@greensocs.com writes:

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

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

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

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

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

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

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

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

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

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

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

Don't suffix methods with '_cb'.

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

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

But it should be called something like VirtioBusProxy.

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

> +    VirtIOBindings virtio_bindings;

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

Regards,

Anthony Liguori

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

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

[Peter Maydell]

On 26 November 2012 14:33, Anthony Liguori <aliguori@us.ibm.com> wrote:
> VirtioBusInfo is not a great name.  This is a proxy class that allows
> for a device to implement the virtio bus interface.
>
> This could be done as an interface but since nothing else uses
> interfaces, I'm okay with something like this.  But the first argument
> ought to be an opaque for all methods.

We have at least one user of Interface in the tree IIRC.
I'd much rather we did this the right way -- the only reason
it's the way Fred has coded it is that there's no obvious
body of code in the tree to copy, so we're thrashing around
a bit. If you tell us what the correct set of structs/classes/
interfaces/etc is then we can implement it :-)

-- PMM

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

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 26 November 2012 14:33, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> VirtioBusInfo is not a great name.  This is a proxy class that allows
>> for a device to implement the virtio bus interface.
>>
>> This could be done as an interface but since nothing else uses
>> interfaces, I'm okay with something like this.  But the first argument
>> ought to be an opaque for all methods.
>
> We have at least one user of Interface in the tree IIRC.
> I'd much rather we did this the right way -- the only reason
> it's the way Fred has coded it is that there's no obvious
> body of code in the tree to copy, so we're thrashing around
> a bit. If you tell us what the correct set of structs/classes/
> interfaces/etc is then we can implement it :-)

I really think extending virtio-bus to a virtio-pci-bus and then
initializing it with a link to the PCI device is the best approach.

It's by far the simpliest approach in terms of coding.

Did I explain it adequately?  To recap:

virtio-bus extends bus-state
 - implements everything that VirtIOBindings implements as methods

virtio-pci-bus extends virtio-bus
 - is constructed with a pointer to a PCIDevice
 - implements the methods necessary to be a virtio bus

virtio-device extends device-state
 - implements methods used by virtio-bus

Regards,

Anthony Liguori

>
> -- PMM

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

[Konrad Frederic]

On 26/11/2012 17:59, Anthony Liguori wrote:
> Peter Maydell<peter.maydell@linaro.org>  writes:
>
>> On 26 November 2012 14:33, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>>> VirtioBusInfo is not a great name.  This is a proxy class that allows
>>> for a device to implement the virtio bus interface.
>>>
>>> This could be done as an interface but since nothing else uses
>>> interfaces, I'm okay with something like this.  But the first argument
>>> ought to be an opaque for all methods.
>> We have at least one user of Interface in the tree IIRC.
>> I'd much rather we did this the right way -- the only reason
>> it's the way Fred has coded it is that there's no obvious
>> body of code in the tree to copy, so we're thrashing around
>> a bit. If you tell us what the correct set of structs/classes/
>> interfaces/etc is then we can implement it :-)
> I really think extending virtio-bus to a virtio-pci-bus and then
> initializing it with a link to the PCI device is the best approach.
>
> It's by far the simpliest approach in terms of coding.
>
> Did I explain it adequately?  To recap:
>
> virtio-bus extends bus-state
>   - implements everything that VirtIOBindings implements as methods
>
> virtio-pci-bus extends virtio-bus
>   - is constructed with a pointer to a PCIDevice
>   - implements the methods necessary to be a virtio bus
I still have trouble with that ^^.

virtio-pci-bus extends virtio-bus so I put something like that :

static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
{
     BusClass *bc = BUS_CLASS(klass);
     VirtioBusClass *k = VIRTIO_BUS_CLASS(klass);
     /* VirtIOBindings */
     k->notify = virtio_pci_notify;
     k->save_config = virtio_pci_save_config;
     k->load_config = virtio_pci_load_config;
     k->save_queue = virtio_pci_save_queue;
     k->load_queue = virtio_pci_load_queue;
     k->get_features = virtio_pci_get_features;
     k->query_guest_notifiers = virtio_pci_query_guest_notifiers;
     k->set_host_notifier = virtio_pci_set_host_notifier;
     k->set_guest_notifiers = virtio_pci_set_guest_notifiers;
     k->vmstate_change = virtio_pci_vmstate_change;
     /*
      * TODO : Init and exit function.
      * void (*init)(void *opaque);
      * void (*exit)(void *opaque);
      */
}

static TypeInfo virtio_pci_bus_info = {
     .name          = TYPE_VIRTIO_PCI_BUS,
     .parent        = TYPE_VIRTIO_BUS,
     .class_init    = virtio_pci_bus_class_init,
};

and I have VirtioDevice which extends DeviceState like that :

static void virtio_device_class_init(ObjectClass *klass, void *data)
{
     /* Set the default value here. */
     DeviceClass *dc = DEVICE_CLASS(klass);
     dc->bus_type = TYPE_VIRTIO_BUS;
     dc->init = virtio_device_init;
}

static TypeInfo virtio_device_info = {
     .name = TYPE_VIRTIO_DEVICE,
     .parent = TYPE_DEVICE,
     .instance_size = sizeof(VirtioDeviceState),
     /* Abstract the virtio-device */
     .class_init = virtio_device_class_init,
     .abstract = true,
     .class_size = sizeof(VirtioDeviceClass),
};

The problem is that the virtio devices can't be connected to the 
virtio-pci-bus even if it extends virtio-bus because TYPE_VIRTIO_BUS != 
TYPE_VIRTIO_PCI_BUS.

Did I miss something ?

Thanks,

Fred


> virtio-device extends device-state
>   - implements methods used by virtio-bus
>
> Regards,
>
> Anthony Liguori
>
>> -- PMM

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

[Peter Maydell]

On 29 November 2012 12:37, Konrad Frederic <fred.konrad@greensocs.com> wrote:
> On 26/11/2012 17:59, Anthony Liguori wrote:
>> virtio-pci-bus extends virtio-bus
>>   - is constructed with a pointer to a PCIDevice
>>   - implements the methods necessary to be a virtio bus
>
> I still have trouble with that ^^.

> The problem is that the virtio devices can't be connected to the
> virtio-pci-bus even if it extends virtio-bus because TYPE_VIRTIO_BUS !=
> TYPE_VIRTIO_PCI_BUS.

Conceptually it ought to work I think: if the bus is-a TYPE_VIRTIO_BUS
then we should permit plugging in even if the actual bus object happens
to be an instance of a subclass.

I suspect that qbus_find_recursive should be doing an
object_class_dynamic_cast() to check that the bus is of a suitable
type, rather than the
    (strcmp(object_get_typename(OBJECT(bus)), bus_typename) != 0)
which it does at the moment.

-- PMM

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

[Konrad Frederic]

On 29/11/2012 14:09, Peter Maydell wrote:
> On 29 November 2012 12:37, Konrad Frederic<fred.konrad@greensocs.com>  wrote:
>> On 26/11/2012 17:59, Anthony Liguori wrote:
>>> virtio-pci-bus extends virtio-bus
>>>    - is constructed with a pointer to a PCIDevice
>>>    - implements the methods necessary to be a virtio bus
>> I still have trouble with that ^^.
>> The problem is that the virtio devices can't be connected to the
>> virtio-pci-bus even if it extends virtio-bus because TYPE_VIRTIO_BUS !=
>> TYPE_VIRTIO_PCI_BUS.
> Conceptually it ought to work I think: if the bus is-a TYPE_VIRTIO_BUS
> then we should permit plugging in even if the actual bus object happens
> to be an instance of a subclass.
Yes, is my implementation doing the right thing ?
I mean is the virtio-pci-bus a virtio-bus ?

>
> I suspect that qbus_find_recursive should be doing an
> object_class_dynamic_cast() to check that the bus is of a suitable
> type, rather than the
>      (strcmp(object_get_typename(OBJECT(bus)), bus_typename) != 0)
> which it does at the moment.
Yes, but we can cast VIRTIO_BUS in BUS no ?
So in this case we could plug VirtioDevice in BUS and that's not what we 
want ?

>
> -- PMM
>

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

[Peter Maydell]

On 29 November 2012 13:47, Konrad Frederic <fred.konrad@greensocs.com> wrote:
> On 29/11/2012 14:09, Peter Maydell wrote:
>> I suspect that qbus_find_recursive should be doing an
>> object_class_dynamic_cast() to check that the bus is of a suitable
>> type, rather than the
>>      (strcmp(object_get_typename(OBJECT(bus)), bus_typename) != 0)
>> which it does at the moment.
>
> Yes, but we can cast VIRTIO_BUS in BUS no ?
> So in this case we could plug VirtioDevice in BUS and that's not what we
> want ?

I don't understand what you're asking. qbus_find_recursive()
looks for a bus which might be specified either by name or by
bus type. At the moment it insists on an exact type match
(so if you ask for a virtio-bus you have to provide a virtio-bus
object, not a subclass of virtio-bus); it should do a dynamic
cast, so anything which is a virtio-bus or a subclass of that
will do. Yes, if you say "please find me any bus which is a
bus of any kind" then you'll get something wrong back -- so
don't do that. In particular, since VirtioDevice sets its
bus_type to TYPE_VIRTIO_BUS then we will ask for that, and
anything which isn't a virtio bus (or subclass) won't be found.

-- PMM

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

[Andreas Färber]

Am 29.11.2012 14:47, schrieb Konrad Frederic:
> On 29/11/2012 14:09, Peter Maydell wrote:
>> On 29 November 2012 12:37, Konrad Frederic<fred.konrad@greensocs.com> 
>> wrote:
>>> On 26/11/2012 17:59, Anthony Liguori wrote:
>>>> virtio-pci-bus extends virtio-bus
>>>>    - is constructed with a pointer to a PCIDevice
>>>>    - implements the methods necessary to be a virtio bus
>>> I still have trouble with that ^^.
>>> The problem is that the virtio devices can't be connected to the
>>> virtio-pci-bus even if it extends virtio-bus because TYPE_VIRTIO_BUS !=
>>> TYPE_VIRTIO_PCI_BUS.
>> Conceptually it ought to work I think: if the bus is-a TYPE_VIRTIO_BUS
>> then we should permit plugging in even if the actual bus object happens
>> to be an instance of a subclass.
> Yes, is my implementation doing the right thing ?
> I mean is the virtio-pci-bus a virtio-bus ?

In your v3 patchset no. In the inline code yes, via
virtio_pci_bus_info's .parent.

>> I suspect that qbus_find_recursive should be doing an
>> object_class_dynamic_cast() to check that the bus is of a suitable
>> type, rather than the
>>      (strcmp(object_get_typename(OBJECT(bus)), bus_typename) != 0)
>> which it does at the moment.
> Yes, but we can cast VIRTIO_BUS in BUS no ?
> So in this case we could plug VirtioDevice in BUS and that's not what we
> want ?

You would want to check whether object_class_dynamic_cast(OBJECT(bus),
TYPE_VIRTIO_BUS) == NULL. This should evaluate to true if the bus is not
a virtio-bus.

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 v2 1/3] virtio-bus : Introduce VirtioBus.

[Konrad Frederic]

On 29/11/2012 14:55, Andreas Färber wrote:
> Am 29.11.2012 14:47, schrieb Konrad Frederic:
>> On 29/11/2012 14:09, Peter Maydell wrote:
>>> On 29 November 2012 12:37, Konrad Frederic<fred.konrad@greensocs.com>
>>> wrote:
>>>> On 26/11/2012 17:59, Anthony Liguori wrote:
>>>>> virtio-pci-bus extends virtio-bus
>>>>>     - is constructed with a pointer to a PCIDevice
>>>>>     - implements the methods necessary to be a virtio bus
>>>> I still have trouble with that ^^.
>>>> The problem is that the virtio devices can't be connected to the
>>>> virtio-pci-bus even if it extends virtio-bus because TYPE_VIRTIO_BUS !=
>>>> TYPE_VIRTIO_PCI_BUS.
>>> Conceptually it ought to work I think: if the bus is-a TYPE_VIRTIO_BUS
>>> then we should permit plugging in even if the actual bus object happens
>>> to be an instance of a subclass.
>> Yes, is my implementation doing the right thing ?
>> I mean is the virtio-pci-bus a virtio-bus ?
> In your v3 patchset no. In the inline code yes, via
> virtio_pci_bus_info's .parent.
>
>>> I suspect that qbus_find_recursive should be doing an
>>> object_class_dynamic_cast() to check that the bus is of a suitable
>>> type, rather than the
>>>       (strcmp(object_get_typename(OBJECT(bus)), bus_typename) != 0)
>>> which it does at the moment.
>> Yes, but we can cast VIRTIO_BUS in BUS no ?
>> So in this case we could plug VirtioDevice in BUS and that's not what we
>> want ?
> You would want to check whether object_class_dynamic_cast(OBJECT(bus),
> TYPE_VIRTIO_BUS) == NULL. This should evaluate to true if the bus is not
> a virtio-bus.
Yes, ok I got it!

Thanks,

Fred

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

[Anthony Liguori]

Konrad Frederic <fred.konrad@greensocs.com> writes:

> On 26/11/2012 17:59, Anthony Liguori wrote:
>> Peter Maydell<peter.maydell@linaro.org>  writes:
>>
>>> On 26 November 2012 14:33, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>>>> VirtioBusInfo is not a great name.  This is a proxy class that allows
>>>> for a device to implement the virtio bus interface.
>>>>
>>>> This could be done as an interface but since nothing else uses
>>>> interfaces, I'm okay with something like this.  But the first argument
>>>> ought to be an opaque for all methods.
>>> We have at least one user of Interface in the tree IIRC.
>>> I'd much rather we did this the right way -- the only reason
>>> it's the way Fred has coded it is that there's no obvious
>>> body of code in the tree to copy, so we're thrashing around
>>> a bit. If you tell us what the correct set of structs/classes/
>>> interfaces/etc is then we can implement it :-)
>> I really think extending virtio-bus to a virtio-pci-bus and then
>> initializing it with a link to the PCI device is the best approach.
>>
>> It's by far the simpliest approach in terms of coding.
>>
>> Did I explain it adequately?  To recap:
>>
>> virtio-bus extends bus-state
>>   - implements everything that VirtIOBindings implements as methods
>>
>> virtio-pci-bus extends virtio-bus
>>   - is constructed with a pointer to a PCIDevice
>>   - implements the methods necessary to be a virtio bus
> I still have trouble with that ^^.
>
> virtio-pci-bus extends virtio-bus so I put something like that :
>
> static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
> {
>      BusClass *bc = BUS_CLASS(klass);
>      VirtioBusClass *k = VIRTIO_BUS_CLASS(klass);
>      /* VirtIOBindings */
>      k->notify = virtio_pci_notify;
>      k->save_config = virtio_pci_save_config;
>      k->load_config = virtio_pci_load_config;
>      k->save_queue = virtio_pci_save_queue;
>      k->load_queue = virtio_pci_load_queue;
>      k->get_features = virtio_pci_get_features;
>      k->query_guest_notifiers = virtio_pci_query_guest_notifiers;
>      k->set_host_notifier = virtio_pci_set_host_notifier;
>      k->set_guest_notifiers = virtio_pci_set_guest_notifiers;
>      k->vmstate_change = virtio_pci_vmstate_change;
>      /*
>       * TODO : Init and exit function.
>       * void (*init)(void *opaque);
>       * void (*exit)(void *opaque);
>       */
> }
>
> static TypeInfo virtio_pci_bus_info = {
>      .name          = TYPE_VIRTIO_PCI_BUS,
>      .parent        = TYPE_VIRTIO_BUS,
>      .class_init    = virtio_pci_bus_class_init,
> };
>
> and I have VirtioDevice which extends DeviceState like that :
>
> static void virtio_device_class_init(ObjectClass *klass, void *data)
> {
>      /* Set the default value here. */
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      dc->bus_type = TYPE_VIRTIO_BUS;
>      dc->init = virtio_device_init;
> }
>
> static TypeInfo virtio_device_info = {
>      .name = TYPE_VIRTIO_DEVICE,
>      .parent = TYPE_DEVICE,
>      .instance_size = sizeof(VirtioDeviceState),
>      /* Abstract the virtio-device */
>      .class_init = virtio_device_class_init,
>      .abstract = true,
>      .class_size = sizeof(VirtioDeviceClass),
> };
>
> The problem is that the virtio devices can't be connected to the 
> virtio-pci-bus even if it extends virtio-bus because TYPE_VIRTIO_BUS != 
> TYPE_VIRTIO_PCI_BUS.

That's just a bug.  See the patch I just sent out to fix it.

The type check is too strict in qdev_device_add().

Regards,

Anthony Liguori

>
> Did I miss something ?
>
> Thanks,
>
> Fred
>
>
>> virtio-device extends device-state
>>   - implements methods used by virtio-bus
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>> -- PMM

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

[Andreas Färber]

Am 26.11.2012 15:33, schrieb Anthony Liguori:
> fred.konrad@greensocs.com writes:
>> +#define DEBUG_VIRTIO_BUS 1
>> +
>> +#define DPRINTF(fmt, ...) if (DEBUG_VIRTIO_BUS) {                        \
>> +                            printf("virtio_bus: " fmt , ## __VA_ARGS__); \
>> +                          }
> 
> #ifdef DEBUG_VIRTIO_BUS
> #define DPRINTF(fmt, ...) ...
> #else
> #define DPRINTF(fmt, ...) do { } while (0)
> #endif
> 
> You're leaving a dangling if clause which can do very strange things if
> used before an else statement.

If you look at the change history, this was a change in a reaction to me
pointing to a proposal by Samsung. I see your point with the else
statement, that can be circumvented by adding else {}. The reason is to
avoid DPRINTF()s bitrotting because your "do { } while (0)" performs no
compile-tests on "fmt, ..." arguments.

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 v2 1/3] virtio-bus : Introduce VirtioBus.

[Anthony Liguori]

Andreas Färber <afaerber@suse.de> writes:

> Am 26.11.2012 15:33, schrieb Anthony Liguori:
>> fred.konrad@greensocs.com writes:
>>> +#define DEBUG_VIRTIO_BUS 1
>>> +
>>> +#define DPRINTF(fmt, ...) if (DEBUG_VIRTIO_BUS) {                        \
>>> +                            printf("virtio_bus: " fmt , ## __VA_ARGS__); \
>>> +                          }
>> 
>> #ifdef DEBUG_VIRTIO_BUS
>> #define DPRINTF(fmt, ...) ...
>> #else
>> #define DPRINTF(fmt, ...) do { } while (0)
>> #endif
>> 
>> You're leaving a dangling if clause which can do very strange things if
>> used before an else statement.
>
> If you look at the change history, this was a change in a reaction to me
> pointing to a proposal by Samsung. I see your point with the else
> statement, that can be circumvented by adding else {}. The reason is to
> avoid DPRINTF()s bitrotting because your "do { } while (0)" performs no
> compile-tests on "fmt, ..." arguments.

This is a well-used idiom in QEMU.  We shouldn't try to change idioms in
random patch series.

If you want to change the way we do DPRINTF(), it should be done
globally in its own series.

Regards,

Anthony Liguori

>
> 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 v2 1/3] virtio-bus : Introduce VirtioBus.

[Konrad Frederic]

On 26/11/2012 15:33, Anthony Liguori wrote:
> fred.konrad@greensocs.com writes:
>
>> From: KONRAD Frederic<fred.konrad@greensocs.com>
>>
>> This patch create a new VirtioBus, which can be added to Virtio transports like
>> virtio-pci, virtio-mmio,...
>>
>> One VirtIODevice can be connected to this device, like virtio-blk in the 3rd
>> patch.
>>
>> The VirtioBus shares through a VirtioBusInfo structure :
>>
>>      * two callbacks with the transport : init_cb and exit_cb, which must be
>>        called by the VirtIODevice, after the initialization and before the
>>        destruction, to put the right PCI IDs and/or stop the event fd.
>>
>>      * a VirtIOBindings structure.
>>
>> Signed-off-by: KONRAD Frederic<fred.konrad@greensocs.com>
>> ---
>>   hw/Makefile.objs |   1 +
>>   hw/virtio-bus.c  | 148 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   hw/virtio-bus.h  |  58 ++++++++++++++++++++++
>>   3 files changed, 207 insertions(+)
>>   create mode 100644 hw/virtio-bus.c
>>   create mode 100644 hw/virtio-bus.h
>>
>> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
>> index ea46f81..bd14d1b 100644
>> --- a/hw/Makefile.objs
>> +++ b/hw/Makefile.objs
>> @@ -2,6 +2,7 @@ common-obj-y = usb/ ide/
>>   common-obj-y += loader.o
>>   common-obj-$(CONFIG_VIRTIO) += virtio-console.o
>>   common-obj-$(CONFIG_VIRTIO) += virtio-rng.o
>> +common-obj-$(CONFIG_VIRTIO) += virtio-bus.o
>>   common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
>>   common-obj-y += fw_cfg.o
>>   common-obj-$(CONFIG_PCI) += pci.o pci_bridge.o pci_bridge_dev.o
>> diff --git a/hw/virtio-bus.c b/hw/virtio-bus.c
>> new file mode 100644
>> index 0000000..991b6f5
>> --- /dev/null
>> +++ b/hw/virtio-bus.c
>> @@ -0,0 +1,148 @@
>> +/*
>> + * VirtioBus
>> + *
>> + *  Copyright (C) 2012 : GreenSocs Ltd
>> + *      http://www.greensocs.com/ , email: info@greensocs.com
>> + *
>> + *  Developed by :
>> + *  Frederic Konrad<fred.konrad@greensocs.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation, either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along
>> + * with this program; if not, see<http://www.gnu.org/licenses/>.
>> + *
>> + */
>> +
>> +#include "hw.h"
>> +#include "qemu-error.h"
>> +#include "qdev.h"
>> +#include "virtio-bus.h"
>> +#include "virtio.h"
>> +
>> +#define DEBUG_VIRTIO_BUS 1
>> +
>> +#define DPRINTF(fmt, ...) if (DEBUG_VIRTIO_BUS) {                        \
>> +                            printf("virtio_bus: " fmt , ## __VA_ARGS__); \
>> +                          }
> #ifdef DEBUG_VIRTIO_BUS
> #define DPRINTF(fmt, ...) ...
> #else
> #define DPRINTF(fmt, ...) do { } while (0)
> #endif
>
> You're leaving a dangling if clause which can do very strange things if
> used before an else statement.
>
>> +
>> +static void virtio_bus_init_cb(VirtioBus *bus);
>> +static int virtio_bus_reset(BusState *qbus);
> You should rearrange the code to avoid forward declarations of static functions.
>
>> +
>> +static void virtio_bus_class_init(ObjectClass *klass, void *data)
>> +{
>> +    BusClass *k = BUS_CLASS(klass);
>> +    k->reset = virtio_bus_reset;
>> +}
>> +
>> +static TypeInfo virtio_bus_info = {
>> +    .name = TYPE_VIRTIO_BUS,
>> +    .parent = TYPE_BUS,
>> +    .instance_size = sizeof(VirtioBus),
>> +    .class_init = virtio_bus_class_init,
>> +};
>> +
>> +/* Reset the bus */
>> +static int virtio_bus_reset(BusState *qbus)
>> +{
>> +    VirtioBus *bus = VIRTIO_BUS(qbus);
>> +    if (bus->bus_in_use) {
>> +        virtio_reset(bus->vdev);
>> +    }
>> +    return 1;
>> +}
>> +
>> +/* Plug the VirtIODevice */
>> +int virtio_bus_plug_device(VirtioBus *bus, VirtIODevice *vdev)
>> +{
>> +    BusState *qbus = BUS(bus);
>> +    /*
>> +     * This is a problem, when bus= option is not set, the last created
>> +     * virtio-bus is used. So it give the following error.
>> +     */
>> +    DPRINTF("plug device into %s.\n", qbus->name);
>> +    if (bus->bus_in_use) {
>> +        error_report("%s in use.\n", qbus->name);
>> +        return -1;
>> +    }
>> +    bus->bus_in_use = true;
>> +
>> +    /* keep the VirtIODevice in the VirtioBus. */
>> +    bus->vdev = vdev;
>> +
>> +    /* call the "transport" callback. */
>> +    virtio_bus_init_cb(bus);
>> +    return 0;
>> +}
> I think the desired semantics here could be achieved by modifying
> qbus_find_recursive() to take an optional argument which then causes it
> to only return a "!full" bus.  You can then add a member to BusState to
> indicate whether the bus is full or not.
>
> You can then set the parameter qdev_device_add() and it should Just Work.
Ok, so I need to modify qdev_monitor.c to add an option ( in command 
line ) to set this optional argument ?

>> +
>> +/* Create a virtio bus.  */
>> +VirtioBus *virtio_bus_new(DeviceState *host, const VirtioBusInfo *info)
>> +{
>> +    /*
>> +     * This is needed, as we want to have different names for each virtio-bus.
>> +     * If we don't do that, we can't add more than one VirtIODevice.
>> +     */
>> +    static int next_virtio_bus;
>> +    char *bus_name = g_strdup_printf("virtio-bus.%d",
>> next_virtio_bus++);
> It's not needed..  qdev will do this automagically as it turns out.
Really ? If I use qbus_create(TYPE_VIRTIO_BUS, host, NULL) and I add two 
virtio-pci in qemu monitor, I have two virtio-bus named "virtio-bus.0".

Did I miss something ?
>> +
>> +    BusState *qbus = qbus_create(TYPE_VIRTIO_BUS, host, bus_name);
>> +    VirtioBus *bus = VIRTIO_BUS(qbus);
>> +    bus->info = info;
>> +    qbus->allow_hotplug = 0;
>> +    bus->bus_in_use = false;
>> +    DPRINTF("%s bus created\n", bus_name);
>> +    return bus;
>> +}
>> +
>> +/* Bind the VirtIODevice to the VirtioBus. */
>> +void virtio_bus_bind_device(VirtioBus *bus)
>> +{
>> +    BusState *qbus = BUS(bus);
>> +    assert(bus != NULL);
>> +    assert(bus->vdev != NULL);
>> +    virtio_bind_device(bus->vdev,&(bus->info->virtio_bindings), qbus->parent);
>> +}
>> +
>> +/*
>> + * Transport independent init.
>> + * This must be called after VirtIODevice initialization.
>> + */
>> +static void virtio_bus_init_cb(VirtioBus *bus)
>> +{
>> +    BusState *qbus = BUS(bus);
>> +    assert(bus->info->init_cb != NULL);
>> +    bus->info->init_cb(qbus->parent);
>> +}
>> +
>> +/*
>> + * Transport independent exit.
>> + * This must be called by the VirtIODevice before destroying it.
>> + */
>> +void virtio_bus_exit_cb(VirtioBus *bus)
>> +{
>> +    BusState *qbus = BUS(bus);
>> +    assert(bus->info->exit_cb != NULL);
>> +    bus->info->exit_cb(qbus->parent);
>> +    bus->bus_in_use = false;
>> +}
>> +
>> +/* Return the virtio device id of the plugged device. */
>> +uint16_t get_virtio_device_id(VirtioBus *bus)
>> +{
>> +    return bus->vdev->device_id;
>> +}
>> +
>> +static void virtio_register_types(void)
>> +{
>> +    type_register_static(&virtio_bus_info);
>> +}
>> +
>> +type_init(virtio_register_types)
>> diff --git a/hw/virtio-bus.h b/hw/virtio-bus.h
>> new file mode 100644
>> index 0000000..31aad53
>> --- /dev/null
>> +++ b/hw/virtio-bus.h
>> @@ -0,0 +1,58 @@
>> +/*
>> + * VirtioBus
>> + *
>> + *  Copyright (C) 2012 : GreenSocs Ltd
>> + *      http://www.greensocs.com/ , email: info@greensocs.com
>> + *
>> + *  Developed by :
>> + *  Frederic Konrad<fred.konrad@greensocs.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation, either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along
>> + * with this program; if not, see<http://www.gnu.org/licenses/>.
>> + *
>> + */
>> +
>> +#ifndef VIRTIO_BUS_H
>> +#define VIRTIO_BUS_H
>> +
>> +#include "qdev.h"
>> +#include "sysemu.h"
>> +#include "virtio.h"
>> +
>> +#define TYPE_VIRTIO_BUS "virtio-bus"
>> +#define VIRTIO_BUS(obj) OBJECT_CHECK(VirtioBus, (obj), TYPE_VIRTIO_BUS)
>> +
>> +typedef struct VirtioBus VirtioBus;
>> +typedef struct VirtioBusInfo VirtioBusInfo;
>> +
>> +struct VirtioBusInfo {
>> +    void (*init_cb)(DeviceState *dev);
>> +    void (*exit_cb)(DeviceState *dev);
> Don't suffix methods with '_cb'.
>
> VirtioBusInfo is not a great name.  This is a proxy class that allows
> for a device to implement the virtio bus interface.
>
> This could be done as an interface but since nothing else uses
> interfaces, I'm okay with something like this.  But the first argument
> ought to be an opaque for all methods.
>
> But it should be called something like VirtioBusProxy.
>
> BTW, comments describing the role of this struct would be very helpful.
>
>> +    VirtIOBindings virtio_bindings;
> Everything that's in VirtIOBindings should be methods of the
> VirtioBusClass (which needs to be defined here).
Ok, so I can put all the virtio_bindings "methods" in this VirtioBusProxy ?

>
> Regards,
>
> Anthony Liguori
Thanks,

Fred

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

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

[Stefan Hajnoczi]

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

This should be:

#define DEBUG_VIRTIO_BUS 1

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

This way there's no dangling else statement problem.  But it also
ensures the we always compile the debug print, thereby avoiding bitrot
you get from #ifdef ... #else ... #endif.

Stefan

[Qemu-devel] [RFC PATCH v2 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 | 152 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-pci.h |   6 +++
 2 files changed, 158 insertions(+)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 71f4fb5..78aa5e8 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"
@@ -1117,15 +1118,166 @@ static TypeInfo virtio_scsi_info = {
     .instance_size = sizeof(VirtIOPCIProxy),
     .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 */
+    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;
+
+    }
+
+    /* 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);
+    DeviceState *qdev = DEVICE(dev);
+
+    /* create a virtio-bus and attach virtio-pci to it */
+    s->bus = virtio_bus_new(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);
     type_register_static(&virtio_rng_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 b58d9a2..a7a9847 100644
--- a/hw/virtio-pci.h
+++ b/hw/virtio-pci.h
@@ -20,6 +20,7 @@
 #include "virtio-rng.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. */
@@ -33,6 +34,7 @@ typedef struct {
 
 typedef struct {
     PCIDevice pci_dev;
+    /* This should be removed */
     VirtIODevice *vdev;
     MemoryRegion bar;
     uint32_t flags;
@@ -51,10 +53,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

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

[Cornelia Huck]

On Thu, 22 Nov 2012 15:50:51 +0100
fred.konrad@greensocs.com wrote:


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

Doesn't the exit function need to be symmetric to the init function?

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

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

[Stefan Hajnoczi]

On Thu, Nov 22, 2012 at 03:50:51PM +0100, fred.konrad@greensocs.com wrote:
> +static const struct VirtioBusInfo virtio_bus_info = {
> +    .init_cb = virtio_pci_init_cb,
> +    .exit_cb = virtio_pci_exit_cb,
> +
> +    .virtio_bindings = {

Eventually VirtIOBindings can probably be inlined into VirtioBusInfo.  I
don't see a need for separate structs.

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

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

[Peter Maydell]

On 23 November 2012 12:29, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Thu, Nov 22, 2012 at 03:50:51PM +0100, fred.konrad@greensocs.com wrote:
>> +static const struct VirtioBusInfo virtio_bus_info = {
>> +    .init_cb = virtio_pci_init_cb,
>> +    .exit_cb = virtio_pci_exit_cb,
>> +
>> +    .virtio_bindings = {
>
> Eventually VirtIOBindings can probably be inlined into VirtioBusInfo.  I
> don't see a need for separate structs.

I agree. It might (or might not) be convenient to retain it
temporarily while converting all the transports, but
VirtIOBindings is part of the old code which we're trying
to refactor here, and I'd expect it to go away when we're done.

-- PMM

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

[Konrad Frederic]

On 23/11/2012 13:34, Peter Maydell wrote:
> On 23 November 2012 12:29, Stefan Hajnoczi<stefanha@redhat.com>  wrote:
>> On Thu, Nov 22, 2012 at 03:50:51PM +0100, fred.konrad@greensocs.com wrote:
>>> +static const struct VirtioBusInfo virtio_bus_info = {
>>> +    .init_cb = virtio_pci_init_cb,
>>> +    .exit_cb = virtio_pci_exit_cb,
>>> +
>>> +    .virtio_bindings = {
>> Eventually VirtIOBindings can probably be inlined into VirtioBusInfo.  I
>> don't see a need for separate structs.
> I agree. It might (or might not) be convenient to retain it
> temporarily while converting all the transports, but
> VirtIOBindings is part of the old code which we're trying
> to refactor here, and I'd expect it to go away when we're done.
>
> -- PMM
Yes, for the moment, I didn't refactor this VirtIOBindings, so it is 
better to separate struct to keep the virtiodevice binding function.

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

[Peter Maydell]

On 23 November 2012 14:23, Konrad Frederic <fred.konrad@greensocs.com> wrote:
> On 23/11/2012 13:34, Peter Maydell wrote:
>> On 23 November 2012 12:29, Stefan Hajnoczi<stefanha@redhat.com>  wrote:
>>> Eventually VirtIOBindings can probably be inlined into VirtioBusInfo.  I
>>> don't see a need for separate structs.
>>
>> I agree. It might (or might not) be convenient to retain it
>> temporarily while converting all the transports, but
>> VirtIOBindings is part of the old code which we're trying
>> to refactor here, and I'd expect it to go away when we're done.

> Yes, for the moment, I didn't refactor this VirtIOBindings, so it
> is better to separate struct to keep the virtiodevice binding function.

Where you're deliberately not changing something as a temporary
step you need to comment it to make that clear. Otherwise
people trying to review the code won't be able to tell...

-- PMM

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

[Konrad Frederic]

On 23/11/2012 15:26, Peter Maydell wrote:
> On 23 November 2012 14:23, Konrad Frederic<fred.konrad@greensocs.com>  wrote:
>> On 23/11/2012 13:34, Peter Maydell wrote:
>>> On 23 November 2012 12:29, Stefan Hajnoczi<stefanha@redhat.com>   wrote:
>>>> Eventually VirtIOBindings can probably be inlined into VirtioBusInfo.  I
>>>> don't see a need for separate structs.
>>> I agree. It might (or might not) be convenient to retain it
>>> temporarily while converting all the transports, but
>>> VirtIOBindings is part of the old code which we're trying
>>> to refactor here, and I'd expect it to go away when we're done.
>> Yes, for the moment, I didn't refactor this VirtIOBindings, so it
>> is better to separate struct to keep the virtiodevice binding function.
> Where you're deliberately not changing something as a temporary
> step you need to comment it to make that clear. Otherwise
> people trying to review the code won't be able to tell...
>
> -- PMM
Ok, sorry for that.

Fred

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

[Anthony Liguori]

fred.konrad@greensocs.com writes:

> 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 | 152 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/virtio-pci.h |   6 +++
>  2 files changed, 158 insertions(+)
>
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 71f4fb5..78aa5e8 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"
> @@ -1117,15 +1118,166 @@ static TypeInfo virtio_scsi_info = {
>      .instance_size = sizeof(VirtIOPCIProxy),
>      .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 */
> +    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;
> +
> +    }
> +
> +    /* 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,
> +    }
> +};

So this looks wrong.

The interactions should roughly look like:

(1) VirtioDevice only interacts with VirtioBus through it's class
    methods.  'notify' is a method.

(2) Since VirtioBus is a descrete object, it needs to interact with
    whatever the actual bus is.  There are two ways to do this:

    (a) Each bus-type can sub-class VirtioBus and implement each method
        using a private interface.  This is probably the easiest
        approach because you can just give VirtioPCIBus a pointer to the
        PCIDevice, and then implement the methods within VirtioPCIBus.

    (b) There can be only one VirtioBus object type with a well defined
        proxy interface.  This should basically mirror the VirtioBus
        class methods.  VirtioPCI can then implement this proxy
        interface.

Seeing how the code is turning out, I think 2.a would require the least
amount of coding.

> +
> +static int virtiopci_qdev_init(PCIDevice *dev)
> +{
> +    VirtIOPCIProxy *s = DO_UPCAST(VirtIOPCIProxy, pci_dev, dev);
> +    DeviceState *qdev = DEVICE(dev);
> +
> +    /* create a virtio-bus and attach virtio-pci to it */
> +    s->bus = virtio_bus_new(qdev, &virtio_bus_info);

You basically end up with virtio_pci_bus_new() here and you pass 'dev'
to it.  VirtioPCIBus is a 'friend' to VirtioPCI so it can access it's
private data.

> +
> +    return 0;
> +}
> +

Regards,

Anthony Liguori

> +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);
>      type_register_static(&virtio_rng_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 b58d9a2..a7a9847 100644
> --- a/hw/virtio-pci.h
> +++ b/hw/virtio-pci.h
> @@ -20,6 +20,7 @@
>  #include "virtio-rng.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. */
> @@ -33,6 +34,7 @@ typedef struct {
>  
>  typedef struct {
>      PCIDevice pci_dev;
> +    /* This should be removed */
>      VirtIODevice *vdev;
>      MemoryRegion bar;
>      uint32_t flags;
> @@ -51,10 +53,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 v2 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 | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-blk.h | 10 +++++++
 2 files changed, 92 insertions(+)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index e25cc96..d8263c3 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,83 @@ void virtio_blk_exit(VirtIODevice *vdev)
     blockdev_mark_auto_del(s->bs);
     virtio_cleanup(vdev);
 }
+
+static int virtio_blk_qdev_init(DeviceState *qdev)
+{
+    VirtIOBLKState *s = VIRTIO_BLK(qdev);
+    /*
+     * When the 'bus=' option is not set, the default bus is the last created.
+     * This is a problem because only one VirtIODevice can be plugged.
+     */
+    VirtioBus *bus = VIRTIO_BUS(qdev_get_parent_bus(qdev));
+
+    /* init the device. */
+    VirtIODevice *vdev = virtio_blk_init(qdev, &s->blk);
+    if (!vdev) {
+        return -1;
+    }
+
+    /* Plug the device */
+    if (virtio_bus_plug_device(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..cb2ea8c 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,15 @@ struct VirtIOBlkConf
     uint32_t config_wce;
 };
 
+typedef struct {
+    DeviceState qdev;
+    uint32_t host_features;
+    VirtIOBlkConf blk;
+} VirtIOBLKState;
+
+#define TYPE_VIRTIO_BLK "virtio-blk"
+#define VIRTIO_BLK(obj) OBJECT_CHECK(VirtIOBLKState, (obj), TYPE_VIRTIO_BLK)
+
 #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

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

[Stefan Hajnoczi]

On Thu, Nov 22, 2012 at 03:50:52PM +0100, fred.konrad@greensocs.com wrote:
> @@ -656,3 +658,83 @@ void virtio_blk_exit(VirtIODevice *vdev)
>      blockdev_mark_auto_del(s->bs);
>      virtio_cleanup(vdev);
>  }
> +
> +static int virtio_blk_qdev_init(DeviceState *qdev)
> +{
> +    VirtIOBLKState *s = VIRTIO_BLK(qdev);

Please use "Block" or "Blk", not "BLK".

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

[Peter Maydell]

On 22 November 2012 14:50,  <fred.konrad@greensocs.com> wrote:
> There are still two issues with the command line :
>
>     * When I use ./qemu* -device virtio-blk -device virtio-pci
>       It is said that no virtio-bus are present.

Does it work if you create the devices in the other order?

>     * The virtio-blk is plugged in the last created virtio-bus if no "bus="
>       option is present. It's an issue as we can only plug one virtio-device.

Anthony said this might Just Work (ie that we should find the
first non-full bus rather than just the first one), and if it
doesn't work it should be fairly straightforward to make it work.

-- PMM

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

[KONRAD Frédéric]

Did you get the full patchset ? The 01 and 02 seems to be lost on
the mailing list. :s.

On 22/11/2012 16:08, Peter Maydell wrote:
> On 22 November 2012 14:50,  <fred.konrad@greensocs.com> wrote:
>> There are still two issues with the command line :
>>
>>      * When I use ./qemu* -device virtio-blk -device virtio-pci
>>        It is said that no virtio-bus are present.
> Does it work if you create the devices in the other order?
Yes the other order works.

>
>>      * The virtio-blk is plugged in the last created virtio-bus if no "bus="
>>        option is present. It's an issue as we can only plug one virtio-device.
> Anthony said this might Just Work (ie that we should find the
> first non-full bus rather than just the first one), and if it
> doesn't work it should be fairly straightforward to make it work.
Yes, maybe it is implemented in the QOM ? Because when I use
qdev_get_parent_bus(qdev) in virtio_blk_qdev_init, it gives me
the last created Virtiobus.

Fred.

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

[Stefan Hajnoczi]

On Thu, Nov 22, 2012 at 03:50:49PM +0100, fred.konrad@greensocs.com wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
> I made the changes you suggest in the last RFC. 
> 
> There are still two issues with the command line :
> 
>     * When I use ./qemu* -device virtio-blk -device virtio-pci
>       It is said that no virtio-bus are present.
>     * The virtio-blk is plugged in the last created virtio-bus if no "bus="
>       option is present. It's an issue as we can only plug one virtio-device.
> 
> The first problem is a more general issue as it is the case for the SCSI bus and
> can be fixed later.

Thanks for sharing virtio refactoring progress.

I think the challenge will be truly converting existing code over to the
new approach.  This RFC series adds a new layer on top of the existing
code but doesn't actually replace it.

Would be interesting to see the complete picture, even if you need to
leave some TODOs in the middle when sending RFC patches.

Stefan

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

[Konrad Frederic]

On 23/11/2012 13:38, Stefan Hajnoczi wrote:
> On Thu, Nov 22, 2012 at 03:50:49PM +0100, fred.konrad@greensocs.com wrote:
>> From: KONRAD Frederic<fred.konrad@greensocs.com>
>> I made the changes you suggest in the last RFC.
>>
>> There are still two issues with the command line :
>>
>>      * When I use ./qemu* -device virtio-blk -device virtio-pci
>>        It is said that no virtio-bus are present.
>>      * The virtio-blk is plugged in the last created virtio-bus if no "bus="
>>        option is present. It's an issue as we can only plug one virtio-device.
>>
>> The first problem is a more general issue as it is the case for the SCSI bus and
>> can be fixed later.
> Thanks for sharing virtio refactoring progress.
>
> I think the challenge will be truly converting existing code over to the
> new approach.  This RFC series adds a new layer on top of the existing
> code but doesn't actually replace it.
>
> Would be interesting to see the complete picture, even if you need to
> leave some TODOs in the middle when sending RFC patches.
>
> Stefan
Yes, sure.

So the next would be :
     * use QOM interface in place of VirtioBusInfo ?
     * refactor the VirtIODevice to remove VirtIOBinding ?

I though modifying the less I can the VirtIODevice as it could break all 
the s390 devices.

Fred

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

[Stefan Hajnoczi]

On Fri, Nov 23, 2012 at 03:29:52PM +0100, Konrad Frederic wrote:
> On 23/11/2012 13:38, Stefan Hajnoczi wrote:
> >On Thu, Nov 22, 2012 at 03:50:49PM +0100, fred.konrad@greensocs.com wrote:
> >>From: KONRAD Frederic<fred.konrad@greensocs.com>
> >>I made the changes you suggest in the last RFC.
> >>
> >>There are still two issues with the command line :
> >>
> >>     * When I use ./qemu* -device virtio-blk -device virtio-pci
> >>       It is said that no virtio-bus are present.
> >>     * The virtio-blk is plugged in the last created virtio-bus if no "bus="
> >>       option is present. It's an issue as we can only plug one virtio-device.
> >>
> >>The first problem is a more general issue as it is the case for the SCSI bus and
> >>can be fixed later.
> >Thanks for sharing virtio refactoring progress.
> >
> >I think the challenge will be truly converting existing code over to the
> >new approach.  This RFC series adds a new layer on top of the existing
> >code but doesn't actually replace it.
> >
> >Would be interesting to see the complete picture, even if you need to
> >leave some TODOs in the middle when sending RFC patches.
> >
> >Stefan
> Yes, sure.
> 
> So the next would be :
>     * use QOM interface in place of VirtioBusInfo ?

This is probably a detail.  It probably doesn't make a big difference in
the RFC series.

>     * refactor the VirtIODevice to remove VirtIOBinding ?

Yes, it would be nice to make your design the core and push the
compatibility stuff ("virtio-blk-pci", etc) to a layer on top, instead
of leaving the existing code as the core and putting QOM on top.

> I though modifying the less I can the VirtIODevice as it could break
> all the s390 devices.

Sure and I think you've done a good job at showing incremental patches
vs a big bang approach that replaces everything in one patch.

Stefan

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

[Konrad Frederic]

On 23/11/2012 17:18, Stefan Hajnoczi wrote:
> On Fri, Nov 23, 2012 at 03:29:52PM +0100, Konrad Frederic wrote:
>> On 23/11/2012 13:38, Stefan Hajnoczi wrote:
>>> On Thu, Nov 22, 2012 at 03:50:49PM +0100, fred.konrad@greensocs.com wrote:
>>>> From: KONRAD Frederic<fred.konrad@greensocs.com>
>>>> I made the changes you suggest in the last RFC.
>>>>
>>>> There are still two issues with the command line :
>>>>
>>>>      * When I use ./qemu* -device virtio-blk -device virtio-pci
>>>>        It is said that no virtio-bus are present.
>>>>      * The virtio-blk is plugged in the last created virtio-bus if no "bus="
>>>>        option is present. It's an issue as we can only plug one virtio-device.
>>>>
>>>> The first problem is a more general issue as it is the case for the SCSI bus and
>>>> can be fixed later.
>>> Thanks for sharing virtio refactoring progress.
>>>
>>> I think the challenge will be truly converting existing code over to the
>>> new approach.  This RFC series adds a new layer on top of the existing
>>> code but doesn't actually replace it.
>>>
>>> Would be interesting to see the complete picture, even if you need to
>>> leave some TODOs in the middle when sending RFC patches.
>>>
>>> Stefan
>> Yes, sure.
>>
>> So the next would be :
>>      * use QOM interface in place of VirtioBusInfo ?
> This is probably a detail.  It probably doesn't make a big difference in
> the RFC series.
>
>>      * refactor the VirtIODevice to remove VirtIOBinding ?
> Yes, it would be nice to make your design the core and push the
> compatibility stuff ("virtio-blk-pci", etc) to a layer on top, instead
> of leaving the existing code as the core and putting QOM on top.
Ok, I'll do that.
>> I though modifying the less I can the VirtIODevice as it could break
>> all the s390 devices.
> Sure and I think you've done a good job at showing incremental patches
> vs a big bang approach that replaces everything in one patch.
>
> Stefan
Thanks,

Fred