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

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

[fred.konrad]

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

I think the global structure is good as we discuted in the V4. So, I tried to
start device refactoring, and also made some little fix.

For the device refactoring, what are you suggesting for replacing
VirtIOBindings ?

Also, the virtio_pci_init_cb(..) currently use direct access to VirtIODevice,
can I just implement functions in virtio-bus to access the fields ?
( eg : get/set_nvectors ).

Finally, any idea on how I can make this patch-set for not breaking anything ?
The virtio-blk refactoring breaks all virtio-blk devices.

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.

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 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 (6):
  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 : Refactor virtio-blk.

 hw/Makefile.objs  |   1 +
 hw/qdev-core.h    |   2 +
 hw/qdev-monitor.c |  11 ++++
 hw/virtio-blk.c   | 170 ++++++++++++++++++++++++++++++++++++++++++++++--------
 hw/virtio-blk.h   |   4 ++
 hw/virtio-bus.c   | 111 +++++++++++++++++++++++++++++++++++
 hw/virtio-bus.h   |  76 ++++++++++++++++++++++++
 hw/virtio-pci.c   | 149 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-pci.h   |  33 ++++++++++-
 hw/virtio.c       |  56 ++++++++++++++++++
 hw/virtio.h       |  29 ++++++++++
 11 files changed, 616 insertions(+), 26 deletions(-)
 create mode 100644 hw/virtio-bus.c
 create mode 100644 hw/virtio-bus.h

-- 
1.7.11.7

[Qemu-devel] [RFC PATCH v5 1/6] 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 v5 2/6] 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  | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-bus.h  |  76 +++++++++++++++++++++++++++++++++++++
 3 files changed, 188 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..ccf1cb0
--- /dev/null
+++ b/hw/virtio-bus.c
@@ -0,0 +1,111 @@
+/*
+ * VirtioBus
+ *
+ *  Copyright (C) 2012 : GreenSocs Ltd
+ *      http://www.greensocs.com/ , email: info@greensocs.com
+ *
+ *  Developed by :
+ *  Frederic Konrad   <fred.konrad@greensocs.com>
+ *
+ * This 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->init != NULL) {
+        klass->init(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;
+}
+
+/* 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->exit != NULL) {
+            klass->exit(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..570ad98
--- /dev/null
+++ b/hw/virtio-bus.h
@@ -0,0 +1,76 @@
+/*
+ * 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. */
+    void (*init)(void *opaque);
+    /* transport independent exit function. */
+    void (*exit)(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_destroy_device(VirtioBusState *bus);
+uint16_t get_virtio_device_id(VirtioBusState *bus);
+#endif /* VIRTIO_BUS_H */
-- 
1.7.11.7

[Qemu-devel] [RFC PATCH v5 3/6] 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 v5 4/6] 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 | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-pci.h |  14 +++++++
 2 files changed, 126 insertions(+)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 5ac8d0d..8426122 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -1119,6 +1119,115 @@ static TypeInfo virtio_scsi_info = {
     .class_init    = virtio_scsi_class_init,
 };
 
+/*
+ * virtio-pci : This is the PCIDevice which have a virtio-pci-bus.
+ */
+
+/* init callback */
+static void virtio_pci_init_cb(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);
+}
+
+/* exit callback */
+static void virtio_pci_exit_cb(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);
+    dev->bus = virtio_pci_bus_new(dev);
+    return 0;
+}
+
+static void virtio_pci_exit(PCIDevice *pci_dev)
+{
+    DeviceState *qdev = DEVICE(pci_dev);
+    BusState *qbus = BUS(qdev_get_parent_bus(qdev));
+    VirtioBusState *bus = VIRTIO_BUS(qbus);
+    virtio_bus_destroy_device(bus);
+    qbus_free(qbus);
+}
+
+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_reset;
+}
+
+static const TypeInfo virtio_pci_info = {
+    .name          = TYPE_VIRTIO_PCI,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(VirtIOPCIProxy),
+    .class_init    = virtio_pci_class_init,
+};
+
 /* virtio-pci-bus */
 
 VirtioBusState *virtio_pci_bus_new(VirtIOPCIProxy *dev)
@@ -1145,6 +1254,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->init = virtio_pci_init_cb;
+    k->exit = virtio_pci_exit_cb;
 }
 
 static const TypeInfo virtio_pci_bus_info = {
@@ -1163,6 +1274,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..2c31b7a 100644
--- a/hw/virtio-pci.h
+++ b/hw/virtio-pci.h
@@ -46,6 +46,17 @@ 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)
+
 struct VirtIOPCIProxy {
     PCIDevice pci_dev;
     VirtIODevice *vdev;
@@ -66,6 +77,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 v5 5/6] 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 | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio.h | 29 +++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+)

diff --git a/hw/virtio.c b/hw/virtio.c
index f40a8c5..cd46af1 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.
@@ -934,6 +935,38 @@ VirtIODevice *virtio_common_init(const char *name, uint16_t device_id,
     return vdev;
 }
 
