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

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

[fred.konrad]

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

I send this RFC, to know if you're all happy with the current structure of the
device (QOM).

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 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 : Introduce virtio-pci device.
  virtio-device : Introduce virtio-device.
  virtio-blk : Refactoring virtio-blk.

 hw/Makefile.objs  |    1 +
 hw/qdev-core.h    |    2 +
 hw/qdev-monitor.c |   11 +++++
 hw/virtio-blk.c   |   61 ++++++++++++++++++++++++++++
 hw/virtio-blk.h   |   16 +++++++
 hw/virtio-bus.c   |  114 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-bus.h   |   78 ++++++++++++++++++++++++++++++++++++
 hw/virtio-pci.c   |   91 ++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-pci.h   |   34 +++++++++++++++-
 hw/virtio.c       |   28 +++++++++++++
 hw/virtio.h       |   27 ++++++++++++
 qerror.h          |    3 +
 12 files changed, 464 insertions(+), 2 deletions(-)
 create mode 100644 hw/virtio-bus.c
 create mode 100644 hw/virtio-bus.h

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

[fred.konrad]

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

Only one device can be connected to virtio-bus.
This patch add a field max_dev which is :
    * the maximum amount of devices connected on the bus ( when
    * max_dev!=0 ).
    * have no effect ( when max_dev=0 ).

The function qbus_find_recursive is modified :
    * to return a non full bus when the "bus=" option is not present.
    * to give an error when the "bus=" option is pointing a full bus.

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

diff --git a/hw/qdev-core.h b/hw/qdev-core.h
index fff7f0f..f5019b1 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. */
+    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..4471b3a 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;
     }
+    /* Check if max_dev is reached */
+    if ((bus->max_dev != 0) && (bus->max_dev <= bus->max_index)) {
+        if (name != NULL) {
+            /* bus was explicitly specified : return an error. */
+            qerror_report(QERR_BUS_IS_FULL, bus->name);
+            return NULL;
+        } else {
+            /* bus was not specified : try to find another one. */
+            match = 0;
+        }
+    }
     if (match) {
         return bus;
     }
diff --git a/qerror.h b/qerror.h
index 8db4309..b159828 100644
--- a/qerror.h
+++ b/qerror.h
@@ -69,6 +69,9 @@ void assert_no_error(Error *err);
 #define QERR_BUS_NOT_FOUND \
     ERROR_CLASS_GENERIC_ERROR, "Bus '%s' not found"
 
+#define QERR_BUS_IS_FULL \
+    ERROR_CLASS_GENERIC_ERROR, "Bus '%s' is full"
+
 #define QERR_COMMAND_DISABLED \
     ERROR_CLASS_GENERIC_ERROR, "The command %s has been disabled for this instance"
 
-- 
1.7.1

[Qemu-devel] [RFC PATCH V4 2/6] virtio-bus : Introduce virtio-bus

[fred.konrad]

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

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 hw/Makefile.objs |    1 +
 hw/virtio-bus.c  |  114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-bus.h  |   78 +++++++++++++++++++++++++++++++++++++
 3 files changed, 193 insertions(+), 0 deletions(-)
 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..ed9c146
