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

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

[fred.konrad]

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

You can clone that from here :
git.greensocs.com/home/greensocs/git/qemu_virtio.git virtio_refactoring_v7

These are the two last steps of refactoring ( only for virtio-blk device. ):

    * It modifies virtio-blk-pci to extend virtio-pci and to connect a
      virtio-blk during the initialisation.
    * It modifies virtio-blk ( QOM cast ).

I think the last step is breaking virtio-blk-s390, can somebody confirm that?

If it is breaking virtio-blk-s390 how can I modify it ?

I think it is the right thing to do, and I want to know what do you think before
turning all the remaining virtio-device.

Any feedback ?

This patch-set is :
    * Introducing a virtio-bus which extends bus-state.
    * Implementing a virtio-pci-bus which extends virtio-bus.
    * Implementing a virtio-pci device which has a virtio-pci-bus.
    * Implementing virtio-device which extends device-states.
    * Implementing a virtio-blk which extends virtio-device.
    * Modifying virtio-blk-pci to extends virtio-pci and connect a virtio-blk.

The first patch is a modification of qdev-monitor.c, it forces the function
qbus_find_recursive(..) to return a non-full bus, and return an error if the
desired bus ( with "bus=" option ) is full. It add a max_dev field to the
bus_state structure. If max_dev=0 it has no limitation, if not the maximum
amount of device connected to the bus is max_dev.

Changes v6 -> v7:
    * virtio-bus : Added virtio-bus-reset.
    * virtio-pci : Fixed virtio-pci-exit.
    * virtio-pci : Added virtio-pci-rst.
    * virtio-pci : Added VirtioPCIClass filled with an init function.
    * virtio-blk : Added virtio_blk_set_conf.
    * virtio-blk : QOM casts.
    * virtio-blk-pci : Switched to the new API.

Changes v5 -> v6:
    * Renamed virtio_common_init_ to virtio_init, modify virtio_common_init to
      allocate and call virtio_init. Drop the unused structure size parameters.
    * Renamed init/exit callback in VirtioBusClass.
    * Renamed virtio_blk_init virtio_blk_common_init.
    * Modified virtio_blk_init to call virtio_blk_common_init.

Changes v4 -> v5:
    * use ERROR_CLASS_GENERIC_ERROR in place of creating a new error type for
      the maximum device limitation. ( Peter )
    * Removed bus_in_use function. We assume that the virtio-bus is not in use,
      when plugin in. ( Peter )
    * Added virtio_bus_destroy_device().
    * Implemented the exit function of virtio-pci.
    * Implemented the init callback for virtio-pci ( must be modified, it still
      access vdev directly. ).
    * Implemented the exit callback for virtio-pci.
    * Started virtio-device refactoring.
    * Started virtio-blk refactoring. 

Changes v3 -> v4:
    * Added virtio-bus.o in Makefile.objs ( accidentally dropped from v3 ).
    * *const* TypeInfo in virtio-bus.
    * Introduced virtio-pci-bus.
    * Reintroduced virtio-pci.
    * Introduced virtio-device.
    * Started virtio-blk refactoring.
    * Added an error type in qerror.h for the "bus full" error.

Changes v2 -> v3:
    * Added VirtioBusClass.
    * Renamed VirtioBus -> VirtioBusState.
    * Renamed qbus -> parent_obj.
    * Plug the device only in a non-full bus.

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.

Fred.

KONRAD Frederic (8):
  qdev : add a maximum device allowed field for the bus.
  virtio-bus : Introduce virtio-bus
  virtio-pci-bus : Introduce virtio-pci-bus.
  virtio-pci : Refactor virtio-pci device.
  virtio-device : Refactor virtio-device.
  virtio-blk : Add the virtio-blk device.
  virtio-pci-blk : Switch to new API.
  virtio-blk : QOM modifications.

 hw/Makefile.objs  |   1 +
 hw/qdev-core.h    |   2 +
 hw/qdev-monitor.c |  11 +++
 hw/virtio-blk.c   | 140 ++++++++++++++++++++-------
 hw/virtio-blk.h   |   8 ++
 hw/virtio-bus.c   | 120 +++++++++++++++++++++++
 hw/virtio-bus.h   |  83 ++++++++++++++++
 hw/virtio-pci.c   | 277 +++++++++++++++++++++++++++++++++++++++++-------------
 hw/virtio-pci.h   |  52 +++++++++-
 hw/virtio.c       |  50 +++++++---
 hw/virtio.h       |  28 ++++++
 11 files changed, 657 insertions(+), 115 deletions(-)
 create mode 100644 hw/virtio-bus.c
 create mode 100644 hw/virtio-bus.h

-- 
1.7.11.7

[Qemu-devel] [RFC PATCH v7 1/8] qdev : add a maximum device allowed field for the bus.

[fred.konrad]

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

Add a max_dev field to BusState to specify the maximum amount of devices allowed
on the bus ( have no effect if max_dev=0 )

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 hw/qdev-core.h    |  2 ++
 hw/qdev-monitor.c | 11 +++++++++++
 2 files changed, 13 insertions(+)

diff --git a/hw/qdev-core.h b/hw/qdev-core.h
index fff7f0f..ee4becd 100644
--- a/hw/qdev-core.h
+++ b/hw/qdev-core.h
@@ -113,6 +113,8 @@ struct BusState {
     const char *name;
     int allow_hotplug;
     int max_index;
+    /* maximum devices allowed on the bus, 0 : no limit. */
+    int max_dev;
     QTAILQ_HEAD(ChildrenHead, BusChild) children;
     QLIST_ENTRY(BusState) sibling;
 };
diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index a1b4d6a..7a9d275 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -292,6 +292,17 @@ static BusState *qbus_find_recursive(BusState *bus, const char *name,
     if (bus_typename && !object_dynamic_cast(OBJECT(bus), bus_typename)) {
         match = 0;
     }
+    if ((bus->max_dev != 0) && (bus->max_dev <= bus->max_index)) {
+        if (name != NULL) {
+            /* bus was explicitly specified : return an error. */
+            qerror_report(ERROR_CLASS_GENERIC_ERROR, "Bus '%s' is full",
+                          bus->name);
+            return NULL;
+        } else {
+            /* bus was not specified : try to find another one. */
+            match = 0;
+        }
+    }
     if (match) {
         return bus;
     }
-- 
1.7.11.7

[Qemu-devel] [RFC PATCH v7 2/8] virtio-bus : Introduce virtio-bus

[fred.konrad]

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

Introduce virtio-bus. Refactored transport device will create a bus which
extends virtio-bus.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 hw/Makefile.objs |   1 +
 hw/virtio-bus.c  | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-bus.h  |  83 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 204 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 d581d8d..6fa4de4 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -3,6 +3,7 @@ common-obj-y += loader.o
 common-obj-$(CONFIG_VIRTIO) += virtio-console.o
 common-obj-$(CONFIG_VIRTIO) += virtio-rng.o
 common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
+common-obj-$(CONFIG_VIRTIO) += virtio-bus.o
 common-obj-y += fw_cfg.o
 common-obj-$(CONFIG_PCI) += pci.o pci_bridge.o pci_bridge_dev.o
 common-obj-$(CONFIG_PCI) += msix.o msi.o
diff --git a/hw/virtio-bus.c b/hw/virtio-bus.c
new file mode 100644
index 0000000..6afd2b6
--- /dev/null
+++ b/hw/virtio-bus.c
@@ -0,0 +1,120 @@
+/*
+ * 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 */
+
+#ifdef DEBUG_VIRTIO_BUS
+#define DPRINTF(fmt, ...) \
+do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
+#else
+#define DPRINTF(fmt, ...) do { } while (0)
+#endif
+
+/* Plug the VirtIODevice */
+int virtio_bus_plug_device(VirtIODevice *vdev)
+{
+    DeviceState *qdev = DEVICE(vdev);
+    BusState *qbus = BUS(qdev_get_parent_bus(qdev));
+    VirtioBusState *bus = VIRTIO_BUS(qbus);
+    VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus);
+    DPRINTF("%s : plug device.\n", qbus->name);
+
+    bus->vdev = vdev;
+
+    if (klass->device_plugged != NULL) {
+        klass->device_plugged(qbus->parent);
+    }
+
+    /*
+     * The lines below will disappear when we drop VirtIOBindings.
+     */
+    bus->bindings.notify = klass->notify;
+    bus->bindings.save_config = klass->save_config;
+    bus->bindings.save_queue = klass->save_queue;
+    bus->bindings.load_config = klass->load_config;
+    bus->bindings.load_queue = klass->load_queue;
+    bus->bindings.load_done = klass->load_done;
+    bus->bindings.get_features = klass->get_features;
+    bus->bindings.query_guest_notifiers = klass->query_guest_notifiers;
+    bus->bindings.set_guest_notifiers = klass->set_guest_notifiers;
+    bus->bindings.set_host_notifier = klass->set_host_notifier;
+    bus->bindings.vmstate_change = klass->vmstate_change;
+    virtio_bind_device(bus->vdev, &(bus->bindings), qbus->parent);
+
+    return 0;
+}
+
+/* Reset the virtio_bus */
+void virtio_bus_reset(VirtioBusState *bus)
+{
+    DPRINTF("%s : reset device.\n", qbus->name);
+    if (bus->vdev != NULL) {
+        virtio_reset(bus->vdev);
+    }
+}
+
+/* Destroy the VirtIODevice */
+void virtio_bus_destroy_device(VirtioBusState *bus)
+{
+    DeviceState *qdev;
+    BusState *qbus = BUS(bus);
+    VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus);
+    DPRINTF("%s : remove device.\n", qbus->name);
+
+    if (bus->vdev != NULL) {
+        if (klass->device_unplug != NULL) {
+            klass->device_unplug(qbus->parent);
+        }
+        qdev = DEVICE(bus->vdev);
+        qdev_free(qdev);
+        bus->vdev = NULL;
+    }
+}
+
+/* Return the virtio device id of the plugged device. */
+uint16_t get_virtio_device_id(VirtioBusState *bus)
+{
+    return bus->vdev->device_id;
+}
+
+static const TypeInfo virtio_bus_info = {
+    .name = TYPE_VIRTIO_BUS,
+    .parent = TYPE_BUS,
+    .instance_size = sizeof(VirtioBusState),
+    .abstract = true,
+    .class_size = sizeof(VirtioBusClass),
+};
+
+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..8bb303a
--- /dev/null
+++ b/hw/virtio-bus.h
@@ -0,0 +1,83 @@
+/*
+ * 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_GET_CLASS(obj) \
+        OBJECT_GET_CLASS(VirtioBusClass, obj, TYPE_VIRTIO_BUS)
+#define VIRTIO_BUS_CLASS(klass) \
+        OBJECT_CLASS_CHECK(VirtioBusClass, klass, TYPE_VIRTIO_BUS)
+#define VIRTIO_BUS(obj) OBJECT_CHECK(VirtioBusState, (obj), TYPE_VIRTIO_BUS)
+
+typedef struct VirtioBusState VirtioBusState;
+
+typedef struct VirtioBusClass {
+    /* This is what a VirtioBus must implement */
+    BusClass parent;
+    void (*notify)(void *opaque, uint16_t vector);
+    void (*save_config)(void *opaque, QEMUFile *f);
+    void (*save_queue)(void *opaque, int n, QEMUFile *f);
+    int (*load_config)(void *opaque, QEMUFile *f);
+    int (*load_queue)(void *opaque, int n, QEMUFile *f);
+    int (*load_done)(void *opaque, QEMUFile *f);
+    unsigned (*get_features)(void *opaque);
+    bool (*query_guest_notifiers)(void *opaque);
+    int (*set_guest_notifiers)(void *opaque, bool assigned);
+    int (*set_host_notifier)(void *opaque, int n, bool assigned);
+    void (*vmstate_change)(void *opaque, bool running);
+    /*
+     * transport independent init function.
+     * This is called by virtio-bus just after the device is plugged.
+     */
+    void (*device_plugged)(void *opaque);
+    /*
+     * transport independent exit function.
+     * This is called by virtio-bus just before the device is unplugged.
+     */
+    void (*device_unplug)(void *opaque);
+} VirtioBusClass;
+
+struct VirtioBusState {
+    BusState parent_obj;
+    /*
+     * Only one VirtIODevice can be plugged on the bus.
+     */
+    VirtIODevice *vdev;
+    /*
+     * This should be removed when we refactor virtio-device.
+     */
+    VirtIOBindings bindings;
+};
+
+int virtio_bus_plug_device(VirtIODevice *vdev);
+void virtio_bus_reset(VirtioBusState *bus);
+void virtio_bus_destroy_device(VirtioBusState *bus);
+uint16_t get_virtio_device_id(VirtioBusState *bus);
+#endif /* VIRTIO_BUS_H */
-- 
1.7.11.7

[Qemu-devel] [RFC PATCH v7 3/8] virtio-pci-bus : Introduce virtio-pci-bus.

[fred.konrad]

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

Introduce virtio-pci-bus, which extends virtio-bus. It is used with virtio-pci
transport device.

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

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 71f4fb5..5ac8d0d 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -32,6 +32,7 @@
 #include "blockdev.h"
 #include "virtio-pci.h"
 #include "range.h"
+#include "virtio-bus.h"
 
 /* from Linux's linux/virtio_pci.h */
 
@@ -1118,6 +1119,41 @@ static TypeInfo virtio_scsi_info = {
     .class_init    = virtio_scsi_class_init,
 };
 
+/* virtio-pci-bus */
+
+VirtioBusState *virtio_pci_bus_new(VirtIOPCIProxy *dev)
+{
+    DeviceState *qdev = DEVICE(dev);
+    BusState *qbus = qbus_create(TYPE_VIRTIO_PCI_BUS, qdev, NULL);
+    VirtioBusState *bus = VIRTIO_BUS(qbus);
+    qbus->allow_hotplug = 0;
+    /* Only one virtio-device allowed for virtio-pci. */
+    qbus->max_dev = 1;
+    return bus;
+}
+
+static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
+{
+    VirtioBusClass *k = VIRTIO_BUS_CLASS(klass);
+    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;
+}
+
+static const TypeInfo virtio_pci_bus_info = {
+    .name          = TYPE_VIRTIO_PCI_BUS,
+    .parent        = TYPE_VIRTIO_BUS,
+    .instance_size = sizeof(VirtioBusState),
+    .class_init    = virtio_pci_bus_class_init,
+};
+
 static void virtio_pci_register_types(void)
 {
     type_register_static(&virtio_blk_info);
@@ -1126,6 +1162,7 @@ static void virtio_pci_register_types(void)
     type_register_static(&virtio_balloon_info);
     type_register_static(&virtio_scsi_info);
     type_register_static(&virtio_rng_info);
+    type_register_static(&virtio_pci_bus_info);
 }
 
 type_init(virtio_pci_register_types)
diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h
index b58d9a2..0e3288e 100644
--- a/hw/virtio-pci.h
+++ b/hw/virtio-pci.h
@@ -20,6 +20,21 @@
 #include "virtio-rng.h"
 #include "virtio-serial.h"
 #include "virtio-scsi.h"
+#include "virtio-bus.h"
+
+/* VirtIOPCIProxy will be renammed VirtioPCIState at the end. */
+typedef struct VirtIOPCIProxy VirtIOPCIProxy;
+
+/* virtio-pci-bus */
+#define TYPE_VIRTIO_PCI_BUS "virtio-pci-bus"
+#define VIRTIO_PCI_BUS_GET_CLASS(obj) \
+        OBJECT_GET_CLASS(VirtioBusClass, obj, TYPE_VIRTIO_PCI_BUS)
+#define VIRTIO_PCI_BUS_CLASS(klass) \
+        OBJECT_CLASS_CHECK(VirtioBusClass, klass, TYPE_VIRTIO_PCI_BUS)
+#define VIRTIO_PCI_BUS(obj) \
+        OBJECT_CHECK(VirtioBusState, (obj), TYPE_VIRTIO_PCI_BUS)
+
+VirtioBusState *virtio_pci_bus_new(VirtIOPCIProxy *dev);
 
 /* Performance improves when virtqueue kick processing is decoupled from the
  * vcpu thread using ioeventfd for some devices. */
@@ -31,7 +46,7 @@ typedef struct {
     unsigned int users;
 } VirtIOIRQFD;
 
-typedef struct {
+struct VirtIOPCIProxy {
     PCIDevice pci_dev;
     VirtIODevice *vdev;
     MemoryRegion bar;
@@ -51,7 +66,7 @@ typedef struct {
     bool ioeventfd_disabled;
     bool ioeventfd_started;
     VirtIOIRQFD *vector_irqfd;
-} VirtIOPCIProxy;
+};
 
 void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev);
 void virtio_pci_reset(DeviceState *d);
-- 
1.7.11.7

[Qemu-devel] [RFC PATCH v7 4/8] virtio-pci : Refactor virtio-pci device.

[fred.konrad]

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

Create the virtio-pci device. This transport device will create a
virtio-pci-bus, so one VirtIODevice can be connected.

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

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 5ac8d0d..8de26fd 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -1119,6 +1119,130 @@ static TypeInfo virtio_scsi_info = {
     .class_init    = virtio_scsi_class_init,
 };
 