+/*
+ * The same initialization as above without allocating the structure.
+ */
+void virtio_common_init_(VirtIODevice *vdev, const char *name,
+                         uint16_t device_id, size_t config_size,
+                         size_t struct_size)
+{
+    int i;
+
+    vdev->device_id = device_id;
+    vdev->status = 0;
+    vdev->isr = 0;
+    vdev->queue_sel = 0;
+    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++) {
+        vdev->vq[i].vector = VIRTIO_NO_VECTOR;
+        vdev->vq[i].vdev = vdev;
+    }
+
+    vdev->name = name;
+    vdev->config_len = config_size;
+    if (vdev->config_len) {
+        vdev->config = g_malloc0(config_size);
+    } else {
+        vdev->config = NULL;
+    }
+    vdev->vmstate = qemu_add_vm_change_state_handler(virtio_vmstate_change,
+                                                     vdev);
+}
+
 void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding,
                         void *opaque)
 {
@@ -1056,3 +1089,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..361ad95 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,22 @@ 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_common_init_(VirtIODevice *vdev, const char *name,
+                         uint16_t device_id, size_t config_size,
+                         size_t struct_size);
+
 VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
                             void (*handle_output)(VirtIODevice *,
                                                   VirtQueue *));
-- 
1.7.11.7

[Qemu-devel] [RFC PATCH v5 6/6] virtio-blk : Refactor virtio-blk.

[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 | 170 ++++++++++++++++++++++++++++++++++++++++++++++++--------
 hw/virtio-blk.h |   4 ++
 2 files changed, 150 insertions(+), 24 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index e25cc96..ee1ea8b 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -21,24 +21,42 @@
 #ifdef __linux__
 # include <scsi/sg.h>
 #endif
+#include "virtio-bus.h"
 
+/* Take this structure as our device structure. */
 typedef struct VirtIOBlock
 {
+    /*
+     * Adding parent_obj breaks to_virtio_blk cast function,
+     * and virtio_blk_init.
+     */
+    DeviceState parent_obj;
+    /*
+     * We don't need that anymore, as we'll use QOM cast to get the
+     * VirtIODevice. Just temporary keep it, for not breaking functionality.
+     */
     VirtIODevice vdev;
     BlockDriverState *bs;
     VirtQueue *vq;
     void *rq;
     QEMUBH *bh;
     BlockConf *conf;
-    VirtIOBlkConf *blk;
+    /*
+     * We can't use pointer with properties.
+     */
+    VirtIOBlkConf blk;
     unsigned short sector_mask;
     DeviceState *qdev;
 } VirtIOBlock;
 
-static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
-{
-    return (VirtIOBlock *)vdev;
-}
+/*
+ * Use the QOM cast, so we don't need that anymore.
+ *
+ * static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
+ * {
+ *     return (VirtIOBlock *)vdev;
+ * }
+ */
 
 typedef struct VirtIOBlockReq
 {
@@ -55,18 +73,20 @@ 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,
     bool is_read)
 {
-    BlockErrorAction action = bdrv_get_error_action(req->dev->bs, is_read, error);
+    BlockErrorAction action = bdrv_get_error_action(req->dev->bs, is_read,
+                                                    error);
     VirtIOBlock *s = req->dev;
 
     if (action == BDRV_ACTION_STOP) {
@@ -164,7 +184,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 +404,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);
@@ -401,7 +421,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,
@@ -422,7 +442,7 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 
 static void virtio_blk_dma_restart_bh(void *opaque)
 {
-    VirtIOBlock *s = opaque;
+    VirtIOBlock *s = VIRTIO_BLK(opaque);
     VirtIOBlockReq *req = s->rq;
     MultiReqBuffer mrb = {
         .num_writes = 0,
@@ -444,7 +464,7 @@ static void virtio_blk_dma_restart_bh(void *opaque)
 static void virtio_blk_dma_restart_cb(void *opaque, int running,
                                       RunState state)
 {
-    VirtIOBlock *s = opaque;
+    VirtIOBlock *s = VIRTIO_BLK(opaque);
 
     if (!running)
         return;
@@ -468,7 +488,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;
@@ -507,7 +527,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));
@@ -516,7 +536,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);
@@ -535,7 +555,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)) {
@@ -548,10 +568,11 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
 
 static void virtio_blk_save(QEMUFile *f, void *opaque)
 {
-    VirtIOBlock *s = opaque;
+    VirtIOBlock *s = VIRTIO_BLK(opaque);
+    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
     VirtIOBlockReq *req = s->rq;
 
-    virtio_save(&s->vdev, f);
+    virtio_save(vdev, f);
     
     while (req) {
         qemu_put_sbyte(f, 1);
@@ -563,13 +584,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(opaque);
     int ret;
 
     if (version_id != 2)
         return -EINVAL;
 
-    ret = virtio_load(&s->vdev, f);
+    ret = virtio_load(vdev, f);
     if (ret) {
         return ret;
     }
@@ -591,9 +613,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 = {
@@ -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;
+    memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf));
     s->rq = NULL;
     s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1;
 
@@ -651,8 +673,108 @@ 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);
+    VirtIOBlock *s = VIRTIO_BLK(qdev);
+
+    VirtIOBlkConf *blk = &(s->blk);
+    static int virtio_blk_id;
+
+    if (!blk->conf.bs) {
+        error_report("drive property not set");
+        return -1;
+    }
+    if (!bdrv_is_inserted(blk->conf.bs)) {
+        error_report("Device needs media, but drive is empty");
+        return -1;
+    }
+
+    blkconf_serial(&blk->conf, &blk->serial);
+    if (blkconf_geometry(&blk->conf, NULL, 65535, 255, 255) < 0) {
+        return -1;
+    }
+
+    virtio_common_init_(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
+                                          sizeof(struct virtio_blk_config),
+                                          sizeof(VirtIOBlock));
+
+    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;
+    memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf));
+    s->rq = NULL;
+    s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1;
+
+    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 = qdev;
+    register_savevm(qdev, "virtio-blk", virtio_blk_id++, 2,
+                    virtio_blk_save, virtio_blk_load, s);
+    bdrv_set_dev_ops(s->bs, &virtio_block_ops, s);
+    bdrv_set_buffer_alignment(s->bs, s->conf->logical_block_size);
+
+    bdrv_iostatus_enable(s->bs);
+    add_boot_device_path(s->conf->bootindex, qdev, "/disk@0,0");
+
+    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..a33336e 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -111,4 +111,8 @@ struct VirtIOBlkConf
         DEFINE_VIRTIO_COMMON_FEATURES(_state, _field), \
         DEFINE_PROP_BIT("config-wce", _state, _field, VIRTIO_BLK_F_CONFIG_WCE, true)
 