--- /dev/null
+++ b/hw/virtio-bus.c
@@ -0,0 +1,114 @@
+/*
+ * 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
+
+/* Is the bus used ? */
+static inline int virtio_bus_in_use(VirtioBusState *bus)
+{
+    return (bus->vdev != NULL);
+}
+
+/* Plug the VirtIODevice */
+int virtio_bus_plug_device(VirtioBusState *bus, VirtIODevice *vdev)
+{
+    BusState *qbus = BUS(bus);
+    VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus);
+    DPRINTF("plug device into %s.\n", qbus->name);
+    if (virtio_bus_in_use(bus)) {
+        /*
+         * that couldn't be reached if we modify qdev-monitor.c
+         * ( see other patch ).
+         */
+        error_report("%s in use.\n", qbus->name);
+        return -1;
+    }
+
+    /* keep the VirtIODevice in the VirtioBus. */
+    bus->vdev = vdev;
+
+    /* call the "transport" callback. */
+    if (klass->init != NULL) {
+        klass->init(qbus->parent);
+    }
+    return 0;
+}
+
+/* Bind the VirtIODevice to the VirtioBus. */
+void virtio_bus_bind_device(VirtioBusState *bus)
+{
+    BusState *qbus = BUS(bus);
+    VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus);
+    assert(bus != NULL);
+    assert(bus->vdev != NULL);
+    /*
+     * The lines below will disappear, as we will refactor virtio-device.
+     */
+    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 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..90ae67c
--- /dev/null
+++ b/hw/virtio-bus.h
@@ -0,0 +1,78 @@
+/*
+ * 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;
+    /* Old VirtioBindings */
+    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;
+};
+
+void virtio_bus_bind_device(VirtioBusState *bus);
+int virtio_bus_plug_device(VirtioBusState *bus, VirtIODevice *vdev);
+uint16_t get_virtio_device_id(VirtioBusState *bus);
+
+#endif /* VIRTIO_BUS_H */
-- 
1.7.1

[Qemu-devel] [RFC PATCH V4 3/6] virtio-pci-bus : Introduce virtio-pci-bus.

[fred.konrad]

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

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

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 71f4fb5..a73704d 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,48 @@ 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)
+{
+    BusClass *bc = BUS_CLASS(klass);
+    VirtioBusClass *k = VIRTIO_BUS_CLASS(klass);
+    /* VirtIOBindings */
+    k->notify = virtio_pci_notify;
+    k->save_config = virtio_pci_save_config;
+    k->load_config = virtio_pci_load_config;
+    k->save_queue = virtio_pci_save_queue;
+    k->load_queue = virtio_pci_load_queue;
+    k->get_features = virtio_pci_get_features;
+    k->query_guest_notifiers = virtio_pci_query_guest_notifiers;
+    k->set_host_notifier = virtio_pci_set_host_notifier;
+    k->set_guest_notifiers = virtio_pci_set_guest_notifiers;
+    k->vmstate_change = virtio_pci_vmstate_change;
+    /*
+     * TODO : Init and exit function.
+     * void (*init)(void *opaque);
+     * void (*exit)(void *opaque);
+     */
+}
+
+static TypeInfo virtio_pci_bus_info = {
+    .name          = TYPE_VIRTIO_PCI_BUS,
+    .parent        = TYPE_VIRTIO_BUS,
+    .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 +1169,8 @@ 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_register_static(&virtio_pci_info);
 }
 
 type_init(virtio_pci_register_types)
diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h
index b58d9a2..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.1

[Qemu-devel] [RFC PATCH V4 4/6] virtio-pci : Introduce virtio-pci device.

[fred.konrad]

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

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

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index a73704d..1fb1905 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -1119,6 +1119,52 @@ static TypeInfo virtio_scsi_info = {
     .class_init    = virtio_scsi_class_init,
 };
 
+/*
+ * virtio-pci : This is the PCIDevice which have a virtio-pci-bus.
+ */
+
+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)
+{
+    VirtIOPCIProxy *dev = VIRTIO_PCI(pci_dev);
+    /*
+     * TODO:
+     * -> Destroy the bus.
+     * -> Destroy the device.
+     *
+     */
+}
+
+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->exit = virtio_scsi_exit_pci;
+    k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
+    k->device_id = PCI_DEVICE_ID_VIRTIO_SCSI;
+    k->revision = 0x00;
+    k->class_id = PCI_CLASS_STORAGE_SCSI;
+    dc->reset = virtio_pci_reset;
+    dc->props = virtio_scsi_properties;*/
+}
+
+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)
@@ -1154,7 +1200,7 @@ static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
      */
 }
 