+/*
+ * virtio-pci : This is the PCIDevice which have a virtio-pci-bus.
+ */
+
+/* This is called by virtio-bus just after the device is plugged. */
+static void virtio_pci_device_plugged(void *opaque)
+{
+    VirtIOPCIProxy *proxy = VIRTIO_PCI(opaque);
+    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;
+
+    }
+
+    /* TODO: vdev should be accessed through virtio-bus functions. */
+    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, get_virtio_device_id(proxy->bus));
+    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;
+    }
+
+    proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
+    proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
+    proxy->host_features = proxy->bus->vdev->get_features(proxy->bus->vdev,
+                                                          proxy->host_features);
+}
+
+/* This is called by virtio-bus just before the device is unplugged. */
+static void virtio_pci_device_unplug(void *opaque)
+{
+    VirtIOPCIProxy *dev = VIRTIO_PCI(opaque);
+    virtio_pci_stop_ioeventfd(dev);
+}
+
+static int virtio_pci_init(PCIDevice *pci_dev)
+{
+    VirtIOPCIProxy *dev = VIRTIO_PCI(pci_dev);
+    VirtioPCIClass *k = VIRTIO_PCI_GET_CLASS(pci_dev);
+    dev->bus = virtio_pci_bus_new(dev);
+    if (k->init != NULL) {
+        return k->init(dev);
+    }
+    return 0;
+}
+
+static void virtio_pci_exit(PCIDevice *pci_dev)
+{
+    VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
+    VirtioBusState *bus = VIRTIO_BUS(proxy->bus);
+    BusState *qbus = BUS(proxy->bus);
+    virtio_bus_destroy_device(bus);
+    qbus_free(qbus);
+}
+
+static void virtio_pci_rst(DeviceState *qdev)
+{
+    VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
+    VirtioBusState *bus = VIRTIO_BUS(proxy->bus);
+    virtio_pci_stop_ioeventfd(proxy);
+    virtio_bus_reset(bus);
+    msix_unuse_all_vectors(&proxy->pci_dev);
+    proxy->flags &= ~VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
+}
+
+static void virtio_pci_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->init = virtio_pci_init;
+    k->exit = virtio_pci_exit;
+    k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
+    k->revision = VIRTIO_PCI_ABI_VERSION;
+    k->class_id = PCI_CLASS_OTHERS;
+    dc->reset = virtio_pci_rst;
+}
+
+static const TypeInfo virtio_pci_info = {
+    .name          = TYPE_VIRTIO_PCI,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(VirtIOPCIProxy),
+    .class_init    = virtio_pci_class_init,
+    .class_size    = sizeof(VirtioPCIClass),
+};
+
 /* virtio-pci-bus */
 
 VirtioBusState *virtio_pci_bus_new(VirtIOPCIProxy *dev)
@@ -1145,6 +1269,8 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
     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;
+    k->device_plugged = virtio_pci_device_plugged;
+    k->device_unplug = virtio_pci_device_unplug;
 }
 
 static const TypeInfo virtio_pci_bus_info = {
@@ -1163,6 +1289,7 @@ static void virtio_pci_register_types(void)
     type_register_static(&virtio_scsi_info);
     type_register_static(&virtio_rng_info);
     type_register_static(&virtio_pci_bus_info);
+    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 0e3288e..e840cea 100644
--- a/hw/virtio-pci.h
+++ b/hw/virtio-pci.h
@@ -46,6 +46,22 @@ typedef struct {
     unsigned int users;
 } VirtIOIRQFD;
 
+/*
+ * virtio-pci : This is the PCIDevice which have a virtio-pci-bus.
+ */
+#define TYPE_VIRTIO_PCI "virtio-pci"
+#define VIRTIO_PCI_GET_CLASS(obj) \
+        OBJECT_GET_CLASS(VirtioPCIClass, obj, TYPE_VIRTIO_PCI)
+#define VIRTIO_PCI_CLASS(klass) \
+        OBJECT_CLASS_CHECK(VirtioPCIClass, klass, TYPE_VIRTIO_PCI)
+#define VIRTIO_PCI(obj) \
+        OBJECT_CHECK(VirtIOPCIProxy, (obj), TYPE_VIRTIO_PCI)
+
+typedef struct VirtioPCIClass {
+    PCIDeviceClass parent_class;
+    int (*init)(VirtIOPCIProxy *vpci_dev);
+} VirtioPCIClass;
+
 struct VirtIOPCIProxy {
     PCIDevice pci_dev;
     VirtIODevice *vdev;
@@ -66,6 +82,9 @@ struct VirtIOPCIProxy {
     bool ioeventfd_disabled;
     bool ioeventfd_started;
     VirtIOIRQFD *vector_irqfd;
+    /* That's the virtio-bus on which VirtioDevice will be connected. */
+    VirtioBusState *bus;
+    /* Nothing more for the moment. */
 };
 
 void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev);
-- 
1.7.11.7

[Qemu-devel] [RFC PATCH v7 5/8] virtio-device : Refactor virtio-device.

[fred.konrad]

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

Create the virtio-device which is abstract. All the virtio-device can extend
this class.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 hw/virtio.c | 50 +++++++++++++++++++++++++++++++++++++++-----------
 hw/virtio.h | 28 ++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+), 11 deletions(-)

diff --git a/hw/virtio.c b/hw/virtio.c
index f40a8c5..2541ea8 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -16,6 +16,7 @@
 #include "trace.h"
 #include "qemu-error.h"
 #include "virtio.h"
+#include "virtio-bus.h"
 #include "qemu-barrier.h"
 
 /* The alignment to use between consumer and producer parts of vring.
@@ -902,14 +903,10 @@ static void virtio_vmstate_change(void *opaque, int running, RunState state)
     }
 }
 
-VirtIODevice *virtio_common_init(const char *name, uint16_t device_id,
-                                 size_t config_size, size_t struct_size)
+void virtio_init(VirtIODevice *vdev, const char *name,
+                 uint16_t device_id, size_t config_size)
 {
-    VirtIODevice *vdev;
     int i;
-
-    vdev = g_malloc0(struct_size);
-
     vdev->device_id = device_id;
     vdev->status = 0;
     vdev->isr = 0;
@@ -917,20 +914,28 @@ VirtIODevice *virtio_common_init(const char *name, uint16_t device_id,
     vdev->config_vector = VIRTIO_NO_VECTOR;
     vdev->vq = g_malloc0(sizeof(VirtQueue) * VIRTIO_PCI_QUEUE_MAX);
     vdev->vm_running = runstate_is_running();
-    for(i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
+    for (i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
         vdev->vq[i].vector = VIRTIO_NO_VECTOR;
         vdev->vq[i].vdev = vdev;
     }
 
     vdev->name = name;
     vdev->config_len = config_size;
-    if (vdev->config_len)
+    if (vdev->config_len) {
         vdev->config = g_malloc0(config_size);
-    else
+    } else {
         vdev->config = NULL;
+    }
+    vdev->vmstate = qemu_add_vm_change_state_handler(virtio_vmstate_change,
+                                                     vdev);
+}
 
-    vdev->vmstate = qemu_add_vm_change_state_handler(virtio_vmstate_change, vdev);
-
+VirtIODevice *virtio_common_init(const char *name, uint16_t device_id,
+                                 size_t config_size, size_t struct_size)
+{
+    VirtIODevice *vdev;
+    vdev = g_malloc0(struct_size);
+    virtio_init(vdev, name, device_id, config_size);
     return vdev;
 }
 
@@ -1056,3 +1061,26 @@ EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq)
 {
     return &vq->host_notifier;
 }
+
+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;
+}
+
+static const TypeInfo virtio_device_info = {
+    .name = TYPE_VIRTIO_DEVICE,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(VirtIODevice),
+    .class_init = virtio_device_class_init,
+    .abstract = true,
+    .class_size = sizeof(VirtioDeviceClass),
+};
+
+static void virtio_register_types(void)
+{
+    type_register_static(&virtio_device_info);
+}
+
+type_init(virtio_register_types)
diff --git a/hw/virtio.h b/hw/virtio.h
index 7c17f7b..79d2b91 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -108,8 +108,17 @@ typedef struct {
 
 #define VIRTIO_NO_VECTOR 0xffff
 
+#define TYPE_VIRTIO_DEVICE "virtio-device"
+#define VIRTIO_DEVICE_GET_CLASS(obj) \
+        OBJECT_GET_CLASS(VirtioDeviceClass, obj, TYPE_VIRTIO_DEVICE)
+#define VIRTIO_DEVICE_CLASS(klass) \
+        OBJECT_CLASS_CHECK(VirtioDeviceClass, klass, TYPE_VIRTIO_DEVICE)
+#define VIRTIO_DEVICE(obj) \
+        OBJECT_CHECK(VirtIODevice, (obj), TYPE_VIRTIO_DEVICE)
+
 struct VirtIODevice
 {
+    DeviceState parent_obj;
     const char *name;
     uint8_t status;
     uint8_t isr;
@@ -119,6 +128,9 @@ struct VirtIODevice
     void *config;
     uint16_t config_vector;
     int nvectors;
+    /*
+     * Must be removed as we have it in VirtioDeviceClass.
+     */
     uint32_t (*get_features)(VirtIODevice *vdev, uint32_t requested_features);
     uint32_t (*bad_features)(VirtIODevice *vdev);
     void (*set_features)(VirtIODevice *vdev, uint32_t val);
@@ -126,6 +138,7 @@ struct VirtIODevice
     void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
     void (*reset)(VirtIODevice *vdev);
     void (*set_status)(VirtIODevice *vdev, uint8_t val);
+    /***/
     VirtQueue *vq;
     const VirtIOBindings *binding;
     void *binding_opaque;
@@ -134,6 +147,21 @@ struct VirtIODevice
     VMChangeStateEntry *vmstate;
 };
 