+#define TYPE_VIRTIO_BLK "virtio-blk"
+#define VIRTIO_BLK(obj) \
+        OBJECT_CHECK(VirtIOBlock, (obj), TYPE_VIRTIO_BLK)
+
 #endif
-- 
1.7.11.7

Re: [Qemu-devel] [RFC PATCH v5 4/6] virtio-pci : Refactor virtio-pci device.

[Peter Maydell]

On 4 December 2012 14:35,  <fred.konrad@greensocs.com> wrote:
> 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 | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/virtio-pci.h |  14 +++++++
>  2 files changed, 126 insertions(+)
>
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 5ac8d0d..8426122 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -1119,6 +1119,115 @@ static TypeInfo virtio_scsi_info = {
>      .class_init    = virtio_scsi_class_init,
>  };
>
> +/*
> + * virtio-pci : This is the PCIDevice which have a virtio-pci-bus.
> + */
> +
> +/* init callback */
> +static void virtio_pci_init_cb(void *opaque)

> +/* exit callback */
> +static void virtio_pci_exit_cb(void *opaque)

> +static int virtio_pci_init(PCIDevice *pci_dev)

> +static void virtio_pci_exit(PCIDevice *pci_dev)

It's rather confusing to have an init and an init_cb and also
an exit and an exit_cb, and not to have anything explaining
what the difference is or when each one is called or what
needs to be done in one that can't be done in the other.

-- PMM

Re: [Qemu-devel] [RFC PATCH v5 5/6] virtio-device : Refactor virtio-device.

[Peter Maydell]

On 4 December 2012 14:35,  <fred.konrad@greensocs.com> wrote:
> 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 | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/virtio.h | 29 +++++++++++++++++++++++++++++
>  2 files changed, 85 insertions(+)
>
> diff --git a/hw/virtio.c b/hw/virtio.c
> index f40a8c5..cd46af1 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.
> @@ -934,6 +935,38 @@ VirtIODevice *virtio_common_init(const char *name, uint16_t device_id,
>      return vdev;
>  }
>
> +/*
> + * The same initialization as above without allocating the structure.
> + */
> +void virtio_common_init_(VirtIODevice *vdev, const char *name,
> +                         uint16_t device_id, size_t config_size,
> +                         size_t struct_size)

If you find yourself cut-and-pasting 25 lines of code, think again.
In this case, just make virtio_common_init() a wrapper that does
a malloc and calls your non-allocation init.

Also, find a better function name than "just add a random underscore".

-- PMM

Re: [Qemu-devel] [RFC PATCH v5 4/6] virtio-pci : Refactor virtio-pci device.

[KONRAD Frédéric]

On 04/12/2012 15:49, Peter Maydell wrote:
> On 4 December 2012 14:35,  <fred.konrad@greensocs.com> wrote:
>> 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 | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   hw/virtio-pci.h |  14 +++++++
>>   2 files changed, 126 insertions(+)
>>
>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>> index 5ac8d0d..8426122 100644
>> --- a/hw/virtio-pci.c
>> +++ b/hw/virtio-pci.c
>> @@ -1119,6 +1119,115 @@ static TypeInfo virtio_scsi_info = {
>>       .class_init    = virtio_scsi_class_init,
>>   };
>>
>> +/*
>> + * virtio-pci : This is the PCIDevice which have a virtio-pci-bus.
>> + */
>> +
>> +/* init callback */
>> +static void virtio_pci_init_cb(void *opaque)
>> +/* exit callback */
>> +static void virtio_pci_exit_cb(void *opaque)
>> +static int virtio_pci_init(PCIDevice *pci_dev)
>> +static void virtio_pci_exit(PCIDevice *pci_dev)
> It's rather confusing to have an init and an init_cb and also
> an exit and an exit_cb, and not to have anything explaining
> what the difference is or when each one is called or what
> needs to be done in one that can't be done in the other.
Right, I'll change the name and add comments.
>
> -- PMM
>

Re: [Qemu-devel] [RFC PATCH v5 5/6] virtio-device : Refactor virtio-device.

[KONRAD Frédéric]

On 04/12/2012 15:55, Peter Maydell wrote:
> On 4 December 2012 14:35,  <fred.konrad@greensocs.com> wrote:
>> 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 | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   hw/virtio.h | 29 +++++++++++++++++++++++++++++
>>   2 files changed, 85 insertions(+)
>>
>> diff --git a/hw/virtio.c b/hw/virtio.c
>> index f40a8c5..cd46af1 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.
>> @@ -934,6 +935,38 @@ VirtIODevice *virtio_common_init(const char *name, uint16_t device_id,
>>       return vdev;
>>   }
>>
>> +/*
>> + * The same initialization as above without allocating the structure.
>> + */
>> +void virtio_common_init_(VirtIODevice *vdev, const char *name,
>> +                         uint16_t device_id, size_t config_size,
>> +                         size_t struct_size)
> If you find yourself cut-and-pasting 25 lines of code, think again.
> In this case, just make virtio_common_init() a wrapper that does
> a malloc and calls your non-allocation init.
I didn't think about it, I'll do it.