-static TypeInfo virtio_pci_bus_info = {
+static const TypeInfo virtio_pci_bus_info = {
     .name          = TYPE_VIRTIO_PCI_BUS,
     .parent        = TYPE_VIRTIO_BUS,
     .instance_size = sizeof(VirtioBusState),
diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h
index 0e3288e..62c9198 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,10 @@ 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.1

[Qemu-devel] [RFC PATCH V4 5/6] virtio-device : Introduce virtio-device.

[fred.konrad]

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

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

diff --git a/hw/virtio.c b/hw/virtio.c
index f40a8c5..1c72d17 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.
@@ -1056,3 +1057,30 @@ EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq)
 {
     return &vq->host_notifier;
 }
+
+/*
+ * Refactored VirtioDevice.
+ */
+
+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..b890eb1 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -108,8 +108,22 @@ typedef struct {
 
 #define VIRTIO_NO_VECTOR 0xffff
 
+/*
+ * Refactored VirtioDevice.
+ */
+
+#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)
+
+/* This should be renammed VirtioDeviceState at the end. */
 struct VirtIODevice
 {
+    DeviceState parent_obj;
     const char *name;
     uint8_t status;
     uint8_t isr;
@@ -134,6 +148,18 @@ 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;
+
 VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
                             void (*handle_output)(VirtIODevice *,
                                                   VirtQueue *));
@@ -244,4 +270,5 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                bool set_handler);
 void virtio_queue_notify_vq(VirtQueue *vq);
 void virtio_irq(VirtQueue *vq);
+
 #endif
-- 
1.7.1

[Qemu-devel] [RFC PATCH V4 6/6] virtio-blk : Refactoring virtio-blk.

[fred.konrad]

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

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 hw/virtio-blk.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-blk.h |   16 ++++++++++++++
 2 files changed, 77 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index e25cc96..3547882 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -656,3 +656,64 @@ void virtio_blk_exit(VirtIODevice *vdev)
     blockdev_mark_auto_del(s->bs);
     virtio_cleanup(vdev);
 }
+
+/*
+ * Refactored virtio-blk.
+ */
+
+static int virtio_device_init(DeviceState *qdev)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
+    /*
+     * TODO: device initialization.
+     */
+    return 0;
+}
+
+static void virtio_device_exit(DeviceState *dev)
+{
+    /*
+     * TODO: destroy device.
+     */
+}
+
+static Property virtio_blk_properties[] = {
+    DEFINE_BLOCK_PROPERTIES(VirtioBlkState, blk.conf),
+    DEFINE_BLOCK_CHS_PROPERTIES(VirtioBlkState, blk.conf),
+    DEFINE_PROP_STRING("serial", VirtioBlkState, blk.serial),
+#ifdef __linux__
+    DEFINE_PROP_BIT("scsi", VirtioBlkState, blk.scsi, 0, true),
+#endif
+    DEFINE_PROP_BIT("config-wce", VirtioBlkState, blk.config_wce, 0, true),
+    DEFINE_VIRTIO_BLK_FEATURES(VirtioBlkState, host_features),
+    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;
+    /* VirtioDeviceClass */
+    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(VirtioBlkState),
+    .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..13876e7 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -111,4 +111,20 @@ struct VirtIOBlkConf
         DEFINE_VIRTIO_COMMON_FEATURES(_state, _field), \
         DEFINE_PROP_BIT("config-wce", _state, _field, VIRTIO_BLK_F_CONFIG_WCE, true)
 
+/*
+ * Refactored virtio-blk.
+ */
+
+#define TYPE_VIRTIO_BLK "virtio-blk"
+#define VIRTIO_BLK(obj) \
+        OBJECT_CHECK(VirtioBlkState, (obj), TYPE_VIRTIO_BLK)
+
+typedef struct {
+    DeviceState parent_obj;
+
+    /* virtio-blk specific */
+    VirtIOBlkConf blk;
+    uint32_t host_features;
+} VirtioBlkState;
+
 #endif
-- 
1.7.1

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

[Peter Maydell]

On 30 November 2012 17:12,  <fred.konrad@greensocs.com> wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> Only one device can be connected to virtio-bus.
> This patch add a field max_dev which is :
>     * the maximum amount of devices connected on the bus ( when
>     * max_dev!=0 ).
>     * have no effect ( when max_dev=0 ).
>
> The function qbus_find_recursive is modified :
>     * to return a non full bus when the "bus=" option is not present.
>     * to give an error when the "bus=" option is pointing a full bus.

You don't need to describe the exact details of code changes in
a commit message -- anybody who wants that can use 'git show'.
Commit messages should describe the overall change and rationale,
not the mechanics. So for instance you should talk about the
effects as visible to the QEMU user, not the changes in semantics
of a particular function.

(Also I don't like commit messages that say "this patch", because
they're not patches once they've been applied; but that's just a
personal quirk.)

> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> ---
>  hw/qdev-core.h    |    2 ++
>  hw/qdev-monitor.c |   11 +++++++++++
>  qerror.h          |    3 +++
>  3 files changed, 16 insertions(+), 0 deletions(-)
>
> diff --git a/hw/qdev-core.h b/hw/qdev-core.h
> index fff7f0f..f5019b1 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 if 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..4471b3a 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;
>      }
> +    /* Check if max_dev is reached */