+typedef struct {
+    /* This is what a VirtioDevice must implement */
+    DeviceClass parent;
+    uint32_t (*get_features)(VirtIODevice *vdev, uint32_t requested_features);
+    uint32_t (*bad_features)(VirtIODevice *vdev);
+    void (*set_features)(VirtIODevice *vdev, uint32_t val);
+    void (*get_config)(VirtIODevice *vdev, uint8_t *config);
+    void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
+    void (*reset)(VirtIODevice *vdev);
+    void (*set_status)(VirtIODevice *vdev, uint8_t val);
+} VirtioDeviceClass;
+
+void virtio_init(VirtIODevice *vdev, const char *name,
+                         uint16_t device_id, size_t config_size);
+
 VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
                             void (*handle_output)(VirtIODevice *,
                                                   VirtQueue *));
-- 
1.7.11.7

[Qemu-devel] [RFC PATCH v7 6/8] virtio-blk : Add the virtio-blk device.

[fred.konrad]

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

Create virtio-blk which extends virtio-device, so it can be connected on
virtio-bus.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 hw/virtio-blk.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 hw/virtio-blk.h |   6 ++++
 2 files changed, 104 insertions(+), 9 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index e25cc96..58c9086 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -21,6 +21,7 @@
 #ifdef __linux__
 # include <scsi/sg.h>
 #endif
+#include "virtio-bus.h"
 
 typedef struct VirtIOBlock
 {
@@ -30,11 +31,14 @@ typedef struct VirtIOBlock
     void *rq;
     QEMUBH *bh;
     BlockConf *conf;
-    VirtIOBlkConf *blk;
+    VirtIOBlkConf blk;
     unsigned short sector_mask;
     DeviceState *qdev;
 } VirtIOBlock;
 
+/*
+ * Moving to QOM later in this series.
+ */
 static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
 {
     return (VirtIOBlock *)vdev;
@@ -164,7 +168,7 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
      */
     req->scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base;
 
-    if (!req->dev->blk->scsi) {
+    if (!req->dev->blk.scsi) {
         status = VIRTIO_BLK_S_UNSUPP;
         goto fail;
     }
@@ -384,7 +388,7 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
          * terminated by '\0' only when shorter than buffer.
          */
         strncpy(req->elem.in_sg[0].iov_base,
-                s->blk->serial ? s->blk->serial : "",
+                s->blk.serial ? s->blk.serial : "",
                 MIN(req->elem.in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES));
         virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
         g_free(req);
@@ -600,9 +604,16 @@ static const BlockDevOps virtio_block_ops = {
     .resize_cb = virtio_blk_resize,
 };
 
-VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk)
+void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk)
+{
+    VirtIOBlock *s = VIRTIO_BLK(dev);
+    memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf));
+}
+
+static VirtIODevice *virtio_blk_common_init(DeviceState *dev,
+                                          VirtIOBlkConf *blk, VirtIOBlock **ps)
 {
-    VirtIOBlock *s;
+    VirtIOBlock *s = *ps;
     static int virtio_blk_id;
 
     if (!blk->conf.bs) {
@@ -619,9 +630,20 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk)
         return NULL;
     }
 
-    s = (VirtIOBlock *)virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
-                                          sizeof(struct virtio_blk_config),
-                                          sizeof(VirtIOBlock));
+    /*
+     * We have two cases here : the old virtio-blk-pci device, and the
+     * refactored virtio-blk.
+     */
+    if (s == NULL) {
+        /* virtio-blk-pci */
+        s = (VirtIOBlock *)virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
+                                              sizeof(struct virtio_blk_config),
+                                              sizeof(VirtIOBlock));
+    } else {
+        /* virtio-blk */
+        virtio_init(VIRTIO_DEVICE(s), "virtio-blk", VIRTIO_ID_BLOCK,
+                    sizeof(struct virtio_blk_config));
+    }
 
     s->vdev.get_config = virtio_blk_update_config;
     s->vdev.set_config = virtio_blk_set_config;
@@ -630,7 +652,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk)
     s->vdev.reset = virtio_blk_reset;
     s->bs = blk->conf.bs;
     s->conf = &blk->conf;
-    s->blk = blk;
+    virtio_blk_set_conf(dev, blk);
     s->rq = NULL;
     s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1;
 
@@ -649,6 +671,12 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk)
     return &s->vdev;
 }
 
+VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk)
+{
+    VirtIOBlock *s = NULL;
+    return virtio_blk_common_init(dev, blk, &s);
+}
+
 void virtio_blk_exit(VirtIODevice *vdev)
 {
     VirtIOBlock *s = to_virtio_blk(vdev);
@@ -656,3 +684,64 @@ void virtio_blk_exit(VirtIODevice *vdev)
     blockdev_mark_auto_del(s->bs);
     virtio_cleanup(vdev);
 }
+
+
+static int virtio_device_init(DeviceState *qdev)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
+    VirtIOBlock *s = VIRTIO_BLK(qdev);
+
+    VirtIOBlkConf *blk = &(s->blk);
+
+    virtio_blk_common_init(qdev, blk, &s);
+
+    virtio_bus_plug_device(vdev);
+
+    return 0;
+}
+
+static int virtio_device_exit(DeviceState *dev)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    virtio_blk_exit(vdev);
+    return 0;
+}
+
+static Property virtio_blk_properties[] = {
+    DEFINE_BLOCK_PROPERTIES(VirtIOBlock, blk.conf),
+    DEFINE_BLOCK_CHS_PROPERTIES(VirtIOBlock, blk.conf),
+    DEFINE_PROP_STRING("serial", VirtIOBlock, blk.serial),
+#ifdef __linux__
+    DEFINE_PROP_BIT("scsi", VirtIOBlock, blk.scsi, 0, true),
+#endif
+    DEFINE_PROP_BIT("config-wce", VirtIOBlock, blk.config_wce, 0, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void virtio_blk_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
+    dc->init = virtio_device_init;
+    dc->exit = virtio_device_exit;
+    dc->props = virtio_blk_properties;
+    vdc->get_config = virtio_blk_update_config;
+    vdc->set_config = virtio_blk_set_config;
+    vdc->get_features = virtio_blk_get_features;
+    vdc->set_status = virtio_blk_set_status;
+    vdc->reset = virtio_blk_reset;
+}
+
+static const TypeInfo virtio_device_info = {
+    .name = TYPE_VIRTIO_BLK,
+    .parent = TYPE_VIRTIO_DEVICE,
+    .instance_size = sizeof(VirtIOBlock),
+    .class_init = virtio_blk_class_init,
+};
+
+static void virtio_register_types(void)
+{
+    type_register_static(&virtio_device_info);
+}
+
+type_init(virtio_register_types)
diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
index f0740d0..4bdaa70 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -17,6 +17,10 @@
 #include "virtio.h"
 #include "hw/block-common.h"
 
+#define TYPE_VIRTIO_BLK "virtio-blk"
+#define VIRTIO_BLK(obj) \
+        OBJECT_CHECK(VirtIOBlock, (obj), TYPE_VIRTIO_BLK)
+
 /* from Linux's linux/virtio_blk.h */
 
 /* The ID for virtio_block */
@@ -111,4 +115,6 @@ struct VirtIOBlkConf
         DEFINE_VIRTIO_COMMON_FEATURES(_state, _field), \
         DEFINE_PROP_BIT("config-wce", _state, _field, VIRTIO_BLK_F_CONFIG_WCE, true)
 
+void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
+
 #endif
-- 
1.7.11.7

Re: [Qemu-devel] [RFC PATCH v7 6/8] virtio-blk : Add the virtio-blk device.

[Peter Maydell]

On 10 December 2012 16:45,  <fred.konrad@greensocs.com> wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> Create virtio-blk which extends virtio-device, so it can be connected on
> virtio-bus.

> +static int virtio_device_init(DeviceState *qdev)
> +{

This is a not a very well named function. It's blk specific
but the function name implies it is generic virtio.

> +    VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
> +    VirtIOBlock *s = VIRTIO_BLK(qdev);
> +
> +    VirtIOBlkConf *blk = &(s->blk);
> +
> +    virtio_blk_common_init(qdev, blk, &s);
> +
> +    virtio_bus_plug_device(vdev);

This doesn't look right. A subclass of VirtIODevice
shouldn't have to do anything specific to plug itself in.
You can make the VirtIODevice's DeviceClass::init function
do that. (compare the way PCIDevice's init function
handles registering the device with the bus and hotplugging
it if the bus does hotplug.)

-- PMM

[Qemu-devel] [RFC PATCH v7 7/8] virtio-pci-blk : Switch to new API.

[fred.konrad]

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

Here the virtio-blk-pci is modified for the new API. The device virtio-pci-blk
extends virtio-pci. It creates and connects a virtio-blk during the init.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 hw/virtio-pci.c | 113 +++++++++++++++++++++++---------------------------------
 hw/virtio-pci.h |  14 ++++++-
 2 files changed, 59 insertions(+), 68 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 8de26fd..776a5b4 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -734,26 +734,6 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev)
     proxy->host_features = vdev->get_features(vdev, proxy->host_features);
 }
 