Thanks,

Fred.

Re: [Qemu-devel] [RFC PATCH v5 6/6] virtio-blk : Refactor virtio-blk.

[Peter Maydell]

On 4 December 2012 14:35,  <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.
>
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> ---
>  hw/virtio-blk.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++++--------
>  hw/virtio-blk.h |   4 ++
>  2 files changed, 150 insertions(+), 24 deletions(-)
>
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index e25cc96..ee1ea8b 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -21,24 +21,42 @@
>  #ifdef __linux__
>  # include <scsi/sg.h>
>  #endif
> +#include "virtio-bus.h"
>
> +/* Take this structure as our device structure. */
>  typedef struct VirtIOBlock
>  {
> +    /*
> +     * Adding parent_obj breaks to_virtio_blk cast function,
> +     * and virtio_blk_init.
> +     */
> +    DeviceState parent_obj;
> +    /*
> +     * We don't need that anymore, as we'll use QOM cast to get the
> +     * VirtIODevice. Just temporary keep it, for not breaking functionality.
> +     */
>      VirtIODevice vdev;

This doesn't make sense. After your previous patch, VirtIODevice
is-a DeviceState, and VirtIOBlock already is-a VirtIODevice,
so there's nothing to do here. Adding this parent_obj field
here is just breaking things (it would make the VirtIOBlock
into a direct child of DeviceState, which isn't what we want).

>      BlockDriverState *bs;
>      VirtQueue *vq;
>      void *rq;
>      QEMUBH *bh;
>      BlockConf *conf;
> -    VirtIOBlkConf *blk;
> +    /*
> +     * We can't use pointer with properties.
> +     */
> +    VirtIOBlkConf blk;
>      unsigned short sector_mask;
>      DeviceState *qdev;
>  } VirtIOBlock;
>
> -static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
> -{
> -    return (VirtIOBlock *)vdev;
> -}
> +/*
> + * Use the QOM cast, so we don't need that anymore.
> + *
> + * static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
> + * {
> + *     return (VirtIOBlock *)vdev;
> + * }
> + */

If we don't need it, just delete it.

-- PMM

Re: [Qemu-devel] [RFC PATCH v5 6/6] virtio-blk : Refactor virtio-blk.

[Andreas Färber]

Am 05.12.2012 17:25, schrieb Peter Maydell:
> On 4 December 2012 14:35,  <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.
>>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> ---
>>  hw/virtio-blk.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++++--------
>>  hw/virtio-blk.h |   4 ++
>>  2 files changed, 150 insertions(+), 24 deletions(-)
>>
>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
>> index e25cc96..ee1ea8b 100644
>> --- a/hw/virtio-blk.c
>> +++ b/hw/virtio-blk.c
>> @@ -21,24 +21,42 @@
>>  #ifdef __linux__
>>  # include <scsi/sg.h>
>>  #endif
>> +#include "virtio-bus.h"
>>
>> +/* Take this structure as our device structure. */
>>  typedef struct VirtIOBlock
>>  {
>> +    /*
>> +     * Adding parent_obj breaks to_virtio_blk cast function,
>> +     * and virtio_blk_init.
>> +     */
>> +    DeviceState parent_obj;
>> +    /*
>> +     * We don't need that anymore, as we'll use QOM cast to get the
>> +     * VirtIODevice. Just temporary keep it, for not breaking functionality.
>> +     */
>>      VirtIODevice vdev;
> 
> This doesn't make sense. After your previous patch, VirtIODevice
> is-a DeviceState, and VirtIOBlock already is-a VirtIODevice,
> so there's nothing to do here. Adding this parent_obj field
> here is just breaking things (it would make the VirtIOBlock
> into a direct child of DeviceState, which isn't what we want).
> 
>>      BlockDriverState *bs;
>>      VirtQueue *vq;
>>      void *rq;
>>      QEMUBH *bh;
>>      BlockConf *conf;
>> -    VirtIOBlkConf *blk;
>> +    /*
>> +     * We can't use pointer with properties.
>> +     */
>> +    VirtIOBlkConf blk;
>>      unsigned short sector_mask;
>>      DeviceState *qdev;
>>  } VirtIOBlock;
>>
>> -static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>> -{
>> -    return (VirtIOBlock *)vdev;
>> -}
>> +/*
>> + * Use the QOM cast, so we don't need that anymore.
>> + *
>> + * static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>> + * {
>> + *     return (VirtIOBlock *)vdev;
>> + * }
>> + */
> 
> If we don't need it, just delete it.

Seconded. You need to introduce a VIRTIO_BLOCK() macro, backed by
OBJECT_CHECK(), and replace all callers of to_virtio_blk() with
VIRTIO_BLOCK(). Compare my ISA series that I intentionally cc'ed you on.
You can then rename vdev to parent_obj as a check that you caught all users.

Regards,
Andreas

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

Re: [Qemu-devel] [RFC PATCH v5 6/6] virtio-blk : Refactor virtio-blk.

[KONRAD Frédéric]

On 05/12/2012 17:25, Peter Maydell wrote:
> On 4 December 2012 14:35,  <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.
>>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> ---
>>   hw/virtio-blk.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++++--------
>>   hw/virtio-blk.h |   4 ++
>>   2 files changed, 150 insertions(+), 24 deletions(-)
>>
>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
>> index e25cc96..ee1ea8b 100644
>> --- a/hw/virtio-blk.c
>> +++ b/hw/virtio-blk.c
>> @@ -21,24 +21,42 @@
>>   #ifdef __linux__
>>   # include <scsi/sg.h>
>>   #endif
>> +#include "virtio-bus.h"
>>
>> +/* Take this structure as our device structure. */
>>   typedef struct VirtIOBlock
>>   {
>> +    /*
>> +     * Adding parent_obj breaks to_virtio_blk cast function,
>> +     * and virtio_blk_init.
>> +     */
>> +    DeviceState parent_obj;
>> +    /*
>> +     * We don't need that anymore, as we'll use QOM cast to get the
>> +     * VirtIODevice. Just temporary keep it, for not breaking functionality.
>> +     */
>>       VirtIODevice vdev;
> This doesn't make sense. After your previous patch, VirtIODevice
> is-a DeviceState, and VirtIOBlock already is-a VirtIODevice,
> so there's nothing to do here. Adding this parent_obj field
> here is just breaking things (it would make the VirtIOBlock
> into a direct child of DeviceState, which isn't what we want).
>
Ok, I think you're right, I'll check.

>>       BlockDriverState *bs;
>>       VirtQueue *vq;
>>       void *rq;
>>       QEMUBH *bh;
>>       BlockConf *conf;
>> -    VirtIOBlkConf *blk;
>> +    /*
>> +     * We can't use pointer with properties.
>> +     */
>> +    VirtIOBlkConf blk;
>>       unsigned short sector_mask;
>>       DeviceState *qdev;
>>   } VirtIOBlock;
>>
>> -static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>> -{
>> -    return (VirtIOBlock *)vdev;
>> -}
>> +/*
>> + * Use the QOM cast, so we don't need that anymore.
>> + *
>> + * static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>> + * {
>> + *     return (VirtIOBlock *)vdev;
>> + * }
>> + */
> If we don't need it, just delete it.
>
> -- PMM
Yes, sure, I put it in comment to explain, why I deleted it.

Thanks,

Fred.

Re: [Qemu-devel] [RFC PATCH v5 6/6] virtio-blk : Refactor virtio-blk.

[Andreas Färber]

Am 06.12.2012 10:11, schrieb KONRAD Frédéric:
> On 05/12/2012 17:25, Peter Maydell wrote:
>> On 4 December 2012 14:35,  <fred.konrad@greensocs.com> wrote:
>>> -static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>>> -{
>>> -    return (VirtIOBlock *)vdev;
>>> -}
>>> +/*
>>> + * Use the QOM cast, so we don't need that anymore.
>>> + *
>>> + * static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>>> + * {
>>> + *     return (VirtIOBlock *)vdev;
>>> + * }
>>> + */
>> If we don't need it, just delete it.
>>
>> -- PMM
> Yes, sure, I put it in comment to explain, why I deleted it.

Please don't comment out unneeded things. Instead, remove them
completely and put the explanation into the commit message. That helps
keep the patch small and avoids commented-out code bitrotting.

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 v5 6/6] virtio-blk : Refactor virtio-blk.

[KONRAD Frédéric]

On 05/12/2012 18:22, Andreas Färber wrote:
> Am 05.12.2012 17:25, schrieb Peter Maydell:
>> On 4 December 2012 14:35,  <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.
>>>
>>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>>> ---
>>>   hw/virtio-blk.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++++--------
>>>   hw/virtio-blk.h |   4 ++
>>>   2 files changed, 150 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
>>> index e25cc96..ee1ea8b 100644
>>> --- a/hw/virtio-blk.c
>>> +++ b/hw/virtio-blk.c
>>> @@ -21,24 +21,42 @@
>>>   #ifdef __linux__
>>>   # include <scsi/sg.h>
>>>   #endif
>>> +#include "virtio-bus.h"
>>>
>>> +/* Take this structure as our device structure. */
>>>   typedef struct VirtIOBlock
>>>   {
>>> +    /*
>>> +     * Adding parent_obj breaks to_virtio_blk cast function,
>>> +     * and virtio_blk_init.
>>> +     */
>>> +    DeviceState parent_obj;
>>> +    /*
>>> +     * We don't need that anymore, as we'll use QOM cast to get the
>>> +     * VirtIODevice. Just temporary keep it, for not breaking functionality.
>>> +     */
>>>       VirtIODevice vdev;
>> This doesn't make sense. After your previous patch, VirtIODevice
>> is-a DeviceState, and VirtIOBlock already is-a VirtIODevice,
>> so there's nothing to do here. Adding this parent_obj field
>> here is just breaking things (it would make the VirtIOBlock
>> into a direct child of DeviceState, which isn't what we want).
>>
>>>       BlockDriverState *bs;
>>>       VirtQueue *vq;
>>>       void *rq;
>>>       QEMUBH *bh;
>>>       BlockConf *conf;
>>> -    VirtIOBlkConf *blk;
>>> +    /*
>>> +     * We can't use pointer with properties.
>>> +     */
>>> +    VirtIOBlkConf blk;
>>>       unsigned short sector_mask;
>>>       DeviceState *qdev;
>>>   } VirtIOBlock;
>>>
>>> -static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>>> -{
>>> -    return (VirtIOBlock *)vdev;
>>> -}
>>> +/*
>>> + * Use the QOM cast, so we don't need that anymore.
>>> + *
>>> + * static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>>> + * {
>>> + *     return (VirtIOBlock *)vdev;
>>> + * }
>>> + */
>> If we don't need it, just delete it.
> Seconded. You need to introduce a VIRTIO_BLOCK() macro, backed by
> OBJECT_CHECK(), and replace all callers of to_virtio_blk() with
> VIRTIO_BLOCK(). Compare my ISA series that I intentionally cc'ed you on.
Yes, that's why I comment to_virtio_blk().

Isn't what I made in this patch with :

+#define TYPE_VIRTIO_BLK "virtio-blk"
+#define VIRTIO_BLK(obj) \
+        OBJECT_CHECK(VirtIOBlock, (obj), TYPE_VIRTIO_BLK)
+

and

-    VirtIOBlock *s = to_virtio_blk(vdev);
+    VirtIOBlock *s = VIRTIO_BLK(vdev);

?


I agree with that, but, there is an issue :
The refactored VirtIOBlk is a device and seems to work, but the device 
which use this VirtIOBlock
(eg virtio-blk-pci) are just allocating a structure ( in 
virtio_common_init ).

That's why this patch is breaking virtio-blk-pci.
>
> Regards,
> Andreas
>

Thanks,

Fred.

Re: [Qemu-devel] [RFC PATCH v5 6/6] virtio-blk : Refactor virtio-blk.

[KONRAD Frédéric]

On 06/12/2012 10:18, Andreas Färber wrote:
> Am 06.12.2012 10:11, schrieb KONRAD Frédéric:
>> On 05/12/2012 17:25, Peter Maydell wrote:
>>> On 4 December 2012 14:35,  <fred.konrad@greensocs.com> wrote:
>>>> -static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>>>> -{
>>>> -    return (VirtIOBlock *)vdev;
>>>> -}
>>>> +/*
>>>> + * Use the QOM cast, so we don't need that anymore.
>>>> + *
>>>> + * static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>>>> + * {
>>>> + *     return (VirtIOBlock *)vdev;
>>>> + * }
>>>> + */
>>> If we don't need it, just delete it.
>>>
>>> -- PMM
>> Yes, sure, I put it in comment to explain, why I deleted it.
> Please don't comment out unneeded things. Instead, remove them
> completely and put the explanation into the commit message. That helps
> keep the patch small and avoids commented-out code bitrotting.
>
> Andreas
>
Ok, no problem.

Re: [Qemu-devel] [RFC PATCH v5 6/6] virtio-blk : Refactor virtio-blk.

[Andreas Färber]

Am 06.12.2012 10:21, schrieb KONRAD Frédéric:
> On 05/12/2012 18:22, Andreas Färber wrote:
>> Am 05.12.2012 17:25, schrieb Peter Maydell:
>>> On 4 December 2012 14:35,  <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.
>>>>
>>>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>>>> ---
>>>>   hw/virtio-blk.c | 170
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++--------
>>>>   hw/virtio-blk.h |   4 ++
>>>>   2 files changed, 150 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
>>>> index e25cc96..ee1ea8b 100644
>>>> --- a/hw/virtio-blk.c
>>>> +++ b/hw/virtio-blk.c
>>>> @@ -21,24 +21,42 @@
>>>>   #ifdef __linux__
>>>>   # include <scsi/sg.h>
>>>>   #endif
>>>> +#include "virtio-bus.h"
>>>>
>>>> +/* Take this structure as our device structure. */
>>>>   typedef struct VirtIOBlock
>>>>   {
>>>> +    /*
>>>> +     * Adding parent_obj breaks to_virtio_blk cast function,
>>>> +     * and virtio_blk_init.
>>>> +     */
>>>> +    DeviceState parent_obj;
>>>> +    /*
>>>> +     * We don't need that anymore, as we'll use QOM cast to get the
>>>> +     * VirtIODevice. Just temporary keep it, for not breaking
>>>> functionality.
>>>> +     */
>>>>       VirtIODevice vdev;
>>> This doesn't make sense. After your previous patch, VirtIODevice
>>> is-a DeviceState, and VirtIOBlock already is-a VirtIODevice,
>>> so there's nothing to do here. Adding this parent_obj field
>>> here is just breaking things (it would make the VirtIOBlock
>>> into a direct child of DeviceState, which isn't what we want).
>>>
>>>>       BlockDriverState *bs;
>>>>       VirtQueue *vq;
>>>>       void *rq;
>>>>       QEMUBH *bh;
>>>>       BlockConf *conf;
>>>> -    VirtIOBlkConf *blk;
>>>> +    /*
>>>> +     * We can't use pointer with properties.
>>>> +     */
>>>> +    VirtIOBlkConf blk;
>>>>       unsigned short sector_mask;
>>>>       DeviceState *qdev;
>>>>   } VirtIOBlock;
>>>>
>>>> -static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>>>> -{
>>>> -    return (VirtIOBlock *)vdev;
>>>> -}
>>>> +/*
>>>> + * Use the QOM cast, so we don't need that anymore.
>>>> + *
>>>> + * static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>>>> + * {
>>>> + *     return (VirtIOBlock *)vdev;
>>>> + * }
>>>> + */
>>> If we don't need it, just delete it.
>> Seconded. You need to introduce a VIRTIO_BLOCK() macro, backed by
>> OBJECT_CHECK(), and replace all callers of to_virtio_blk() with
>> VIRTIO_BLOCK(). Compare my ISA series that I intentionally cc'ed you on.
> Yes, that's why I comment to_virtio_blk().
> 
> Isn't what I made in this patch with :
> 
> +#define TYPE_VIRTIO_BLK "virtio-blk"
> +#define VIRTIO_BLK(obj) \
> +        OBJECT_CHECK(VirtIOBlock, (obj), TYPE_VIRTIO_BLK)
> +
> 
> and
> 
> -    VirtIOBlock *s = to_virtio_blk(vdev);
> +    VirtIOBlock *s = VIRTIO_BLK(vdev);
> 
> ?

Sorry. I expected to see the macros above the typedef above, but in the
header is even better! :) VIRTIO_BLOCK vs. VIRTIO_BLK is just a style
question.

Further, I missed on brief sight that the to_* function was commented
out, thought it was still being used. Didn't find enough time to review
the series fully yet.

> I agree with that, but, there is an issue :
> The refactored VirtIOBlk is a device and seems to work, but the device
> which use this VirtIOBlock
> (eg virtio-blk-pci) are just allocating a structure ( in
> virtio_common_init ).
> 
> That's why this patch is breaking virtio-blk-pci.

Don't understand that part due to lack of virtio knowledge...
Patch 5/6 introduces VirtIODevice as sitting on TYPE_VIRTIO_BUS. So with
this patch VirtIOBlk is moving to that new bus and virtio-blk-pci should
only be necessary as a command line option alias for backwards
compatibility, no? Are you saying you can't make this switch and
refactoring for virtio-blk *only*?

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 v5 6/6] virtio-blk : Refactor virtio-blk.

[KONRAD Frédéric]

On 06/12/2012 10:53, Andreas Färber wrote:
> Am 06.12.2012 10:21, schrieb KONRAD Frédéric:
>> On 05/12/2012 18:22, Andreas Färber wrote:
>>> Am 05.12.2012 17:25, schrieb Peter Maydell:
>>>> On 4 December 2012 14:35,  <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.
>>>>>
>>>>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>>>>> ---
>>>>>    hw/virtio-blk.c | 170
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++--------
>>>>>    hw/virtio-blk.h |   4 ++
>>>>>    2 files changed, 150 insertions(+), 24 deletions(-)
>>>>>
>>>>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
>>>>> index e25cc96..ee1ea8b 100644
>>>>> --- a/hw/virtio-blk.c
>>>>> +++ b/hw/virtio-blk.c
>>>>> @@ -21,24 +21,42 @@
>>>>>    #ifdef __linux__
>>>>>    # include <scsi/sg.h>
>>>>>    #endif
>>>>> +#include "virtio-bus.h"
>>>>>
>>>>> +/* Take this structure as our device structure. */
>>>>>    typedef struct VirtIOBlock
>>>>>    {
>>>>> +    /*
>>>>> +     * Adding parent_obj breaks to_virtio_blk cast function,
>>>>> +     * and virtio_blk_init.
>>>>> +     */
>>>>> +    DeviceState parent_obj;
>>>>> +    /*
>>>>> +     * We don't need that anymore, as we'll use QOM cast to get the
>>>>> +     * VirtIODevice. Just temporary keep it, for not breaking
>>>>> functionality.
>>>>> +     */
>>>>>        VirtIODevice vdev;
>>>> This doesn't make sense. After your previous patch, VirtIODevice
>>>> is-a DeviceState, and VirtIOBlock already is-a VirtIODevice,
>>>> so there's nothing to do here. Adding this parent_obj field
>>>> here is just breaking things (it would make the VirtIOBlock
>>>> into a direct child of DeviceState, which isn't what we want).
>>>>
>>>>>        BlockDriverState *bs;
>>>>>        VirtQueue *vq;
>>>>>        void *rq;
>>>>>        QEMUBH *bh;
>>>>>        BlockConf *conf;
>>>>> -    VirtIOBlkConf *blk;
>>>>> +    /*
>>>>> +     * We can't use pointer with properties.
>>>>> +     */
>>>>> +    VirtIOBlkConf blk;
>>>>>        unsigned short sector_mask;
>>>>>        DeviceState *qdev;
>>>>>    } VirtIOBlock;
>>>>>
>>>>> -static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>>>>> -{
>>>>> -    return (VirtIOBlock *)vdev;
>>>>> -}
>>>>> +/*
>>>>> + * Use the QOM cast, so we don't need that anymore.
>>>>> + *
>>>>> + * static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>>>>> + * {
>>>>> + *     return (VirtIOBlock *)vdev;
>>>>> + * }
>>>>> + */
>>>> If we don't need it, just delete it.
>>> Seconded. You need to introduce a VIRTIO_BLOCK() macro, backed by
>>> OBJECT_CHECK(), and replace all callers of to_virtio_blk() with
>>> VIRTIO_BLOCK(). Compare my ISA series that I intentionally cc'ed you on.
>> Yes, that's why I comment to_virtio_blk().
>>
>> Isn't what I made in this patch with :
>>
>> +#define TYPE_VIRTIO_BLK "virtio-blk"
>> +#define VIRTIO_BLK(obj) \
>> +        OBJECT_CHECK(VirtIOBlock, (obj), TYPE_VIRTIO_BLK)
>> +
>>
>> and
>>
>> -    VirtIOBlock *s = to_virtio_blk(vdev);
>> +    VirtIOBlock *s = VIRTIO_BLK(vdev);
>>
>> ?
> Sorry. I expected to see the macros above the typedef above, but in the
> header is even better! :) VIRTIO_BLOCK vs. VIRTIO_BLK is just a style
> question.
>
> Further, I missed on brief sight that the to_* function was commented
> out, thought it was still being used. Didn't find enough time to review
> the series fully yet.
Sorry for that, I'll remove the comment.
>
>> I agree with that, but, there is an issue :
>> The refactored VirtIOBlk is a device and seems to work, but the device
>> which use this VirtIOBlock
>> (eg virtio-blk-pci) are just allocating a structure ( in
>> virtio_common_init ).
>>
>> That's why this patch is breaking virtio-blk-pci.
> Don't understand that part due to lack of virtio knowledge...
> Patch 5/6 introduces VirtIODevice as sitting on TYPE_VIRTIO_BUS. So with
> this patch VirtIOBlk is moving to that new bus and virtio-blk-pci should
> only be necessary as a command line option alias for backwards
> compatibility, no? Are you saying you can't make this switch and
> refactoring for virtio-blk *only*?
I mean that all device which use virtio_blk_init will be broken as we 
use the
OBJECT_CHECK cast and virtio_blk_init currently call virtio_common_init
which allocate a VirtIODevice structure ( and don't create any device ).

The other virtio-* devices shouldn't be affected. I must test.

You suggest an alias for backwards compatibility ? It must create a 
virtio-pci and
a virtio-blk. Is it possible ?

Fred
>
> Andreas
>

Re: [Qemu-devel] [RFC PATCH v5 6/6] virtio-blk : Refactor virtio-blk.

[Peter Maydell]

On 6 December 2012 09:53, Andreas Färber <afaerber@suse.de> wrote:
> Am 06.12.2012 10:21, schrieb KONRAD Frédéric:
>> I agree with that, but, there is an issue :
>> The refactored VirtIOBlk is a device and seems to work, but the device
>> which use this VirtIOBlock
>> (eg virtio-blk-pci) are just allocating a structure ( in
>> virtio_common_init ).
>>
>> That's why this patch is breaking virtio-blk-pci.
>
> Don't understand that part due to lack of virtio knowledge...
> Patch 5/6 introduces VirtIODevice as sitting on TYPE_VIRTIO_BUS. So with
> this patch VirtIOBlk is moving to that new bus and virtio-blk-pci should
> only be necessary as a command line option alias for backwards
> compatibility, no?

It can't just be a command line alias, or we will break migration.
It has to be a simple device that composes together the virtio-pci
and virtio-blk devices, plus legacy support for properties and
migration state, I think.

-- PMM

Re: [Qemu-devel] [RFC PATCH v5 6/6] virtio-blk : Refactor virtio-blk.

[KONRAD Frédéric]

On 06/12/2012 11:13, Peter Maydell wrote:
> On 6 December 2012 09:53, Andreas Färber <afaerber@suse.de> wrote:
>> Am 06.12.2012 10:21, schrieb KONRAD Frédéric:
>>> I agree with that, but, there is an issue :
>>> The refactored VirtIOBlk is a device and seems to work, but the device
>>> which use this VirtIOBlock
>>> (eg virtio-blk-pci) are just allocating a structure ( in
>>> virtio_common_init ).
>>>
>>> That's why this patch is breaking virtio-blk-pci.
>> Don't understand that part due to lack of virtio knowledge...
>> Patch 5/6 introduces VirtIODevice as sitting on TYPE_VIRTIO_BUS. So with
>> this patch VirtIOBlk is moving to that new bus and virtio-blk-pci should
>> only be necessary as a command line option alias for backwards
>> compatibility, no?
> It can't just be a command line alias, or we will break migration.
> It has to be a simple device that composes together the virtio-pci
> and virtio-blk devices, plus legacy support for properties and
> migration state, I think.
>
> -- PMM
Can we do virtio-blk refactoring and virtio-blk-pci at the same time for not
breaking anything ?

Or do you have a better idea ?

Fred

Re: [Qemu-devel] [RFC PATCH v5 6/6] virtio-blk : Refactor virtio-blk.

[Peter Maydell]

On 6 December 2012 13:58, KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
> On 06/12/2012 11:13, Peter Maydell wrote:
>> It can't just be a command line alias, or we will break migration.
>> It has to be a simple device that composes together the virtio-pci
>> and virtio-blk devices, plus legacy support for properties and
>> migration state, I think.

> Can we do virtio-blk refactoring and virtio-blk-pci at the same time for not
> breaking anything ?

Not breaking things is a key part of the requirements here.
It's ok to say "I haven't converted virtio-net or the s390
transport in this patchset and therefore they are broken" as
an initial RFC (because we can look at how PCI/blk is done
and check it works before we expand the same thing out to
other transports/devices).  But you need to show how the
virtio-blk / virtio-pci refactoring works and leaves you with
a virtio-blk-pci that isn't broken (either at the end or at
any step along the way).

-- PMM

Re: [Qemu-devel] [RFC PATCH v5 6/6] virtio-blk : Refactor virtio-blk.

[KONRAD Frédéric]

On 06/12/2012 15:21, Peter Maydell wrote:
> On 6 December 2012 13:58, KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
>> On 06/12/2012 11:13, Peter Maydell wrote:
>>> It can't just be a command line alias, or we will break migration.
>>> It has to be a simple device that composes together the virtio-pci
>>> and virtio-blk devices, plus legacy support for properties and
>>> migration state, I think.
>> Can we do virtio-blk refactoring and virtio-blk-pci at the same time for not
>> breaking anything ?
> Not breaking things is a key part of the requirements here.
Agree with that.

> It's ok to say "I haven't converted virtio-net or the s390
> transport in this patchset and therefore they are broken" as
> an initial RFC (because we can look at how PCI/blk is done
> and check it works before we expand the same thing out to
> other transports/devices).  But you need to show how the
> virtio-blk / virtio-pci refactoring works and leaves you with
> a virtio-blk-pci that isn't broken (either at the end or at
> any step along the way).
And if virtio-blk-pci is broken can we refactor it in the same "patch" ?

>
> -- PMM
>