This comment is veering perilously close to "Add one to x" territory.

> +    if ((bus->max_dev != 0) && (bus->max_dev <= bus->max_index)) {
> +        if (name != NULL) {
> +            /* bus was explicitly specified : return an error. */
> +            qerror_report(QERR_BUS_IS_FULL, bus->name);

So I could be wrong here, but I think we aren't defining new
QERR constants any more, but just having format-string style
errors inline. In this case that would be something like:

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

-- PMM

Re: [Qemu-devel] [RFC PATCH V4 2/6] virtio-bus : Introduce virtio-bus

[Peter Maydell]

On 30 November 2012 17:12,  <fred.konrad@greensocs.com> wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> ---
>  hw/Makefile.objs |    1 +
>  hw/virtio-bus.c  |  114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/virtio-bus.h  |   78 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 193 insertions(+), 0 deletions(-)
>  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..ed9c146
> --- /dev/null
> +++ b/hw/virtio-bus.c
> @@ -0,0 +1,114 @@
> +/*
> + * 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

Don't enable debug by default.

> +
> +#ifdef DEBUG_VIRTIO_BUS
> +#define DPRINTF(fmt, ...) \
> +do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) do { } while (0)
> +#endif
> +
> +/* Is the bus used ? */
> +static inline int virtio_bus_in_use(VirtioBusState *bus)
> +{
> +    return (bus->vdev != NULL);
> +}
> +
> +/* Plug the VirtIODevice */
> +int virtio_bus_plug_device(VirtioBusState *bus, VirtIODevice *vdev)
> +{
> +    BusState *qbus = BUS(bus);
> +    VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus);
> +    DPRINTF("plug device into %s.\n", qbus->name);
> +    if (virtio_bus_in_use(bus)) {
> +        /*
> +         * that couldn't be reached if we modify qdev-monitor.c
> +         * ( see other patch ).
> +         */
> +        error_report("%s in use.\n", qbus->name);
> +        return -1;

You can assume that patch 1 in your series has been applied
here: just assert(bus->vdev == NULL) (and drop the virtio_bus_in_use
function).

> +    }
> +
> +    /* keep the VirtIODevice in the VirtioBus. */

This comment is useless: say something useful, not something which
is obvious from the code.

> +    bus->vdev = vdev;
> +
> +    /* call the "transport" callback. */
> +    if (klass->init != NULL) {
> +        klass->init(qbus->parent);
> +    }

Comment and code disagree about what's happening.

> +    return 0;
> +}
> +
> +/* Bind the VirtIODevice to the VirtioBus. */
> +void virtio_bus_bind_device(VirtioBusState *bus)
> +{
> +    BusState *qbus = BUS(bus);
> +    VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus);
> +    assert(bus != NULL);
> +    assert(bus->vdev != NULL);
> +    /*
> +     * The lines below will disappear, as we will refactor virtio-device.
> +     */
> +    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 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..90ae67c
> --- /dev/null
> +++ b/hw/virtio-bus.h
> @@ -0,0 +1,78 @@
> +/*
> + * 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;
> +    /* Old VirtioBindings */

This is not a particularly useful comment. There's an opportunity
here to provide some doc comments which describe what all these
callbacks a VirtioBus must implement are for.

> +    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;
> +};
> +
> +void virtio_bus_bind_device(VirtioBusState *bus);
> +int virtio_bus_plug_device(VirtioBusState *bus, VirtIODevice *vdev);
> +uint16_t get_virtio_device_id(VirtioBusState *bus);
> +
> +#endif /* VIRTIO_BUS_H */
> --
> 1.7.1
>