-static int virtio_blk_init_pci(PCIDevice *pci_dev)
-{
-    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
-    VirtIODevice *vdev;
-
-    if (proxy->class_code != PCI_CLASS_STORAGE_SCSI &&
-        proxy->class_code != PCI_CLASS_STORAGE_OTHER)
-        proxy->class_code = PCI_CLASS_STORAGE_SCSI;
-
-    vdev = virtio_blk_init(&pci_dev->qdev, &proxy->blk);
-    if (!vdev) {
-        return -1;
-    }
-    vdev->nvectors = proxy->nvectors;
-    virtio_init_pci(proxy, vdev);
-    /* make the actual value visible */
-    proxy->nvectors = vdev->nvectors;
-    return 0;
-}
-
 static void virtio_exit_pci(PCIDevice *pci_dev)
 {
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
@@ -762,15 +742,6 @@ static void virtio_exit_pci(PCIDevice *pci_dev)
     msix_uninit_exclusive_bar(pci_dev);
 }
 
-static void virtio_blk_exit_pci(PCIDevice *pci_dev)
-{
-    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
-
-    virtio_pci_stop_ioeventfd(proxy);
-    virtio_blk_exit(proxy->vdev);
-    virtio_exit_pci(pci_dev);
-}
-
 static int virtio_serial_init_pci(PCIDevice *pci_dev)
 {
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
@@ -888,43 +859,6 @@ static void virtio_rng_exit_pci(PCIDevice *pci_dev)
     virtio_exit_pci(pci_dev);
 }
 
-static Property virtio_blk_properties[] = {
-    DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
-    DEFINE_BLOCK_PROPERTIES(VirtIOPCIProxy, blk.conf),
-    DEFINE_BLOCK_CHS_PROPERTIES(VirtIOPCIProxy, blk.conf),
-    DEFINE_PROP_STRING("serial", VirtIOPCIProxy, blk.serial),
-#ifdef __linux__
-    DEFINE_PROP_BIT("scsi", VirtIOPCIProxy, blk.scsi, 0, true),
-#endif
-    DEFINE_PROP_BIT("config-wce", VirtIOPCIProxy, blk.config_wce, 0, true),
-    DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
-    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
-    DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void virtio_blk_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-
-    k->init = virtio_blk_init_pci;
-    k->exit = virtio_blk_exit_pci;
-    k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
-    k->device_id = PCI_DEVICE_ID_VIRTIO_BLOCK;
-    k->revision = VIRTIO_PCI_ABI_VERSION;
-    k->class_id = PCI_CLASS_STORAGE_SCSI;
-    dc->reset = virtio_pci_reset;
-    dc->props = virtio_blk_properties;
-}
-
-static TypeInfo virtio_blk_info = {
-    .name          = "virtio-blk-pci",
-    .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(VirtIOPCIProxy),
-    .class_init    = virtio_blk_class_init,
-};
-
 static Property virtio_net_properties[] = {
     DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, false),
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
@@ -1243,6 +1177,51 @@ static const TypeInfo virtio_pci_info = {
     .class_size    = sizeof(VirtioPCIClass),
 };
 