-- PMM

Re: [Qemu-devel] [RFC PATCH V4 5/6] virtio-device : Introduce virtio-device.

[Peter Maydell]

On 30 November 2012 17:12,  <fred.konrad@greensocs.com> wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> ---
>  hw/virtio.c |   28 ++++++++++++++++++++++++++++
>  hw/virtio.h |   27 +++++++++++++++++++++++++++
>  2 files changed, 55 insertions(+), 0 deletions(-)
>
> diff --git a/hw/virtio.c b/hw/virtio.c
> index f40a8c5..1c72d17 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.
> @@ -1056,3 +1057,30 @@ EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq)
>  {
>      return &vq->host_notifier;
>  }
> +
> +/*
> + * Refactored VirtioDevice.
> + */
> +
> +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..b890eb1 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -108,8 +108,22 @@ typedef struct {
>
>  #define VIRTIO_NO_VECTOR 0xffff
>
> +/*
> + * Refactored VirtioDevice.

This isn't a useful comment. Something like "Base class for
virtio backends (blk, net, etc)" would be more helpful.

> + */
> +
> +#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)
> +
> +/* This should be renammed VirtioDeviceState at the end. */

"renamed", but does it really need renaming?

>  struct VirtIODevice
>  {
> +    DeviceState parent_obj;
>      const char *name;
>      uint8_t status;
>      uint8_t isr;
> @@ -134,6 +148,18 @@ 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;
> +
>  VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
>                              void (*handle_output)(VirtIODevice *,
>                                                    VirtQueue *));
> @@ -244,4 +270,5 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
>                                                 bool set_handler);
>  void virtio_queue_notify_vq(VirtQueue *vq);
>  void virtio_irq(VirtQueue *vq);
> +

Don't introduce stray blank lines in patches.

>  #endif
> --
> 1.7.1
>

-- PMM

Re: [Qemu-devel] [RFC PATCH V4 6/6] virtio-blk : Refactoring virtio-blk.

[Peter Maydell]

On 30 November 2012 17:12,  <fred.konrad@greensocs.com> wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>

This is a dreadful commit message.

>
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> ---
>  hw/virtio-blk.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/virtio-blk.h |   16 ++++++++++++++
>  2 files changed, 77 insertions(+), 0 deletions(-)
>
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index e25cc96..3547882 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -656,3 +656,64 @@ void virtio_blk_exit(VirtIODevice *vdev)
>      blockdev_mark_auto_del(s->bs);
>      virtio_cleanup(vdev);
>  }
> +
> +/*
> + * Refactored virtio-blk.

...and a not very useful comment.

> + */

-- PMM

Re: [Qemu-devel] [RFC PATCH V4 0/6] Virtio refactoring.

[Peter Maydell]

On 30 November 2012 17:12,  <fred.konrad@greensocs.com> wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> I send this RFC, to know if you're all happy with the current structure of the
> device (QOM).

The general layout of classes looks OK to me, which is why
I've moved on to some more nitpicky stuff. I'll have a closer
look next week and probably add some more comments later.

-- PMM

Re: [Qemu-devel] [RFC PATCH V4 0/6] Virtio refactoring.

[KONRAD Frédéric]

On 30/11/2012 19:43, Peter Maydell wrote:
> On 30 November 2012 17:12,  <fred.konrad@greensocs.com> wrote:
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> I send this RFC, to know if you're all happy with the current structure of the
>> device (QOM).
> The general layout of classes looks OK to me, which is why
> I've moved on to some more nitpicky stuff. I'll have a closer
> look next week and probably add some more comments later.
>
> -- PMM
Ok, so I need to complete this, and refactor virtio-blk
to fit this new structure.

Is this making sense ?

Fred.