+/* virtio-blk-pci */
+
+static Property virtio_blk_pci_properties[] = {
+    DEFINE_PROP_HEX32("class", VirtIOBlkPCI, parent_obj.class_code, 0),
+    DEFINE_BLOCK_PROPERTIES(VirtIOBlkPCI, blk.conf),
+    DEFINE_BLOCK_CHS_PROPERTIES(VirtIOBlkPCI, blk.conf),
+    DEFINE_PROP_STRING("serial", VirtIOBlkPCI, blk.serial),
+#ifdef __linux__
+    DEFINE_PROP_BIT("scsi", VirtIOBlkPCI, blk.scsi, 0, true),
+#endif
+    DEFINE_PROP_BIT("config-wce", VirtIOBlkPCI, blk.config_wce, 0, true),
+    DEFINE_PROP_BIT("ioeventfd", VirtIOBlkPCI, parent_obj.flags,
+                    VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
+    DEFINE_PROP_UINT32("vectors", VirtIOBlkPCI, parent_obj.nvectors, 2),
+    DEFINE_VIRTIO_BLK_FEATURES(VirtIOBlkPCI, parent_obj.host_features),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static int virtio_blk_pci_init(VirtIOPCIProxy *vpci_dev)
+{
+    DeviceState *vdev;
+    VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(vpci_dev);
+    vdev = qdev_create(BUS(vpci_dev->bus), "virtio-blk");
+    virtio_blk_set_conf(vdev, &(dev->blk));
+    if (qdev_init(vdev) < 0) {
+        return -1;
+    }
+    return 0;
+}
+
+static void virtio_blk_pci_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
+    dc->props = virtio_blk_pci_properties;
+    k->init = virtio_blk_pci_init;
+}
+
+static const TypeInfo virtio_blk_pci_info = {
+    .name          = TYPE_VIRTIO_BLK_PCI,
+    .parent        = TYPE_VIRTIO_PCI,
+    .instance_size = sizeof(VirtIOBlkPCI),
+    .class_init    = virtio_blk_pci_class_init,
+};
+
 /* virtio-pci-bus */
 
 VirtioBusState *virtio_pci_bus_new(VirtIOPCIProxy *dev)
@@ -1282,7 +1261,6 @@ static const TypeInfo virtio_pci_bus_info = {
 
 static void virtio_pci_register_types(void)
 {
-    type_register_static(&virtio_blk_info);
     type_register_static(&virtio_net_info);
     type_register_static(&virtio_serial_info);
     type_register_static(&virtio_balloon_info);
@@ -1290,6 +1268,7 @@ static void virtio_pci_register_types(void)
     type_register_static(&virtio_rng_info);
     type_register_static(&virtio_pci_bus_info);
     type_register_static(&virtio_pci_info);
+    type_register_static(&virtio_blk_pci_info);
 }
 
 type_init(virtio_pci_register_types)
diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h
index e840cea..8a68d6e 100644
--- a/hw/virtio-pci.h
+++ b/hw/virtio-pci.h
@@ -24,6 +24,7 @@
 
 /* VirtIOPCIProxy will be renammed VirtioPCIState at the end. */
 typedef struct VirtIOPCIProxy VirtIOPCIProxy;
+typedef struct VirtIOBlkPCI VirtIOBlkPCI;
 
 /* virtio-pci-bus */
 #define TYPE_VIRTIO_PCI_BUS "virtio-pci-bus"
@@ -69,7 +70,6 @@ struct VirtIOPCIProxy {
     uint32_t flags;
     uint32_t class_code;
     uint32_t nvectors;
-    VirtIOBlkConf blk;
     NICConf nic;
     uint32_t host_features;
 #ifdef CONFIG_LINUX
@@ -87,6 +87,18 @@ struct VirtIOPCIProxy {
     /* Nothing more for the moment. */
 };
 
+/*
+ * virtio-blk-pci : This extends VirtioPCIProxy.
+ */
+#define TYPE_VIRTIO_BLK_PCI "virtio-blk-pci"
+#define VIRTIO_BLK_PCI(obj) \
+        OBJECT_CHECK(VirtIOBlkPCI, (obj), TYPE_VIRTIO_BLK_PCI)
+
+struct VirtIOBlkPCI {
+    VirtIOPCIProxy parent_obj;
+    VirtIOBlkConf blk;
+};
+
 void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev);
 void virtio_pci_reset(DeviceState *d);
 
-- 
1.7.11.7

Re: [Qemu-devel] [RFC PATCH v7 7/8] virtio-pci-blk : Switch to new API.

[Peter Maydell]

On 10 December 2012 16:45,  <fred.konrad@greensocs.com> wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> Here the virtio-blk-pci is modified for the new API. The device virtio-pci-blk
> extends virtio-pci. It creates and connects a virtio-blk during the init.

Did you check whether this maintains backwards compatibility
for vmstate migration information and device properties?
(haven't investigated myself yet, just asking whether you have)

-- PMM

Re: [Qemu-devel] [RFC PATCH v7 7/8] virtio-pci-blk : Switch to new API.

[KONRAD Frédéric]

On 11/12/2012 18:50, Peter Maydell wrote:
> On 10 December 2012 16:45,  <fred.konrad@greensocs.com> wrote:
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> Here the virtio-blk-pci is modified for the new API. The device virtio-pci-blk
>> extends virtio-pci. It creates and connects a virtio-blk during the init.
> Did you check whether this maintains backwards compatibility
> for vmstate migration information and device properties?
> (haven't investigated myself yet, just asking whether you have)
>
> -- PMM
What do you mean exactly by backwards compatibility for vmstate migration ?

The device properties didn't change.
The virtio-blk-pci creates a virtio-blk device, puts the virtio block 
properties and  inits the virtio-blk.

Re: [Qemu-devel] [RFC PATCH v7 7/8] virtio-pci-blk : Switch to new API.

[Peter Maydell]

On 10 December 2012 16:45,  <fred.konrad@greensocs.com> wrote:
> -static void virtio_blk_class_init(ObjectClass *klass, void *data)
> -{
> -    DeviceClass *dc = DEVICE_CLASS(klass);
> -    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> -
> -    k->init = virtio_blk_init_pci;
> -    k->exit = virtio_blk_exit_pci;
> -    k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
> -    k->device_id = PCI_DEVICE_ID_VIRTIO_BLOCK;
> -    k->revision = VIRTIO_PCI_ABI_VERSION;
> -    k->class_id = PCI_CLASS_STORAGE_SCSI;
> -    dc->reset = virtio_pci_reset;
> -    dc->props = virtio_blk_properties;
> -}

This hunk removes the setting of the PCI vendor and device IDs
but I can't see where they are set in the new code.

How will the PCI transport's PCI vendor/device/class IDs be
set (a) when a virtio-blk backend is created and separately
plugged into a virtio-pci transport (b) for the legacy
virtio-pci-blk? [ideally the answer to (b) should be "in the
same way as for (a)"]

-- PMM

Re: [Qemu-devel] [RFC PATCH v7 7/8] virtio-pci-blk : Switch to new API.

[Andreas Färber]

Am 12.12.2012 15:25, schrieb Peter Maydell:
> How will the PCI transport's PCI vendor/device/class IDs be
> set (a) when a virtio-blk backend is created and separately
> plugged into a virtio-pci transport (b) for the legacy
> virtio-pci-blk? [ideally the answer to (b) should be "in the
> same way as for (a)"]

The obvious answer would be that PCI properties need to be set on the
PCI device, not an a VirtioDevice sitting on a virtio-bus.

I.e., with the proposed refactoring we'd have on the virtio-bus:

- VirtioDevice
  + VirtioBlockDevice
  + VirtioSCSIDevice - has-a scsi-bus
  ...

In turn that means that every VirtioDevice to be exposed as PCI device
to the guest needs it own PCIDevice exposing a private virtio-bus.

- PCIDevice
  - VirtioPCIDevice - has-a virtio-bus
    + virtio-blk-pci - has-a VirtioBlockDevice on its virtio-bus
    + virtio-scsi-pci - has-a VirtioSCSIDevice on its virtio-bus
    ...

This also happens to solve most of the migration compatibility pretty
nicely because the wapping PCI devices would be used almost as before,
some state may need to be forwarded to the VirtioDevice.

Finally supplying a public device_initialize() or so would be helpful
for the latter since VMState cannot cross pointers IIRC. I'll look into
that part since inlining the old qdev functions cripples my Tegra work.

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 v7 7/8] virtio-pci-blk : Switch to new API.

[Paolo Bonzini]

Il 12/12/2012 18:53, Andreas Färber ha scritto:
> Am 12.12.2012 15:25, schrieb Peter Maydell:
>> How will the PCI transport's PCI vendor/device/class IDs be
>> set (a) when a virtio-blk backend is created and separately
>> plugged into a virtio-pci transport (b) for the legacy
>> virtio-pci-blk? [ideally the answer to (b) should be "in the
>> same way as for (a)"]
> 
> The obvious answer would be that PCI properties need to be set on the
> PCI device, not an a VirtioDevice sitting on a virtio-bus.

Yes, but the question is *how*... if there will be no usable "-device
virtio-pci", the value of this refactoring becomes a bit lower...

Paolo

> I.e., with the proposed refactoring we'd have on the virtio-bus:
> 
> - VirtioDevice
>   + VirtioBlockDevice
>   + VirtioSCSIDevice - has-a scsi-bus
>   ...
> 
> In turn that means that every VirtioDevice to be exposed as PCI device
> to the guest needs it own PCIDevice exposing a private virtio-bus.
> 
> - PCIDevice
>   - VirtioPCIDevice - has-a virtio-bus
>     + virtio-blk-pci - has-a VirtioBlockDevice on its virtio-bus
>     + virtio-scsi-pci - has-a VirtioSCSIDevice on its virtio-bus
>     ...
> 
> This also happens to solve most of the migration compatibility pretty
> nicely because the wapping PCI devices would be used almost as before,
> some state may need to be forwarded to the VirtioDevice.
> 
> Finally supplying a public device_initialize() or so would be helpful
> for the latter since VMState cannot cross pointers IIRC. I'll look into
> that part since inlining the old qdev functions cripples my Tegra work.
> 
> Andreas
> 

Re: [Qemu-devel] [RFC PATCH v7 7/8] virtio-pci-blk : Switch to new API.

[Peter Maydell]

On 12 December 2012 17:53, Andreas Färber <afaerber@suse.de> wrote:
> Am 12.12.2012 15:25, schrieb Peter Maydell:
>> How will the PCI transport's PCI vendor/device/class IDs be
>> set (a) when a virtio-blk backend is created and separately
>> plugged into a virtio-pci transport (b) for the legacy
>> virtio-pci-blk? [ideally the answer to (b) should be "in the
>> same way as for (a)"]
>
> The obvious answer would be that PCI properties need to be set on the
> PCI device, not an a VirtioDevice sitting on a virtio-bus.
>
> I.e., with the proposed refactoring we'd have on the virtio-bus:
>
> - VirtioDevice
>   + VirtioBlockDevice
>   + VirtioSCSIDevice - has-a scsi-bus
>   ...
>
> In turn that means that every VirtioDevice to be exposed as PCI device
> to the guest needs it own PCIDevice exposing a private virtio-bus.
>
> - PCIDevice
>   - VirtioPCIDevice - has-a virtio-bus
>     + virtio-blk-pci - has-a VirtioBlockDevice on its virtio-bus
>     + virtio-scsi-pci - has-a VirtioSCSIDevice on its virtio-bus
>     ...

...this bit is only for legacy back-compat. It should be equally
valid to just use the PCI transport plugged into a VirtioDevice,
both of which were created by the user with -device [and for
new transports, separate transport and backend should be the
standard]. That means the virtio-bus interface needs a way for
the backend to announce to the transport what it is so that
the PCI transport can set the right PCI IDs.

-- PMM

Re: [Qemu-devel] [RFC PATCH v7 7/8] virtio-pci-blk : Switch to new API.

[Paolo Bonzini]

Il 12/12/2012 18:58, Peter Maydell ha scritto:
> It should be equally
> valid to just use the PCI transport plugged into a VirtioDevice,
> both of which were created by the user with -device [and for
> new transports, separate transport and backend should be the
> standard]. That means the virtio-bus interface needs a way for
> the backend to announce to the transport what it is so that
> the PCI transport can set the right PCI IDs.

There is such an interface (the device_id, aka VIRTIO_ID_*).  Then
virtio-pci needs a mapping from the device_id to the (default)
vendor_id/device_id/class tuple.

Paolo

Re: [Qemu-devel] [RFC PATCH v7 7/8] virtio-pci-blk : Switch to new API.

[Michael S. Tsirkin]

On Wed, Dec 12, 2012 at 07:03:21PM +0100, Paolo Bonzini wrote:
> Il 12/12/2012 18:58, Peter Maydell ha scritto:
> > It should be equally
> > valid to just use the PCI transport plugged into a VirtioDevice,
> > both of which were created by the user with -device [and for
> > new transports, separate transport and backend should be the
> > standard]. That means the virtio-bus interface needs a way for
> > the backend to announce to the transport what it is so that
> > the PCI transport can set the right PCI IDs.
> 
> There is such an interface (the device_id, aka VIRTIO_ID_*).  Then
> virtio-pci needs a mapping from the device_id to the (default)
> vendor_id/device_id/class tuple.
> 
> Paolo

At least device id and vedor id are easy.

Re: [Qemu-devel] [RFC PATCH v7 7/8] virtio-pci-blk : Switch to new API.

[Paolo Bonzini]

Il 12/12/2012 22:22, Michael S. Tsirkin ha scritto:
> > > It should be equally
> > > valid to just use the PCI transport plugged into a VirtioDevice,
> > > both of which were created by the user with -device [and for
> > > new transports, separate transport and backend should be the
> > > standard]. That means the virtio-bus interface needs a way for
> > > the backend to announce to the transport what it is so that
> > > the PCI transport can set the right PCI IDs.
> > 
> > There is such an interface (the device_id, aka VIRTIO_ID_*).  Then
> > virtio-pci needs a mapping from the device_id to the (default)
> > vendor_id/device_id/class tuple.
> 
> At least device id and vedor id are easy.

Class too, this is a new device virtio-pci so it doesn't need to support
old machine types.

Paolo

Re: [Qemu-devel] [RFC PATCH v7 7/8] virtio-pci-blk : Switch to new API.

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 12/12/2012 18:58, Peter Maydell ha scritto:
>> It should be equally
>> valid to just use the PCI transport plugged into a VirtioDevice,
>> both of which were created by the user with -device [and for
>> new transports, separate transport and backend should be the
>> standard]. That means the virtio-bus interface needs a way for
>> the backend to announce to the transport what it is so that
>> the PCI transport can set the right PCI IDs.
>
> There is such an interface (the device_id, aka VIRTIO_ID_*).  Then
> virtio-pci needs a mapping from the device_id to the (default)
> vendor_id/device_id/class tuple.

Why?

I think it's perfectly fine to have to specify a device ID for
virtio-pci.

The way virtio-pci is designed is such that every device that uses
virtio-pci ends up looking like an independent PCI device.

We should always have virtio-blk-pci, virtio-net-pci, etc.  The goal of
this refactoring should not be to eliminate that.

But these devices should be trivial to implement and modelled in a sane
way.

I don't think we should be trying to solve the problem of making
virtio-pci "easy to use".  Why would you say:

  -device virtio-pci,id=foo -device virtio-blk,bus=foo

When you can just say:

  -device virtio-pci-blk

I think we're optimizing for the wrong thing here...

Regards,

Anthony Liguori

>
> Paolo

Re: [Qemu-devel] [RFC PATCH v7 7/8] virtio-pci-blk : Switch to new API.

[Michael S. Tsirkin]

On Thu, Dec 13, 2012 at 08:51:30AM -0600, Anthony Liguori wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > Il 12/12/2012 18:58, Peter Maydell ha scritto:
> >> It should be equally
> >> valid to just use the PCI transport plugged into a VirtioDevice,
> >> both of which were created by the user with -device [and for
> >> new transports, separate transport and backend should be the
> >> standard]. That means the virtio-bus interface needs a way for
> >> the backend to announce to the transport what it is so that
> >> the PCI transport can set the right PCI IDs.
> >
> > There is such an interface (the device_id, aka VIRTIO_ID_*).  Then
> > virtio-pci needs a mapping from the device_id to the (default)
> > vendor_id/device_id/class tuple.
> 
> Why?
> 
> I think it's perfectly fine to have to specify a device ID for
> virtio-pci.
> 
> The way virtio-pci is designed is such that every device that uses
> virtio-pci ends up looking like an independent PCI device.
> 
> We should always have virtio-blk-pci, virtio-net-pci, etc.  The goal of
> this refactoring should not be to eliminate that.
> 
> But these devices should be trivial to implement and modelled in a sane
> way.
> 
> I don't think we should be trying to solve the problem of making
> virtio-pci "easy to use".  Why would you say:
> 
>   -device virtio-pci,id=foo -device virtio-blk,bus=foo
> 
> When you can just say:
> 
>   -device virtio-pci-blk
> 
> I think we're optimizing for the wrong thing here...
> 
> Regards,
> 
> Anthony Liguori
> 
> >
> > Paolo


I agree. What I mean is this: virtio has device IDs.
These are not pci specific and are defined in linux/virtio_ids.h
Now it looks like pci device IDs of virtio devices are
pci id = 0x1000 + virtio device ID; so let's pull virtio_ids.h
from linux and write a macro that does + 0x1000 instead of hard-coding
values.

Makes sense?

-- 
MST

Re: [Qemu-devel] [RFC PATCH v7 7/8] virtio-pci-blk : Switch to new API.

[KONRAD Frédéric]

On 12/12/2012 18:58, Peter Maydell wrote:
> On 12 December 2012 17:53, Andreas Färber <afaerber@suse.de> wrote:
>> Am 12.12.2012 15:25, schrieb Peter Maydell:
>>> How will the PCI transport's PCI vendor/device/class IDs be
>>> set (a) when a virtio-blk backend is created and separately
>>> plugged into a virtio-pci transport (b) for the legacy
>>> virtio-pci-blk? [ideally the answer to (b) should be "in the
>>> same way as for (a)"]
>> The obvious answer would be that PCI properties need to be set on the
>> PCI device, not an a VirtioDevice sitting on a virtio-bus.
>>
>> I.e., with the proposed refactoring we'd have on the virtio-bus:
>>
>> - VirtioDevice
>>    + VirtioBlockDevice
>>    + VirtioSCSIDevice - has-a scsi-bus
>>    ...
>>
>> In turn that means that every VirtioDevice to be exposed as PCI device
>> to the guest needs it own PCIDevice exposing a private virtio-bus.
>>
>> - PCIDevice
>>    - VirtioPCIDevice - has-a virtio-bus
>>      + virtio-blk-pci - has-a VirtioBlockDevice on its virtio-bus
>>      + virtio-scsi-pci - has-a VirtioSCSIDevice on its virtio-bus
>>      ...
> ...this bit is only for legacy back-compat. It should be equally
> valid to just use the PCI transport plugged into a VirtioDevice,
> both of which were created by the user with -device [and for
> new transports, separate transport and backend should be the
> standard]. That means the virtio-bus interface needs a way for
> the backend to announce to the transport what it is so that
> the PCI transport can set the right PCI IDs.
>
> -- PMM
Yes, it's done with uint16_t get_virtio_device_id(VirtioBusState *bus) 
function from second step.

Re: [Qemu-devel] [RFC PATCH v7 7/8] virtio-pci-blk : Switch to new API.

[Andreas Färber]

Am 12.12.2012 18:53, schrieb Andreas Färber:
> Finally supplying a public device_initialize() or so would be helpful
> for the latter since VMState cannot cross pointers IIRC. I'll look into
> that part since inlining the old qdev functions cripples my Tegra work.

Investigating this, it turned out that my branch was outdated in still
calling qdev_prop_set_globals(), which some good soul (Paolo?) inlined
into device initfn. So the only step taken by qdev_try_create() that
remains after object_initialize() is qdev_set_parent_bus(), which I
think does not warrant a

device_post_initialize:
-> qdev_set_parent_bus()

qdev_try_create:
-> object_new()
-> device_post_initialize()

qdev_create:
-> qdev_try_create()

device_initialize:
-> object_initialize()
-> device_post_initialize()

construction.

So, given that virtio device states are in (or moved to) a header, they
can already be embedded in a has-a virtio-bus has-a Virtio*Device device
today and initialized with object_initialize(&s->the_device) and
qdev_set_parent_bus(&s->the_device, &s->the_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 v7 7/8] virtio-pci-blk : Switch to new API.

[KONRAD Frédéric]

On 12/12/2012 15:25, Peter Maydell wrote:
> On 10 December 2012 16:45,  <fred.konrad@greensocs.com> wrote:
>> -static void virtio_blk_class_init(ObjectClass *klass, void *data)
>> -{
>> -    DeviceClass *dc = DEVICE_CLASS(klass);
>> -    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> -
>> -    k->init = virtio_blk_init_pci;
>> -    k->exit = virtio_blk_exit_pci;
>> -    k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
>> -    k->device_id = PCI_DEVICE_ID_VIRTIO_BLOCK;
>> -    k->revision = VIRTIO_PCI_ABI_VERSION;
>> -    k->class_id = PCI_CLASS_STORAGE_SCSI;
>> -    dc->reset = virtio_pci_reset;
>> -    dc->props = virtio_blk_properties;
>> -}
> This hunk removes the setting of the PCI vendor and device IDs
> but I can't see where they are set in the new code.
>
> How will the PCI transport's PCI vendor/device/class IDs be
> set (a) when a virtio-blk backend is created and separately
> plugged into a virtio-pci transport (b) for the legacy
> virtio-pci-blk? [ideally the answer to (b) should be "in the
> same way as for (a)"]
>
> -- PMM

It's done in the virtio_pci_device_plugged(), ( step 4 )
At this time we have the device ID, so we can put the PCI IDs :

+static void virtio_pci_device_plugged(void *opaque)
+{
+    VirtIOPCIProxy *proxy = VIRTIO_PCI(opaque);
+    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;
+
+    }

I'll move the "case" to the step 7 as it should be.

Fred

Re: [Qemu-devel] [RFC PATCH v7 7/8] virtio-pci-blk : Switch to new API.

[KONRAD Frédéric]

On 13/12/2012 09:24, KONRAD Frédéric wrote:
> On 12/12/2012 15:25, Peter Maydell wrote:
>> On 10 December 2012 16:45, <fred.konrad@greensocs.com> wrote:
>>> -static void virtio_blk_class_init(ObjectClass *klass, void *data)
>>> -{
>>> -    DeviceClass *dc = DEVICE_CLASS(klass);
>>> -    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>> -
>>> -    k->init = virtio_blk_init_pci;
>>> -    k->exit = virtio_blk_exit_pci;
>>> -    k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
>>> -    k->device_id = PCI_DEVICE_ID_VIRTIO_BLOCK;
>>> -    k->revision = VIRTIO_PCI_ABI_VERSION;
>>> -    k->class_id = PCI_CLASS_STORAGE_SCSI;
>>> -    dc->reset = virtio_pci_reset;
>>> -    dc->props = virtio_blk_properties;
>>> -}
>> This hunk removes the setting of the PCI vendor and device IDs
>> but I can't see where they are set in the new code.
>>
>> How will the PCI transport's PCI vendor/device/class IDs be
>> set (a) when a virtio-blk backend is created and separately
>> plugged into a virtio-pci transport (b) for the legacy
>> virtio-pci-blk? [ideally the answer to (b) should be "in the
>> same way as for (a)"]
>>
>> -- PMM
>
> It's done in the virtio_pci_device_plugged(), ( step 4 )
> At this time we have the device ID, so we can put the PCI IDs :
>
> +static void virtio_pci_device_plugged(void *opaque)
> +{
> +    VirtIOPCIProxy *proxy = VIRTIO_PCI(opaque);
> +    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;
> +
> +    }
>
> I'll move the "case" to the step 7 as it should be.
>
I meant step 6* : Add the virtio-blk device.

the virtio-blk-pci set the PCI IDs on the same way as for virtio-blk.

> Fred
>
>

[Qemu-devel] [RFC PATCH v7 8/8] virtio-blk : QOM modifications.

[fred.konrad]

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

As the virtio-blk-pci is switched to the new API, we can use QOM casts and
remove the separate init for the old API.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 hw/virtio-blk.c | 69 ++++++++++++++++++++++-----------------------------------
 1 file changed, 26 insertions(+), 43 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 58c9086..9f5865d 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -25,7 +25,7 @@
 
 typedef struct VirtIOBlock
 {
-    VirtIODevice vdev;
+    VirtIODevice parent_obj;
     BlockDriverState *bs;
     VirtQueue *vq;
     void *rq;
@@ -36,14 +36,6 @@ typedef struct VirtIOBlock
     DeviceState *qdev;
 } VirtIOBlock;
 
-/*
- * Moving to QOM later in this series.
- */
-static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
-{
-    return (VirtIOBlock *)vdev;
-}
-
 typedef struct VirtIOBlockReq
 {
     VirtIOBlock *dev;
@@ -59,12 +51,13 @@ typedef struct VirtIOBlockReq
 static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
 {
     VirtIOBlock *s = req->dev;
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
     trace_virtio_blk_req_complete(req, status);
 
     stb_p(&req->in->status, status);
     virtqueue_push(s->vq, &req->elem, req->qiov.size + sizeof(*req->in));
-    virtio_notify(&s->vdev, s->vq);
+    virtio_notify(vdev, s->vq);
 }
 
 static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
@@ -405,7 +398,7 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
 
 static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
-    VirtIOBlock *s = to_virtio_blk(vdev);
+    VirtIOBlock *s = VIRTIO_BLK(vdev);
     VirtIOBlockReq *req;
     MultiReqBuffer mrb = {
         .num_writes = 0,
@@ -472,7 +465,7 @@ static void virtio_blk_reset(VirtIODevice *vdev)
  */
 static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
 {
-    VirtIOBlock *s = to_virtio_blk(vdev);
+    VirtIOBlock *s = VIRTIO_BLK(vdev);
     struct virtio_blk_config blkcfg;
     uint64_t capacity;
     int blk_size = s->conf->logical_block_size;
@@ -511,7 +504,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
 
 static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
 {
-    VirtIOBlock *s = to_virtio_blk(vdev);
+    VirtIOBlock *s = VIRTIO_BLK(vdev);
     struct virtio_blk_config blkcfg;
 
     memcpy(&blkcfg, config, sizeof(blkcfg));
@@ -520,7 +513,7 @@ static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
 
 static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features)
 {
-    VirtIOBlock *s = to_virtio_blk(vdev);
+    VirtIOBlock *s = VIRTIO_BLK(vdev);
 
     features |= (1 << VIRTIO_BLK_F_SEG_MAX);
     features |= (1 << VIRTIO_BLK_F_GEOMETRY);
@@ -539,7 +532,7 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features)
 
 static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
 {
-    VirtIOBlock *s = to_virtio_blk(vdev);
+    VirtIOBlock *s = VIRTIO_BLK(vdev);
     uint32_t features;
 
     if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
@@ -553,9 +546,10 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
 static void virtio_blk_save(QEMUFile *f, void *opaque)
 {
     VirtIOBlock *s = opaque;
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
     VirtIOBlockReq *req = s->rq;
 
-    virtio_save(&s->vdev, f);
+    virtio_save(vdev, f);
     
     while (req) {
         qemu_put_sbyte(f, 1);
@@ -567,13 +561,14 @@ static void virtio_blk_save(QEMUFile *f, void *opaque)
 
 static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
 {
-    VirtIOBlock *s = opaque;
+    VirtIOBlock *s = VIRTIO_BLK(opaque);
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
     int ret;
 
     if (version_id != 2)
         return -EINVAL;
 
-    ret = virtio_load(&s->vdev, f);
+    ret = virtio_load(vdev, f);
     if (ret) {
         return ret;
     }
@@ -595,9 +590,9 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
 
 static void virtio_blk_resize(void *opaque)
 {
-    VirtIOBlock *s = opaque;
+    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
 
-    virtio_notify_config(&s->vdev);
+    virtio_notify_config(vdev);
 }
 
 static const BlockDevOps virtio_block_ops = {
@@ -614,6 +609,7 @@ static VirtIODevice *virtio_blk_common_init(DeviceState *dev,
                                           VirtIOBlkConf *blk, VirtIOBlock **ps)
 {
     VirtIOBlock *s = *ps;
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
     static int virtio_blk_id;
 
     if (!blk->conf.bs) {
@@ -630,33 +626,21 @@ static VirtIODevice *virtio_blk_common_init(DeviceState *dev,
         return NULL;
     }
 
-    /*
-     * We have two cases here : the old virtio-blk-pci device, and the
-     * refactored virtio-blk.
-     */
-    if (s == NULL) {
-        /* virtio-blk-pci */
-        s = (VirtIOBlock *)virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
-                                              sizeof(struct virtio_blk_config),
-                                              sizeof(VirtIOBlock));
-    } else {
-        /* virtio-blk */
-        virtio_init(VIRTIO_DEVICE(s), "virtio-blk", VIRTIO_ID_BLOCK,
-                    sizeof(struct virtio_blk_config));
-    }
+    virtio_init(VIRTIO_DEVICE(s), "virtio-blk", VIRTIO_ID_BLOCK,
+                sizeof(struct virtio_blk_config));
 
-    s->vdev.get_config = virtio_blk_update_config;
-    s->vdev.set_config = virtio_blk_set_config;
-    s->vdev.get_features = virtio_blk_get_features;
-    s->vdev.set_status = virtio_blk_set_status;
-    s->vdev.reset = virtio_blk_reset;
+    vdev->get_config = virtio_blk_update_config;
+    vdev->set_config = virtio_blk_set_config;
+    vdev->get_features = virtio_blk_get_features;
+    vdev->set_status = virtio_blk_set_status;
+    vdev->reset = virtio_blk_reset;
     s->bs = blk->conf.bs;
     s->conf = &blk->conf;
     virtio_blk_set_conf(dev, blk);
     s->rq = NULL;
     s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1;
 
-    s->vq = virtio_add_queue(&s->vdev, 128, virtio_blk_handle_output);
+    s->vq = virtio_add_queue(vdev, 128, virtio_blk_handle_output);
 
     qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
     s->qdev = dev;
@@ -668,7 +652,7 @@ static VirtIODevice *virtio_blk_common_init(DeviceState *dev,
     bdrv_iostatus_enable(s->bs);
     add_boot_device_path(s->conf->bootindex, dev, "/disk@0,0");
 
-    return &s->vdev;
+    return vdev;
 }
 
 VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk)
@@ -679,13 +663,12 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk)
 
 void virtio_blk_exit(VirtIODevice *vdev)
 {
-    VirtIOBlock *s = to_virtio_blk(vdev);
+    VirtIOBlock *s = VIRTIO_BLK(vdev);
     unregister_savevm(s->qdev, "virtio-blk", s);
     blockdev_mark_auto_del(s->bs);
     virtio_cleanup(vdev);
 }
 
-
 static int virtio_device_init(DeviceState *qdev)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
-- 
1.7.11.7

Re: [Qemu-devel] [RFC PATCH v7 8/8] virtio-blk : QOM modifications.

[Peter Maydell]

On 10 December 2012 16:45,  <fred.konrad@greensocs.com> wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> As the virtio-blk-pci is switched to the new API, we can use QOM casts and
> remove the separate init for the old API.

This patch effectively breaks virtio_blk_init() [because it
removes the code path that handles s==NULL] but it does not
actually remove that function, so it has not completely cleaned
up the legacy code. It should be possible to inline the
virtio_blk_common_init() function into virtio_device_init()
then. We should end up with all the legacy stuff removed so
the only APIs for manipulating virtio-blk are the qdev ones.

Also some cleanup patch somewhere needs to get rid of the
function pointers in VirtIODevice.

(If the cleanup phase looks like it would be a bit hard to
understand done as a single patch, feel free to split it suitably.)

Incidentally this patch should probably have a note in the
commit message that it's only intended to be applied after
conversion of all the transports (ie of s390).

Basically you need to keep going here to actually finish the
demonstration of what the final cleaned up transport and
backend look like.

-- PMM

Re: [Qemu-devel] [RFC PATCH v7 0/8] Virtio refactoring.

[Peter Maydell]

On 10 December 2012 16:45,  <fred.konrad@greensocs.com> wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> You can clone that from here :
> git.greensocs.com/home/greensocs/git/qemu_virtio.git virtio_refactoring_v7
>
> These are the two last steps of refactoring ( only for virtio-blk device. ):
>
>     * It modifies virtio-blk-pci to extend virtio-pci and to connect a
>       virtio-blk during the initialisation.
>     * It modifies virtio-blk ( QOM cast ).
>
> I think the last step is breaking virtio-blk-s390, can somebody confirm that?
>
> If it is breaking virtio-blk-s390 how can I modify it ?

The idea would be that the final patchset for committing would
convert the s390 transport as well as the pci transport before
doing the final cleanup and removal of the legacy support.
(Similarly it would convert all the backends, not just blk).
It would probably be useful in the cover letter for the next
series to indicate the points at which patches converting the
other transports and backends would be inserted.

Ideally if your patches to convert the pci transport are
well written it should be clear from them how to convert the
s390 transport. We can probably persuade somebody to assist
with testing at that point...

-- PMM