* [Qemu-devel] [RFC PATCH 0/3] Virtio refactoring.
@ 2012-11-16 15:35 fred.konrad
2012-11-16 15:35 ` [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus fred.konrad
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: fred.konrad @ 2012-11-16 15:35 UTC (permalink / raw)
To: qemu-devel; +Cc: mark.burton
Hi,
I submit this RFC to be sure I'm doing the right thing about the VirtioBus.
I push the patchset here :
git://git.greensocs.com/qemu_virtio.git virtio_refact
What I proposed to do :
* Introduce a new VirtioBus ( same way as scsi-bus.c ), with VirtIODevice
interface :
-> callback to completely abstract the VirtioDevice from VirtioPCI.
-> for the queue, load/save the queue/config, features, ..., other ?
* Add a VirtioBus to the VirtioPCIProxy. ( virtio-pci.c ) :
-> moving all to the newer callback.
* For each of the virtio-device : ( virtio-x.c )
-> making a separate class for virtio-x which is a VirtioDevice.
-> making a virtio-x-pci which has a virtio-x.
* Create virtio-mmio ( virtio-mmio.c ).
What it is done in this RFC :
* introduce VirtioBus, which can be added to Virtio transports like
virtio-pci or virtio-mmio. I think the interface must be improved.
* add a VirtioBus to the existing VirtioPCIProxy ( virtio-pci ), and add a
"virtio-pci" device which can be loaded without a VirtIODevice, and create
a VirtioBus.
* add a "virtio-blk" device which must be connected on a virtio-bus, to try
the virtio-bus and virtio-pci.
The info qtree return this structure :
bus: main-system-bus
type System
dev: i440FX-pcihost, id ""
bus: pci.0
type PCI
dev: virtio-pci, id ""
bus: virtio.0
type VIRTIO
dev: virtio-blk, id ""
What I must improve :
* the Virtio-PCI still need the VirtIODevice.
* others devices.
* spliting virtio-x-pci to virtio-x + virtio-pci.
Did I miss something ?
Thanks,
Fred.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus.
2012-11-16 15:35 [Qemu-devel] [RFC PATCH 0/3] Virtio refactoring fred.konrad
@ 2012-11-16 15:35 ` fred.konrad
2012-11-19 17:33 ` Peter Maydell
2012-11-21 13:04 ` Andreas Färber
2012-11-16 15:35 ` [Qemu-devel] [RFC PATCH 2/3] virtio-pci : Add a virtio-bus interface fred.konrad
2012-11-16 15:35 ` [Qemu-devel] [RFC PATCH 3/3] virtio-blk : add the virtio-blk device fred.konrad
2 siblings, 2 replies; 15+ messages in thread
From: fred.konrad @ 2012-11-16 15:35 UTC (permalink / raw)
To: qemu-devel; +Cc: mark.burton, KONRAD Frederic
From: KONRAD Frederic <fred.konrad@greensocs.com>
This patch create a new VirtioBus, which can be added to Virtio transports like
virtio-pci, virtio-mmio,...
One VirtIODevice can be connected to this device, like virtio-blk in the 3rd
patch.
The VirtioBus shares through a VirtioBusInfo structure :
* two callbacks with the transport : init_cb and exit_cb, which must be
called by the VirtIODevice, after the initialization and before the
destruction, to put the right PCI IDs and/or stop the event fd.
* a VirtIOBindings structure.
Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
hw/Makefile.objs | 1 +
hw/virtio-bus.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
hw/virtio-bus.h | 49 ++++++++++++++++++++++++
3 files changed, 161 insertions(+)
create mode 100644 hw/virtio-bus.c
create mode 100644 hw/virtio-bus.h
diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index af4ab0c..1b25d77 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -1,6 +1,7 @@
common-obj-y = usb/ ide/
common-obj-y += loader.o
common-obj-$(CONFIG_VIRTIO) += virtio-console.o
+common-obj-$(CONFIG_VIRTIO) += virtio-bus.o
common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
common-obj-y += fw_cfg.o
common-obj-$(CONFIG_PCI) += pci.o pci_bridge.o pci_bridge_dev.o
diff --git a/hw/virtio-bus.c b/hw/virtio-bus.c
new file mode 100644
index 0000000..b2e7e9c
--- /dev/null
+++ b/hw/virtio-bus.c
@@ -0,0 +1,111 @@
+/*
+ * VirtioBus
+ *
+ * Copyright (C) 2012 : GreenSocs Ltd
+ * http://www.greensocs.com/ , email: info@greensocs.com
+ *
+ * Developed by :
+ * Frederic Konrad <fred.konrad@greensocs.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "hw.h"
+#include "qemu-error.h"
+#include "qdev.h"
+#include "virtio-bus.h"
+#include "virtio.h"
+
+#define DEBUG_VIRTIO_BUS
+
+#ifdef DEBUG_VIRTIO_BUS
+
+#define DPRINTF(fmt, ...) \
+do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
+#else
+#define DPRINTF(fmt, ...) do {} while (0)
+#endif
+
+static char *virtio_bus_get_fw_dev_path(DeviceState *dev);
+
+static void virtio_bus_class_init(ObjectClass *klass, void *data)
+{
+ BusClass *k = BUS_CLASS(klass);
+ k->get_fw_dev_path = virtio_bus_get_fw_dev_path;
+}
+
+static const TypeInfo virtio_bus_info = {
+ .name = TYPE_VIRTIO_BUS,
+ .parent = TYPE_BUS,
+ .instance_size = sizeof(VirtioBus),
+ .class_init = virtio_bus_class_init,
+};
+
+/* VirtioBus */
+
+static int next_virtio_bus;
+
+/* Create a virtio bus, and attach to transport. */
+void virtio_bus_new(VirtioBus *bus, DeviceState *host,
+ const VirtioBusInfo *info)
+{
+ /*
+ * Setting name to NULL return always "virtio.0"
+ * as bus name in info qtree.
+ */
+ char *bus_name = g_strdup_printf("%s.%d", BUS_NAME, next_virtio_bus);
+ qbus_create_inplace(&bus->qbus, TYPE_VIRTIO_BUS, host, bus_name);
+ bus->busnr = next_virtio_bus++;
+ bus->info = info;
+ /* no hotplug for the moment ? */
+ bus->qbus.allow_hotplug = 0;
+ bus->bus_in_use = false;
+ DPRINTF("bus %s created\n", bus_name);
+}
+
+/* Bind the VirtIODevice to the VirtioBus. */
+void virtio_bus_bind_device(VirtioBus *bus)
+{
+ assert(bus != NULL);
+ assert(bus->vdev != NULL);
+ virtio_bind_device(bus->vdev, &(bus->info->virtio_bindings),
+ bus->qbus.parent);
+}
+
+/* This must be called to when the VirtIODevice init */
+int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *vdev)
+{
+ if (bus->bus_in_use == true) {
+ error_report("%s in use.\n", bus->qbus.name);
+ return -1;
+ }
+ assert(bus->info->init_cb != NULL);
+ /* keep the VirtIODevice in the VirtioBus */
+ bus->vdev = vdev;
+ bus->info->init_cb(bus->qbus.parent);
+
+ bus->bus_in_use = true;
+ return 0;
+}
+
+/* This must be called when the VirtIODevice exit */
+void virtio_bus_exit_cb(VirtioBus *bus)
+{
+ assert(bus->info->exit_cb != NULL);
+ bus->info->exit_cb(bus->qbus.parent);
+ bus->bus_in_use = false;
+}
+
+static char *virtio_bus_get_fw_dev_path(DeviceState *dev)
+{
+ return g_strdup_printf("%s", qdev_fw_name(dev));
+}
+
+
+static void virtio_register_types(void)
+{
+ type_register_static(&virtio_bus_info);
+}
+
+type_init(virtio_register_types)
diff --git a/hw/virtio-bus.h b/hw/virtio-bus.h
new file mode 100644
index 0000000..6ea5035
--- /dev/null
+++ b/hw/virtio-bus.h
@@ -0,0 +1,49 @@
+/*
+ * VirtioBus
+ *
+ * Copyright (C) 2012 : GreenSocs Ltd
+ * http://www.greensocs.com/ , email: info@greensocs.com
+ *
+ * Developed by :
+ * Frederic Konrad <fred.konrad@greensocs.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ */
+
+#ifndef _VIRTIO_BUS_H_
+#define _VIRTIO_BUS_H_
+
+#include "qdev.h"
+#include "sysemu.h"
+#include "virtio.h"
+
+#define TYPE_VIRTIO_BUS "VIRTIO"
+#define BUS_NAME "virtio"
+
+typedef struct VirtioBus VirtioBus;
+typedef struct VirtioBusInfo VirtioBusInfo;
+
+struct VirtioBusInfo {
+ void (*init_cb)(DeviceState *dev);
+ void (*exit_cb)(DeviceState *dev);
+ VirtIOBindings virtio_bindings;
+};
+
+struct VirtioBus {
+ BusState qbus;
+ int busnr;
+ bool bus_in_use;
+ uint16_t pci_device_id;
+ uint16_t pci_class;
+ VirtIODevice *vdev;
+ const VirtioBusInfo *info;
+};
+
+void virtio_bus_new(VirtioBus *bus, DeviceState *host,
+ const VirtioBusInfo *info);
+void virtio_bus_bind_device(VirtioBus *bus);
+int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *dev);
+void virtio_bus_exit_cb(VirtioBus *bus);
+
+#endif
--
1.7.11.7
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [RFC PATCH 2/3] virtio-pci : Add a virtio-bus interface
2012-11-16 15:35 [Qemu-devel] [RFC PATCH 0/3] Virtio refactoring fred.konrad
2012-11-16 15:35 ` [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus fred.konrad
@ 2012-11-16 15:35 ` fred.konrad
2012-11-16 15:35 ` [Qemu-devel] [RFC PATCH 3/3] virtio-blk : add the virtio-blk device fred.konrad
2 siblings, 0 replies; 15+ messages in thread
From: fred.konrad @ 2012-11-16 15:35 UTC (permalink / raw)
To: qemu-devel; +Cc: mark.burton, KONRAD Frederic
From: KONRAD Frederic <fred.konrad@greensocs.com>
This patch add a VirtioBus in the VirtIOPCIProxy structure. It creates a new
device : "virtio-pci" which init the VirtioBus. Two callback are written :
* void virtio_pci_init_cb(DeviceState *dev) to initialize the PCI interface
after the VirtIODevice init, it is approximately the same as
virtio_init_pci.
* void virtio_pci_exit_cb(DeviceState *dev) to stop the ioevent before the
VirtIODevice exit.
Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
hw/virtio-pci.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
hw/virtio-pci.h | 6 +++
2 files changed, 148 insertions(+)
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 9603150..09bbb2a 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -22,6 +22,7 @@
#include "virtio-net.h"
#include "virtio-serial.h"
#include "virtio-scsi.h"
+#include "virtio-bus.h"
#include "pci.h"
#include "qemu-error.h"
#include "msi.h"
@@ -1039,13 +1040,154 @@ static TypeInfo virtio_scsi_info = {
.class_init = virtio_scsi_class_init,
};
+/* The new virtio-pci device */
+
+void virtio_pci_init_cb(DeviceState *dev)
+{
+ PCIDevice *d = DO_UPCAST(PCIDevice, qdev, dev);
+ VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, d);
+ uint8_t *config;
+ uint32_t size;
+ /* Put the PCI IDs */
+ pci_config_set_device_id(proxy->pci_dev.config,
+ proxy->bus.pci_device_id);
+ pci_config_set_class(proxy->pci_dev.config, proxy->bus.pci_class);
+
+ /* virtio_init_pci code without bindings */
+
+ /* This should disappear */
+ proxy->vdev = proxy->bus.vdev;
+
+ config = proxy->pci_dev.config;
+
+ if (proxy->class_code) {
+ pci_config_set_class(config, proxy->class_code);
+ }
+ pci_set_word(config + PCI_SUBSYSTEM_VENDOR_ID,
+ pci_get_word(config + PCI_VENDOR_ID));
+ pci_set_word(config + PCI_SUBSYSTEM_ID, proxy->bus.vdev->device_id);
+ config[PCI_INTERRUPT_PIN] = 1;
+
+ if (proxy->bus.vdev->nvectors &&
+ msix_init_exclusive_bar(&proxy->pci_dev, proxy->bus.vdev->nvectors,
+ 1)) {
+ proxy->bus.vdev->nvectors = 0;
+ }
+
+ proxy->pci_dev.config_write = virtio_write_config;
+
+ size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev)
+ + proxy->bus.vdev->config_len;
+ if (size & (size-1)) {
+ size = 1 << qemu_fls(size);
+ }
+
+ memory_region_init_io(&proxy->bar, &virtio_pci_config_ops, proxy,
+ "virtio-pci", size);
+ pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
+ &proxy->bar);
+
+ if (!kvm_has_many_ioeventfds()) {
+ proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
+ }
+
+ /* Bind the VirtIODevice to the VirtioBus. */
+ virtio_bus_bind_device(&(proxy->bus));
+
+ proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
+ proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
+ /* Should be modified */
+ proxy->host_features = proxy->bus.vdev->get_features(proxy->bus.vdev,
+ proxy->host_features);
+}
+
+void virtio_pci_exit_cb(DeviceState *dev)
+{
+ PCIDevice *d = DO_UPCAST(PCIDevice, qdev, dev);
+ VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, d);
+ /* Put the PCI IDs */
+ pci_config_set_device_id(proxy->pci_dev.config, 0x0000);
+ pci_config_set_class(proxy->pci_dev.config, PCI_CLASS_OTHERS);
+
+ virtio_pci_stop_ioeventfd(proxy);
+}
+
+static const struct VirtioBusInfo virtio_bus_info = {
+ .init_cb = virtio_pci_init_cb,
+ .exit_cb = virtio_pci_exit_cb,
+
+ .virtio_bindings = {
+ .notify = virtio_pci_notify,
+ .save_config = virtio_pci_save_config,
+ .load_config = virtio_pci_load_config,
+ .save_queue = virtio_pci_save_queue,
+ .load_queue = virtio_pci_load_queue,
+ .get_features = virtio_pci_get_features,
+ .query_guest_notifiers = virtio_pci_query_guest_notifiers,
+ .set_host_notifier = virtio_pci_set_host_notifier,
+ .set_guest_notifiers = virtio_pci_set_guest_notifiers,
+ .vmstate_change = virtio_pci_vmstate_change,
+ }
+};
+
+static int virtiopci_qdev_init(PCIDevice *dev)
+{
+ VirtIOPCIProxy *s = DO_UPCAST(VirtIOPCIProxy, pci_dev, dev);
+
+ /* create a virtio-bus and attach virtio-pci to it */
+ virtio_bus_new(&s->bus, &dev->qdev, &virtio_bus_info);
+
+ return 0;
+}
+
+static Property virtiopci_properties[] = {
+ /* TODO : Add the correct properties */
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void virtiopci_class_init(ObjectClass *oc, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(oc);
+ PCIDeviceClass *pc = PCI_DEVICE_CLASS(oc);
+
+ pc->init = virtiopci_qdev_init;
+ pc->exit = virtio_exit_pci;
+ pc->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
+ pc->revision = VIRTIO_PCI_ABI_VERSION;
+ pc->class_id = PCI_CLASS_OTHERS;
+ /* TODO : Add the correct device information below */
+ /* pc->exit =
+ * pc->device_id =
+ * pc->subsystem_vendor_id =
+ * pc->subsystem_id =
+ * dc->reset =
+ * dc->vmsd =
+ */
+ dc->props = virtiopci_properties;
+ dc->desc = "virtio-pci transport.";
+}
+
+static const TypeInfo virtio_pci_info = {
+ .name = "virtio-pci",
+ .parent = TYPE_PCI_DEVICE,
+ .instance_size = sizeof(VirtIOPCIProxy),
+ .class_init = virtiopci_class_init,
+};
+
+
+/************************************/
+
+
static void virtio_pci_register_types(void)
{
+ /* This should disappear */
type_register_static(&virtio_blk_info);
type_register_static(&virtio_net_info);
type_register_static(&virtio_serial_info);
type_register_static(&virtio_balloon_info);
type_register_static(&virtio_scsi_info);
+ /* new virtio-pci device */
+ type_register_static(&virtio_pci_info);
}
type_init(virtio_pci_register_types)
diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h
index ac9d522..d3ce579 100644
--- a/hw/virtio-pci.h
+++ b/hw/virtio-pci.h
@@ -19,6 +19,7 @@
#include "virtio-net.h"
#include "virtio-serial.h"
#include "virtio-scsi.h"
+#include "virtio-bus.h"
/* Performance improves when virtqueue kick processing is decoupled from the
* vcpu thread using ioeventfd for some devices. */
@@ -32,6 +33,7 @@ typedef struct {
typedef struct {
PCIDevice pci_dev;
+ /* This should be removed */
VirtIODevice *vdev;
MemoryRegion bar;
uint32_t flags;
@@ -49,10 +51,14 @@ typedef struct {
bool ioeventfd_disabled;
bool ioeventfd_started;
VirtIOIRQFD *vector_irqfd;
+ /* VirtIOBus to connect the VirtIODevice */
+ VirtioBus bus;
} VirtIOPCIProxy;
void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev);
void virtio_pci_reset(DeviceState *d);
+void virtio_pci_init_cb(DeviceState *dev);
+void virtio_pci_exit_cb(DeviceState *dev);
/* Virtio ABI version, if we increment this, we break the guest driver. */
#define VIRTIO_PCI_ABI_VERSION 0
--
1.7.11.7
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [RFC PATCH 3/3] virtio-blk : add the virtio-blk device.
2012-11-16 15:35 [Qemu-devel] [RFC PATCH 0/3] Virtio refactoring fred.konrad
2012-11-16 15:35 ` [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus fred.konrad
2012-11-16 15:35 ` [Qemu-devel] [RFC PATCH 2/3] virtio-pci : Add a virtio-bus interface fred.konrad
@ 2012-11-16 15:35 ` fred.konrad
2 siblings, 0 replies; 15+ messages in thread
From: fred.konrad @ 2012-11-16 15:35 UTC (permalink / raw)
To: qemu-devel; +Cc: mark.burton, KONRAD Frederic
From: KONRAD Frederic <fred.konrad@greensocs.com>
This patch just add the virtio-blk device which can connect on a Virtio-Bus. The
initialization fail if no free VirtioBus are present.
Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
hw/virtio-blk.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
hw/virtio-blk.h | 7 +++++
2 files changed, 91 insertions(+)
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index e25cc96..0fa2ab9 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -17,6 +17,8 @@
#include "hw/block-common.h"
#include "blockdev.h"
#include "virtio-blk.h"
+#include "virtio-bus.h"
+#include "hw/pci.h" /* for PCI IDs */
#include "scsi-defs.h"
#ifdef __linux__
# include <scsi/sg.h>
@@ -656,3 +658,85 @@ void virtio_blk_exit(VirtIODevice *vdev)
blockdev_mark_auto_del(s->bs);
virtio_cleanup(vdev);
}
+
+static int virtio_blk_qdev_init(DeviceState *qdev)
+{
+ VirtIODevice *vdev;
+ VirtIOBLKState *s = DO_UPCAST(VirtIOBLKState, qdev, qdev);
+ VirtioBus *bus = DO_UPCAST(VirtioBus, qbus, s->qdev.parent_bus);
+
+ vdev = virtio_blk_init(qdev, &s->blk);
+ if (!vdev) {
+ return -1;
+ }
+
+ /* Set the PCI IDs :
+ * What if we add both IDs in VirtIODevice ?
+ */
+ bus->pci_device_id = PCI_DEVICE_ID_VIRTIO_BLOCK;
+ bus->pci_class = PCI_CLASS_STORAGE_SCSI;
+
+ /* Call the init callback of the transport device eg: virtio-pci */
+ if (virtio_bus_init_cb(bus, vdev) != 0) {
+ /* this can happen when the bus is not free */
+ return -1;
+ }
+
+ return 0;
+}
+
+static int virtio_blk_qdev_exit(DeviceState *qdev)
+{
+ VirtioBus *bus = DO_UPCAST(VirtioBus, qbus, qdev->parent_bus);
+ virtio_bus_exit_cb(bus);
+ virtio_blk_exit(bus->vdev);
+ return 0;
+}
+
+static Property virtio_blk_properties[] = {
+ DEFINE_BLOCK_PROPERTIES(VirtIOBLKState, blk.conf),
+ DEFINE_BLOCK_CHS_PROPERTIES(VirtIOBLKState, blk.conf),
+ DEFINE_PROP_STRING("serial", VirtIOBLKState, blk.serial),
+#ifdef __linux__
+ DEFINE_PROP_BIT("scsi", VirtIOBLKState, blk.scsi, 0, true),
+#endif
+ DEFINE_PROP_BIT("config-wce", VirtIOBLKState, blk.config_wce, 0, true),
+ /*
+ * Theses one are PCI related.
+ * DEFINE_PROP_BIT("ioeventfd", VirtIOBLKState, flags,
+ * VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
+ * DEFINE_PROP_UINT32("vectors", VirtIOBLKState, nvectors, 2),
+ */
+ DEFINE_VIRTIO_BLK_FEATURES(VirtIOBLKState, host_features),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void virtio_blk_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *k = DEVICE_CLASS(klass);
+ k->bus_type = TYPE_VIRTIO_BUS;
+ k->init = virtio_blk_qdev_init;
+ k->exit = virtio_blk_qdev_exit;
+ /*
+ * k->unplug = ?
+ */
+ k->props = virtio_blk_properties;
+}
+
+static TypeInfo virtio_blk_info = {
+ .name = "virtio-blk",
+ .parent = TYPE_DEVICE,
+ .instance_size = sizeof(VirtIOBLKState),
+ /*
+ * .abstract = true,
+ * .class_size = sizeof(?),
+ */
+ .class_init = virtio_blk_class_init,
+};
+
+static void virtio_blk_register_types(void)
+{
+ type_register_static(&virtio_blk_info);
+}
+
+type_init(virtio_blk_register_types)
diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
index f0740d0..9e4bd27 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -16,6 +16,7 @@
#include "virtio.h"
#include "hw/block-common.h"
+#include "virtio-bus.h"
/* from Linux's linux/virtio_blk.h */
@@ -107,6 +108,12 @@ struct VirtIOBlkConf
uint32_t config_wce;
};
+typedef struct {
+ DeviceState qdev;
+ uint32_t host_features;
+ VirtIOBlkConf blk;
+} VirtIOBLKState;
+
#define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
DEFINE_VIRTIO_COMMON_FEATURES(_state, _field), \
DEFINE_PROP_BIT("config-wce", _state, _field, VIRTIO_BLK_F_CONFIG_WCE, true)
--
1.7.11.7
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus.
2012-11-16 15:35 ` [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus fred.konrad
@ 2012-11-19 17:33 ` Peter Maydell
2012-11-20 14:12 ` Cornelia Huck
2012-11-21 9:31 ` KONRAD Frédéric
2012-11-21 13:04 ` Andreas Färber
1 sibling, 2 replies; 15+ messages in thread
From: Peter Maydell @ 2012-11-19 17:33 UTC (permalink / raw)
To: fred.konrad
Cc: Anthony Liguori, Evgeny Voevodin, mark.burton, qemu-devel,
Cornelia Huck, Andreas Färber
On 16 November 2012 15:35, <fred.konrad@greensocs.com> wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
You forgot to CC enough people...
> This patch create a new VirtioBus, which can be added to Virtio transports like
> virtio-pci, virtio-mmio,...
>
> One VirtIODevice can be connected to this device, like virtio-blk in the 3rd
> patch.
>
> The VirtioBus shares through a VirtioBusInfo structure :
>
> * two callbacks with the transport : init_cb and exit_cb, which must be
> called by the VirtIODevice, after the initialization and before the
> destruction, to put the right PCI IDs and/or stop the event fd.
>
> * a VirtIOBindings structure.
>
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> ---
> hw/Makefile.objs | 1 +
> hw/virtio-bus.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> hw/virtio-bus.h | 49 ++++++++++++++++++++++++
> 3 files changed, 161 insertions(+)
> create mode 100644 hw/virtio-bus.c
> create mode 100644 hw/virtio-bus.h
>
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index af4ab0c..1b25d77 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -1,6 +1,7 @@
> common-obj-y = usb/ ide/
> common-obj-y += loader.o
> common-obj-$(CONFIG_VIRTIO) += virtio-console.o
> +common-obj-$(CONFIG_VIRTIO) += virtio-bus.o
> common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
> common-obj-y += fw_cfg.o
> common-obj-$(CONFIG_PCI) += pci.o pci_bridge.o pci_bridge_dev.o
> diff --git a/hw/virtio-bus.c b/hw/virtio-bus.c
> new file mode 100644
> index 0000000..b2e7e9c
> --- /dev/null
> +++ b/hw/virtio-bus.c
> @@ -0,0 +1,111 @@
> +/*
> + * VirtioBus
> + *
> + * Copyright (C) 2012 : GreenSocs Ltd
> + * http://www.greensocs.com/ , email: info@greensocs.com
> + *
> + * Developed by :
> + * Frederic Konrad <fred.konrad@greensocs.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
Unless you copied this code from an existing v2-only file, preferred
license for new code is v2-or-any-later-version.
> + */
> +
> +#include "hw.h"
> +#include "qemu-error.h"
> +#include "qdev.h"
> +#include "virtio-bus.h"
> +#include "virtio.h"
> +
> +#define DEBUG_VIRTIO_BUS
> +
> +#ifdef DEBUG_VIRTIO_BUS
> +
> +#define DPRINTF(fmt, ...) \
> +do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) do {} while (0)
> +#endif
> +
> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev);
Is this function necessary? I think your implementation
is doing the same as the default.
> +static void virtio_bus_class_init(ObjectClass *klass, void *data)
> +{
> + BusClass *k = BUS_CLASS(klass);
> + k->get_fw_dev_path = virtio_bus_get_fw_dev_path;
> +}
> +
> +static const TypeInfo virtio_bus_info = {
> + .name = TYPE_VIRTIO_BUS,
> + .parent = TYPE_BUS,
> + .instance_size = sizeof(VirtioBus),
> + .class_init = virtio_bus_class_init,
> +};
> +
> +/* VirtioBus */
> +
> +static int next_virtio_bus;
> +
> +/* Create a virtio bus, and attach to transport. */
> +void virtio_bus_new(VirtioBus *bus, DeviceState *host,
> + const VirtioBusInfo *info)
> +{
> + /*
> + * Setting name to NULL return always "virtio.0"
> + * as bus name in info qtree.
> + */
> + char *bus_name = g_strdup_printf("%s.%d", BUS_NAME, next_virtio_bus);
> + qbus_create_inplace(&bus->qbus, TYPE_VIRTIO_BUS, host, bus_name);
> + bus->busnr = next_virtio_bus++;
This looks suspicious to me -- is keeping a count of the global
bus number really the right way to do this?
> + bus->info = info;
> + /* no hotplug for the moment ? */
> + bus->qbus.allow_hotplug = 0;
Correct -- we don't need to hotplug the virtio backend.
> + bus->bus_in_use = false;
> + DPRINTF("bus %s created\n", bus_name);
> +}
> +
> +/* Bind the VirtIODevice to the VirtioBus. */
> +void virtio_bus_bind_device(VirtioBus *bus)
> +{
> + assert(bus != NULL);
> + assert(bus->vdev != NULL);
> + virtio_bind_device(bus->vdev, &(bus->info->virtio_bindings),
> + bus->qbus.parent);
This looks wrong -- virtio_bind_device() is part of the
old-style transport API.
> +}
> +
> +/* This must be called to when the VirtIODevice init */
> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *vdev)
> +{
> + if (bus->bus_in_use == true) {
> + error_report("%s in use.\n", bus->qbus.name);
> + return -1;
> + }
> + assert(bus->info->init_cb != NULL);
> + /* keep the VirtIODevice in the VirtioBus */
> + bus->vdev = vdev;
This shouldn't be happening here, it should happen as
part of plugging the device into the bus.
> + bus->info->init_cb(bus->qbus.parent);
> +
> + bus->bus_in_use = true;
> + return 0;
> +}
> +
> +/* This must be called when the VirtIODevice exit */
> +void virtio_bus_exit_cb(VirtioBus *bus)
> +{
> + assert(bus->info->exit_cb != NULL);
> + bus->info->exit_cb(bus->qbus.parent);
> + bus->bus_in_use = false;
> +}
These shouldn't be necessary -- the VirtioDevice should
have a pointer to the VirtioBus and can just invoke the
init/exit callbacks directly.
> +
> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev)
> +{
> + return g_strdup_printf("%s", qdev_fw_name(dev));
> +}
> +
> +
> +static void virtio_register_types(void)
> +{
> + type_register_static(&virtio_bus_info);
> +}
> +
> +type_init(virtio_register_types)
> diff --git a/hw/virtio-bus.h b/hw/virtio-bus.h
> new file mode 100644
> index 0000000..6ea5035
> --- /dev/null
> +++ b/hw/virtio-bus.h
> @@ -0,0 +1,49 @@
> +/*
> + * VirtioBus
> + *
> + * Copyright (C) 2012 : GreenSocs Ltd
> + * http://www.greensocs.com/ , email: info@greensocs.com
> + *
> + * Developed by :
> + * Frederic Konrad <fred.konrad@greensocs.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#ifndef _VIRTIO_BUS_H_
> +#define _VIRTIO_BUS_H_
Leading underscores are reserved by the C standard. VIRTIO_BUS_H will
do fine.
> +
> +#include "qdev.h"
> +#include "sysemu.h"
> +#include "virtio.h"
> +
> +#define TYPE_VIRTIO_BUS "VIRTIO"
Type names are generally "virtio-bus" by convention.
> +#define BUS_NAME "virtio"
I don't think you want this.
> +typedef struct VirtioBus VirtioBus;
> +typedef struct VirtioBusInfo VirtioBusInfo;
> +
> +struct VirtioBusInfo {
> + void (*init_cb)(DeviceState *dev);
> + void (*exit_cb)(DeviceState *dev);
> + VirtIOBindings virtio_bindings;
This doesn't look right -- VirtIOBindings is the
old-style way for the transport to specify a bunch
of function pointers for its specific implementation.
Those function pointers should probably just be in
the VirtioBus struct.
> +};
I was just talking to Anthony on IRC and he said SCSIBusInfo
is really kind of a historical accident. So we can just fold
this struct into VirtioBus. Sorry for misleading you earlier.
> +struct VirtioBus {
> + BusState qbus;
> + int busnr;
Why does the bus need to know what number it is?
> + bool bus_in_use;
> + uint16_t pci_device_id;
> + uint16_t pci_class;
This shouldn't be here -- VirtioBus should be transport
independent, so no PCI related info.
> + VirtIODevice *vdev;
This could use a comment saying that we only ever have one
child device on the bus.
> + const VirtioBusInfo *info;
> +};
> +
> +void virtio_bus_new(VirtioBus *bus, DeviceState *host,
> + const VirtioBusInfo *info);
> +void virtio_bus_bind_device(VirtioBus *bus);
> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *dev);
> +void virtio_bus_exit_cb(VirtioBus *bus);
> +
> +#endif
> --
> 1.7.11.7
-- PMM
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus.
2012-11-19 17:33 ` Peter Maydell
@ 2012-11-20 14:12 ` Cornelia Huck
2012-11-20 14:30 ` KONRAD Frédéric
2012-11-21 9:31 ` KONRAD Frédéric
1 sibling, 1 reply; 15+ messages in thread
From: Cornelia Huck @ 2012-11-20 14:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Anthony Liguori, Evgeny Voevodin, mark.burton, qemu-devel,
Andreas Färber, fred.konrad
On Mon, 19 Nov 2012 17:33:01 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:
> On 16 November 2012 15:35, <fred.konrad@greensocs.com> wrote:
> > From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> You forgot to CC enough people...
>
> > This patch create a new VirtioBus, which can be added to Virtio transports like
> > virtio-pci, virtio-mmio,...
> >
> > One VirtIODevice can be connected to this device, like virtio-blk in the 3rd
> > patch.
> >
> > The VirtioBus shares through a VirtioBusInfo structure :
> >
> > * two callbacks with the transport : init_cb and exit_cb, which must be
> > called by the VirtIODevice, after the initialization and before the
> > destruction, to put the right PCI IDs and/or stop the event fd.
Can we specify the general purpose of the {init,exit}_cb a bit better?
Is it analogous to any of the functions performed by the transport
busses today?
> >
> > * a VirtIOBindings structure.
> >
> > Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> > ---
> > hw/Makefile.objs | 1 +
> > hw/virtio-bus.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > hw/virtio-bus.h | 49 ++++++++++++++++++++++++
> > 3 files changed, 161 insertions(+)
> > create mode 100644 hw/virtio-bus.c
> > create mode 100644 hw/virtio-bus.h
> >
> > diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> > index af4ab0c..1b25d77 100644
> > --- a/hw/Makefile.objs
> > +++ b/hw/Makefile.objs
> > @@ -1,6 +1,7 @@
> > common-obj-y = usb/ ide/
> > common-obj-y += loader.o
> > common-obj-$(CONFIG_VIRTIO) += virtio-console.o
> > +common-obj-$(CONFIG_VIRTIO) += virtio-bus.o
> > common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
> > common-obj-y += fw_cfg.o
> > common-obj-$(CONFIG_PCI) += pci.o pci_bridge.o pci_bridge_dev.o
> > diff --git a/hw/virtio-bus.c b/hw/virtio-bus.c
> > new file mode 100644
> > index 0000000..b2e7e9c
> > --- /dev/null
> > +++ b/hw/virtio-bus.c
> > @@ -0,0 +1,111 @@
> > +/*
> > + * VirtioBus
> > + *
> > + * Copyright (C) 2012 : GreenSocs Ltd
> > + * http://www.greensocs.com/ , email: info@greensocs.com
> > + *
> > + * Developed by :
> > + * Frederic Konrad <fred.konrad@greensocs.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2. See
> > + * the COPYING file in the top-level directory.
>
> Unless you copied this code from an existing v2-only file, preferred
> license for new code is v2-or-any-later-version.
>
> > + */
> > +
> > +#include "hw.h"
> > +#include "qemu-error.h"
> > +#include "qdev.h"
> > +#include "virtio-bus.h"
> > +#include "virtio.h"
> > +
> > +#define DEBUG_VIRTIO_BUS
> > +
> > +#ifdef DEBUG_VIRTIO_BUS
> > +
> > +#define DPRINTF(fmt, ...) \
> > +do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
> > +#else
> > +#define DPRINTF(fmt, ...) do {} while (0)
> > +#endif
> > +
> > +static char *virtio_bus_get_fw_dev_path(DeviceState *dev);
>
> Is this function necessary? I think your implementation
> is doing the same as the default.
>
> > +static void virtio_bus_class_init(ObjectClass *klass, void *data)
> > +{
> > + BusClass *k = BUS_CLASS(klass);
> > + k->get_fw_dev_path = virtio_bus_get_fw_dev_path;
> > +}
> > +
> > +static const TypeInfo virtio_bus_info = {
> > + .name = TYPE_VIRTIO_BUS,
> > + .parent = TYPE_BUS,
> > + .instance_size = sizeof(VirtioBus),
> > + .class_init = virtio_bus_class_init,
> > +};
> > +
> > +/* VirtioBus */
> > +
> > +static int next_virtio_bus;
> > +
> > +/* Create a virtio bus, and attach to transport. */
> > +void virtio_bus_new(VirtioBus *bus, DeviceState *host,
> > + const VirtioBusInfo *info)
> > +{
> > + /*
> > + * Setting name to NULL return always "virtio.0"
> > + * as bus name in info qtree.
> > + */
> > + char *bus_name = g_strdup_printf("%s.%d", BUS_NAME, next_virtio_bus);
> > + qbus_create_inplace(&bus->qbus, TYPE_VIRTIO_BUS, host, bus_name);
> > + bus->busnr = next_virtio_bus++;
>
> This looks suspicious to me -- is keeping a count of the global
> bus number really the right way to do this?
If we want an unique number, we need to track usage of numbers, else we
end up with duplicates on wraparound.
>
> > + bus->info = info;
> > + /* no hotplug for the moment ? */
> > + bus->qbus.allow_hotplug = 0;
>
> Correct -- we don't need to hotplug the virtio backend.
>
> > + bus->bus_in_use = false;
> > + DPRINTF("bus %s created\n", bus_name);
> > +}
> > +
> > +/* Bind the VirtIODevice to the VirtioBus. */
> > +void virtio_bus_bind_device(VirtioBus *bus)
> > +{
> > + assert(bus != NULL);
> > + assert(bus->vdev != NULL);
> > + virtio_bind_device(bus->vdev, &(bus->info->virtio_bindings),
> > + bus->qbus.parent);
>
> This looks wrong -- virtio_bind_device() is part of the
> old-style transport API.
>
> > +}
> > +
> > +/* This must be called to when the VirtIODevice init */
> > +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *vdev)
> > +{
> > + if (bus->bus_in_use == true) {
> > + error_report("%s in use.\n", bus->qbus.name);
> > + return -1;
> > + }
> > + assert(bus->info->init_cb != NULL);
> > + /* keep the VirtIODevice in the VirtioBus */
> > + bus->vdev = vdev;
>
> This shouldn't be happening here, it should happen as
> part of plugging the device into the bus.
>
> > + bus->info->init_cb(bus->qbus.parent);
> > +
> > + bus->bus_in_use = true;
> > + return 0;
> > +}
> > +
> > +/* This must be called when the VirtIODevice exit */
> > +void virtio_bus_exit_cb(VirtioBus *bus)
> > +{
> > + assert(bus->info->exit_cb != NULL);
> > + bus->info->exit_cb(bus->qbus.parent);
> > + bus->bus_in_use = false;
> > +}
>
> These shouldn't be necessary -- the VirtioDevice should
> have a pointer to the VirtioBus and can just invoke the
> init/exit callbacks directly.
>
> > +
> > +static char *virtio_bus_get_fw_dev_path(DeviceState *dev)
> > +{
> > + return g_strdup_printf("%s", qdev_fw_name(dev));
> > +}
> > +
> > +
> > +static void virtio_register_types(void)
> > +{
> > + type_register_static(&virtio_bus_info);
> > +}
> > +
> > +type_init(virtio_register_types)
> > diff --git a/hw/virtio-bus.h b/hw/virtio-bus.h
> > new file mode 100644
> > index 0000000..6ea5035
> > --- /dev/null
> > +++ b/hw/virtio-bus.h
> > @@ -0,0 +1,49 @@
> > +/*
> > + * VirtioBus
> > + *
> > + * Copyright (C) 2012 : GreenSocs Ltd
> > + * http://www.greensocs.com/ , email: info@greensocs.com
> > + *
> > + * Developed by :
> > + * Frederic Konrad <fred.konrad@greensocs.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2. See
> > + * the COPYING file in the top-level directory.
> > + */
> > +
> > +#ifndef _VIRTIO_BUS_H_
> > +#define _VIRTIO_BUS_H_
>
> Leading underscores are reserved by the C standard. VIRTIO_BUS_H will
> do fine.
>
> > +
> > +#include "qdev.h"
> > +#include "sysemu.h"
> > +#include "virtio.h"
> > +
> > +#define TYPE_VIRTIO_BUS "VIRTIO"
>
> Type names are generally "virtio-bus" by convention.
>
> > +#define BUS_NAME "virtio"
>
> I don't think you want this.
>
>
> > +typedef struct VirtioBus VirtioBus;
> > +typedef struct VirtioBusInfo VirtioBusInfo;
> > +
> > +struct VirtioBusInfo {
> > + void (*init_cb)(DeviceState *dev);
> > + void (*exit_cb)(DeviceState *dev);
> > + VirtIOBindings virtio_bindings;
>
> This doesn't look right -- VirtIOBindings is the
> old-style way for the transport to specify a bunch
> of function pointers for its specific implementation.
> Those function pointers should probably just be in
> the VirtioBus struct.
>
> > +};
>
> I was just talking to Anthony on IRC and he said SCSIBusInfo
> is really kind of a historical accident. So we can just fold
> this struct into VirtioBus. Sorry for misleading you earlier.
>
> > +struct VirtioBus {
> > + BusState qbus;
> > + int busnr;
>
> Why does the bus need to know what number it is?
>
> > + bool bus_in_use;
> > + uint16_t pci_device_id;
> > + uint16_t pci_class;
>
> This shouldn't be here -- VirtioBus should be transport
> independent, so no PCI related info.
Agreed - this should be a property of the bridge device.
>
> > + VirtIODevice *vdev;
>
> This could use a comment saying that we only ever have one
> child device on the bus.
>
> > + const VirtioBusInfo *info;
> > +};
> > +
> > +void virtio_bus_new(VirtioBus *bus, DeviceState *host,
> > + const VirtioBusInfo *info);
> > +void virtio_bus_bind_device(VirtioBus *bus);
> > +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *dev);
> > +void virtio_bus_exit_cb(VirtioBus *bus);
> > +
> > +#endif
> > --
> > 1.7.11.7
>
> -- PMM
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus.
2012-11-20 14:12 ` Cornelia Huck
@ 2012-11-20 14:30 ` KONRAD Frédéric
2012-11-20 16:15 ` Cornelia Huck
0 siblings, 1 reply; 15+ messages in thread
From: KONRAD Frédéric @ 2012-11-20 14:30 UTC (permalink / raw)
To: Cornelia Huck
Cc: Peter Maydell, Anthony Liguori, Evgeny Voevodin, mark.burton,
qemu-devel, Andreas Färber
On 20/11/2012 15:12, Cornelia Huck wrote:
> On Mon, 19 Nov 2012 17:33:01 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> On 16 November 2012 15:35, <fred.konrad@greensocs.com> wrote:
>>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>> You forgot to CC enough people...
>>
>>> This patch create a new VirtioBus, which can be added to Virtio transports like
>>> virtio-pci, virtio-mmio,...
>>>
>>> One VirtIODevice can be connected to this device, like virtio-blk in the 3rd
>>> patch.
>>>
>>> The VirtioBus shares through a VirtioBusInfo structure :
>>>
>>> * two callbacks with the transport : init_cb and exit_cb, which must be
>>> called by the VirtIODevice, after the initialization and before the
>>> destruction, to put the right PCI IDs and/or stop the event fd.
> Can we specify the general purpose of the {init,exit}_cb a bit better?
> Is it analogous to any of the functions performed by the transport
> busses today?
For example, look at the function static int
virtio_blk_init_pci(PCIDevice *pci_dev) in virtio-pci.c,
it creates the VirtIODevice and then call virtio_init_pci(proxy, vdev);
It is what I tryed to reproduce, and abstract it from the new device.
eg : for the new virtio-blk I add, in the function static int
virtio_blk_qdev_init(DeviceState *qdev) in virtio-blk.c :
it creates the VirtIODevice, create the virtiobus and call the
virtio_bus_init_cb(bus, vdev)
which call the virtio_init_pci callback when virtio-pci bridge is used.
it is the same thing for the exit.
Is it making sense ?
>
>>> * a VirtIOBindings structure.
>>>
>>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>>> ---
>>> hw/Makefile.objs | 1 +
>>> hw/virtio-bus.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> hw/virtio-bus.h | 49 ++++++++++++++++++++++++
>>> 3 files changed, 161 insertions(+)
>>> create mode 100644 hw/virtio-bus.c
>>> create mode 100644 hw/virtio-bus.h
>>>
>>> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
>>> index af4ab0c..1b25d77 100644
>>> --- a/hw/Makefile.objs
>>> +++ b/hw/Makefile.objs
>>> @@ -1,6 +1,7 @@
>>> common-obj-y = usb/ ide/
>>> common-obj-y += loader.o
>>> common-obj-$(CONFIG_VIRTIO) += virtio-console.o
>>> +common-obj-$(CONFIG_VIRTIO) += virtio-bus.o
>>> common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
>>> common-obj-y += fw_cfg.o
>>> common-obj-$(CONFIG_PCI) += pci.o pci_bridge.o pci_bridge_dev.o
>>> diff --git a/hw/virtio-bus.c b/hw/virtio-bus.c
>>> new file mode 100644
>>> index 0000000..b2e7e9c
>>> --- /dev/null
>>> +++ b/hw/virtio-bus.c
>>> @@ -0,0 +1,111 @@
>>> +/*
>>> + * VirtioBus
>>> + *
>>> + * Copyright (C) 2012 : GreenSocs Ltd
>>> + * http://www.greensocs.com/ , email: info@greensocs.com
>>> + *
>>> + * Developed by :
>>> + * Frederic Konrad <fred.konrad@greensocs.com>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2. See
>>> + * the COPYING file in the top-level directory.
>> Unless you copied this code from an existing v2-only file, preferred
>> license for new code is v2-or-any-later-version.
>>
>>> + */
>>> +
>>> +#include "hw.h"
>>> +#include "qemu-error.h"
>>> +#include "qdev.h"
>>> +#include "virtio-bus.h"
>>> +#include "virtio.h"
>>> +
>>> +#define DEBUG_VIRTIO_BUS
>>> +
>>> +#ifdef DEBUG_VIRTIO_BUS
>>> +
>>> +#define DPRINTF(fmt, ...) \
>>> +do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
>>> +#else
>>> +#define DPRINTF(fmt, ...) do {} while (0)
>>> +#endif
>>> +
>>> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev);
>> Is this function necessary? I think your implementation
>> is doing the same as the default.
>>
>>> +static void virtio_bus_class_init(ObjectClass *klass, void *data)
>>> +{
>>> + BusClass *k = BUS_CLASS(klass);
>>> + k->get_fw_dev_path = virtio_bus_get_fw_dev_path;
>>> +}
>>> +
>>> +static const TypeInfo virtio_bus_info = {
>>> + .name = TYPE_VIRTIO_BUS,
>>> + .parent = TYPE_BUS,
>>> + .instance_size = sizeof(VirtioBus),
>>> + .class_init = virtio_bus_class_init,
>>> +};
>>> +
>>> +/* VirtioBus */
>>> +
>>> +static int next_virtio_bus;
>>> +
>>> +/* Create a virtio bus, and attach to transport. */
>>> +void virtio_bus_new(VirtioBus *bus, DeviceState *host,
>>> + const VirtioBusInfo *info)
>>> +{
>>> + /*
>>> + * Setting name to NULL return always "virtio.0"
>>> + * as bus name in info qtree.
>>> + */
>>> + char *bus_name = g_strdup_printf("%s.%d", BUS_NAME, next_virtio_bus);
>>> + qbus_create_inplace(&bus->qbus, TYPE_VIRTIO_BUS, host, bus_name);
>>> + bus->busnr = next_virtio_bus++;
>> This looks suspicious to me -- is keeping a count of the global
>> bus number really the right way to do this?
> If we want an unique number, we need to track usage of numbers, else we
> end up with duplicates on wraparound.
I put that because the number was all identical in "info qtree", is it
the right thing to do ?
>>> + bus->info = info;
>>> + /* no hotplug for the moment ? */
>>> + bus->qbus.allow_hotplug = 0;
>> Correct -- we don't need to hotplug the virtio backend.
>>
>>> + bus->bus_in_use = false;
>>> + DPRINTF("bus %s created\n", bus_name);
>>> +}
>>> +
>>> +/* Bind the VirtIODevice to the VirtioBus. */
>>> +void virtio_bus_bind_device(VirtioBus *bus)
>>> +{
>>> + assert(bus != NULL);
>>> + assert(bus->vdev != NULL);
>>> + virtio_bind_device(bus->vdev, &(bus->info->virtio_bindings),
>>> + bus->qbus.parent);
>> This looks wrong -- virtio_bind_device() is part of the
>> old-style transport API.
>>
>>> +}
>>> +
>>> +/* This must be called to when the VirtIODevice init */
>>> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *vdev)
>>> +{
>>> + if (bus->bus_in_use == true) {
>>> + error_report("%s in use.\n", bus->qbus.name);
>>> + return -1;
>>> + }
>>> + assert(bus->info->init_cb != NULL);
>>> + /* keep the VirtIODevice in the VirtioBus */
>>> + bus->vdev = vdev;
>> This shouldn't be happening here, it should happen as
>> part of plugging the device into the bus.
>>
>>> + bus->info->init_cb(bus->qbus.parent);
>>> +
>>> + bus->bus_in_use = true;
>>> + return 0;
>>> +}
>>> +
>>> +/* This must be called when the VirtIODevice exit */
>>> +void virtio_bus_exit_cb(VirtioBus *bus)
>>> +{
>>> + assert(bus->info->exit_cb != NULL);
>>> + bus->info->exit_cb(bus->qbus.parent);
>>> + bus->bus_in_use = false;
>>> +}
>> These shouldn't be necessary -- the VirtioDevice should
>> have a pointer to the VirtioBus and can just invoke the
>> init/exit callbacks directly.
>>
>>> +
>>> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev)
>>> +{
>>> + return g_strdup_printf("%s", qdev_fw_name(dev));
>>> +}
>>> +
>>> +
>>> +static void virtio_register_types(void)
>>> +{
>>> + type_register_static(&virtio_bus_info);
>>> +}
>>> +
>>> +type_init(virtio_register_types)
>>> diff --git a/hw/virtio-bus.h b/hw/virtio-bus.h
>>> new file mode 100644
>>> index 0000000..6ea5035
>>> --- /dev/null
>>> +++ b/hw/virtio-bus.h
>>> @@ -0,0 +1,49 @@
>>> +/*
>>> + * VirtioBus
>>> + *
>>> + * Copyright (C) 2012 : GreenSocs Ltd
>>> + * http://www.greensocs.com/ , email: info@greensocs.com
>>> + *
>>> + * Developed by :
>>> + * Frederic Konrad <fred.konrad@greensocs.com>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2. See
>>> + * the COPYING file in the top-level directory.
>>> + */
>>> +
>>> +#ifndef _VIRTIO_BUS_H_
>>> +#define _VIRTIO_BUS_H_
>> Leading underscores are reserved by the C standard. VIRTIO_BUS_H will
>> do fine.
>>
>>> +
>>> +#include "qdev.h"
>>> +#include "sysemu.h"
>>> +#include "virtio.h"
>>> +
>>> +#define TYPE_VIRTIO_BUS "VIRTIO"
>> Type names are generally "virtio-bus" by convention.
>>
>>> +#define BUS_NAME "virtio"
>> I don't think you want this.
>>
>>
>>> +typedef struct VirtioBus VirtioBus;
>>> +typedef struct VirtioBusInfo VirtioBusInfo;
>>> +
>>> +struct VirtioBusInfo {
>>> + void (*init_cb)(DeviceState *dev);
>>> + void (*exit_cb)(DeviceState *dev);
>>> + VirtIOBindings virtio_bindings;
>> This doesn't look right -- VirtIOBindings is the
>> old-style way for the transport to specify a bunch
>> of function pointers for its specific implementation.
>> Those function pointers should probably just be in
>> the VirtioBus struct.
>>
>>> +};
>> I was just talking to Anthony on IRC and he said SCSIBusInfo
>> is really kind of a historical accident. So we can just fold
>> this struct into VirtioBus. Sorry for misleading you earlier.
>>
>>> +struct VirtioBus {
>>> + BusState qbus;
>>> + int busnr;
>> Why does the bus need to know what number it is?
>>
>>> + bool bus_in_use;
>>> + uint16_t pci_device_id;
>>> + uint16_t pci_class;
>> This shouldn't be here -- VirtioBus should be transport
>> independent, so no PCI related info.
> Agreed - this should be a property of the bridge device.
Yes, So I must add the virtiodevice identifier and put a switch case in
the transport to set the right PCI IDs?
In that case, the transport won't be device independent.
>>> + VirtIODevice *vdev;
>> This could use a comment saying that we only ever have one
>> child device on the bus.
>>
>> + const VirtioBusInfo *info;
>> +};
>> +
>> +void virtio_bus_new(VirtioBus *bus, DeviceState *host,
>> + const VirtioBusInfo *info);
>> +void virtio_bus_bind_device(VirtioBus *bus);
>> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *dev);
>> +void virtio_bus_exit_cb(VirtioBus *bus);
>> +
>> +#endif
>> --
>> 1.7.11.7
>> -- PMM
>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus.
2012-11-20 14:30 ` KONRAD Frédéric
@ 2012-11-20 16:15 ` Cornelia Huck
2012-11-20 16:45 ` KONRAD Frédéric
0 siblings, 1 reply; 15+ messages in thread
From: Cornelia Huck @ 2012-11-20 16:15 UTC (permalink / raw)
To: KONRAD Frédéric
Cc: Peter Maydell, Anthony Liguori, Evgeny Voevodin, mark.burton,
qemu-devel, Andreas Färber
On Tue, 20 Nov 2012 15:30:35 +0100
KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
> On 20/11/2012 15:12, Cornelia Huck wrote:
> > On Mon, 19 Nov 2012 17:33:01 +0000
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> >> On 16 November 2012 15:35, <fred.konrad@greensocs.com> wrote:
> >>> From: KONRAD Frederic <fred.konrad@greensocs.com>
> >> You forgot to CC enough people...
> >>
> >>> This patch create a new VirtioBus, which can be added to Virtio transports like
> >>> virtio-pci, virtio-mmio,...
> >>>
> >>> One VirtIODevice can be connected to this device, like virtio-blk in the 3rd
> >>> patch.
> >>>
> >>> The VirtioBus shares through a VirtioBusInfo structure :
> >>>
> >>> * two callbacks with the transport : init_cb and exit_cb, which must be
> >>> called by the VirtIODevice, after the initialization and before the
> >>> destruction, to put the right PCI IDs and/or stop the event fd.
> > Can we specify the general purpose of the {init,exit}_cb a bit better?
> > Is it analogous to any of the functions performed by the transport
> > busses today?
> For example, look at the function static int
> virtio_blk_init_pci(PCIDevice *pci_dev) in virtio-pci.c,
>
> it creates the VirtIODevice and then call virtio_init_pci(proxy, vdev);
>
> It is what I tryed to reproduce, and abstract it from the new device.
>
> eg : for the new virtio-blk I add, in the function static int
> virtio_blk_qdev_init(DeviceState *qdev) in virtio-blk.c :
>
> it creates the VirtIODevice, create the virtiobus and call the
> virtio_bus_init_cb(bus, vdev)
> which call the virtio_init_pci callback when virtio-pci bridge is used.
>
> it is the same thing for the exit.
>
> Is it making sense ?
Yes, I think I understand. It would probably make sense to add a
comment like "transport specific, device type independent
initialization" or so.
>
> >
> >>> * a VirtIOBindings structure.
> >>>
> >>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> >>> ---
> >>> hw/Makefile.objs | 1 +
> >>> hw/virtio-bus.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>> hw/virtio-bus.h | 49 ++++++++++++++++++++++++
> >>> 3 files changed, 161 insertions(+)
> >>> create mode 100644 hw/virtio-bus.c
> >>> create mode 100644 hw/virtio-bus.h
> >>>
> >>> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> >>> index af4ab0c..1b25d77 100644
> >>> --- a/hw/Makefile.objs
> >>> +++ b/hw/Makefile.objs
> >>> @@ -1,6 +1,7 @@
> >>> common-obj-y = usb/ ide/
> >>> common-obj-y += loader.o
> >>> common-obj-$(CONFIG_VIRTIO) += virtio-console.o
> >>> +common-obj-$(CONFIG_VIRTIO) += virtio-bus.o
> >>> common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
> >>> common-obj-y += fw_cfg.o
> >>> common-obj-$(CONFIG_PCI) += pci.o pci_bridge.o pci_bridge_dev.o
> >>> diff --git a/hw/virtio-bus.c b/hw/virtio-bus.c
> >>> new file mode 100644
> >>> index 0000000..b2e7e9c
> >>> --- /dev/null
> >>> +++ b/hw/virtio-bus.c
> >>> @@ -0,0 +1,111 @@
> >>> +/*
> >>> + * VirtioBus
> >>> + *
> >>> + * Copyright (C) 2012 : GreenSocs Ltd
> >>> + * http://www.greensocs.com/ , email: info@greensocs.com
> >>> + *
> >>> + * Developed by :
> >>> + * Frederic Konrad <fred.konrad@greensocs.com>
> >>> + *
> >>> + * This work is licensed under the terms of the GNU GPL, version 2. See
> >>> + * the COPYING file in the top-level directory.
> >> Unless you copied this code from an existing v2-only file, preferred
> >> license for new code is v2-or-any-later-version.
> >>
> >>> + */
> >>> +
> >>> +#include "hw.h"
> >>> +#include "qemu-error.h"
> >>> +#include "qdev.h"
> >>> +#include "virtio-bus.h"
> >>> +#include "virtio.h"
> >>> +
> >>> +#define DEBUG_VIRTIO_BUS
> >>> +
> >>> +#ifdef DEBUG_VIRTIO_BUS
> >>> +
> >>> +#define DPRINTF(fmt, ...) \
> >>> +do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
> >>> +#else
> >>> +#define DPRINTF(fmt, ...) do {} while (0)
> >>> +#endif
> >>> +
> >>> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev);
> >> Is this function necessary? I think your implementation
> >> is doing the same as the default.
> >>
> >>> +static void virtio_bus_class_init(ObjectClass *klass, void *data)
> >>> +{
> >>> + BusClass *k = BUS_CLASS(klass);
> >>> + k->get_fw_dev_path = virtio_bus_get_fw_dev_path;
> >>> +}
> >>> +
> >>> +static const TypeInfo virtio_bus_info = {
> >>> + .name = TYPE_VIRTIO_BUS,
> >>> + .parent = TYPE_BUS,
> >>> + .instance_size = sizeof(VirtioBus),
> >>> + .class_init = virtio_bus_class_init,
> >>> +};
> >>> +
> >>> +/* VirtioBus */
> >>> +
> >>> +static int next_virtio_bus;
> >>> +
> >>> +/* Create a virtio bus, and attach to transport. */
> >>> +void virtio_bus_new(VirtioBus *bus, DeviceState *host,
> >>> + const VirtioBusInfo *info)
> >>> +{
> >>> + /*
> >>> + * Setting name to NULL return always "virtio.0"
> >>> + * as bus name in info qtree.
> >>> + */
> >>> + char *bus_name = g_strdup_printf("%s.%d", BUS_NAME, next_virtio_bus);
> >>> + qbus_create_inplace(&bus->qbus, TYPE_VIRTIO_BUS, host, bus_name);
> >>> + bus->busnr = next_virtio_bus++;
> >> This looks suspicious to me -- is keeping a count of the global
> >> bus number really the right way to do this?
> > If we want an unique number, we need to track usage of numbers, else we
> > end up with duplicates on wraparound.
> I put that because the number was all identical in "info qtree", is it
> the right thing to do ?
Not sure whether we need a unique name for each bus as each proxy
device will have only one bus beneath it - but if we need it, it must
stay unique when hot(un)plugging devices, even after walking the int
space once.
>
> >>> + bus->info = info;
> >>> + /* no hotplug for the moment ? */
> >>> + bus->qbus.allow_hotplug = 0;
> >> Correct -- we don't need to hotplug the virtio backend.
> >>
> >>> + bus->bus_in_use = false;
> >>> + DPRINTF("bus %s created\n", bus_name);
> >>> +}
> >>> +
> >>> +/* Bind the VirtIODevice to the VirtioBus. */
> >>> +void virtio_bus_bind_device(VirtioBus *bus)
> >>> +{
> >>> + assert(bus != NULL);
> >>> + assert(bus->vdev != NULL);
> >>> + virtio_bind_device(bus->vdev, &(bus->info->virtio_bindings),
> >>> + bus->qbus.parent);
> >> This looks wrong -- virtio_bind_device() is part of the
> >> old-style transport API.
> >>
>
>
> >>> +}
> >>> +
> >>> +/* This must be called to when the VirtIODevice init */
> >>> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *vdev)
> >>> +{
> >>> + if (bus->bus_in_use == true) {
> >>> + error_report("%s in use.\n", bus->qbus.name);
> >>> + return -1;
> >>> + }
> >>> + assert(bus->info->init_cb != NULL);
> >>> + /* keep the VirtIODevice in the VirtioBus */
> >>> + bus->vdev = vdev;
> >> This shouldn't be happening here, it should happen as
> >> part of plugging the device into the bus.
> >>
> >>> + bus->info->init_cb(bus->qbus.parent);
> >>> +
> >>> + bus->bus_in_use = true;
> >>> + return 0;
> >>> +}
> >>> +
> >>> +/* This must be called when the VirtIODevice exit */
> >>> +void virtio_bus_exit_cb(VirtioBus *bus)
> >>> +{
> >>> + assert(bus->info->exit_cb != NULL);
> >>> + bus->info->exit_cb(bus->qbus.parent);
> >>> + bus->bus_in_use = false;
> >>> +}
> >> These shouldn't be necessary -- the VirtioDevice should
> >> have a pointer to the VirtioBus and can just invoke the
> >> init/exit callbacks directly.
> >>
> >>> +
> >>> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev)
> >>> +{
> >>> + return g_strdup_printf("%s", qdev_fw_name(dev));
> >>> +}
> >>> +
> >>> +
> >>> +static void virtio_register_types(void)
> >>> +{
> >>> + type_register_static(&virtio_bus_info);
> >>> +}
> >>> +
> >>> +type_init(virtio_register_types)
> >>> diff --git a/hw/virtio-bus.h b/hw/virtio-bus.h
> >>> new file mode 100644
> >>> index 0000000..6ea5035
> >>> --- /dev/null
> >>> +++ b/hw/virtio-bus.h
> >>> @@ -0,0 +1,49 @@
> >>> +/*
> >>> + * VirtioBus
> >>> + *
> >>> + * Copyright (C) 2012 : GreenSocs Ltd
> >>> + * http://www.greensocs.com/ , email: info@greensocs.com
> >>> + *
> >>> + * Developed by :
> >>> + * Frederic Konrad <fred.konrad@greensocs.com>
> >>> + *
> >>> + * This work is licensed under the terms of the GNU GPL, version 2. See
> >>> + * the COPYING file in the top-level directory.
> >>> + */
> >>> +
> >>> +#ifndef _VIRTIO_BUS_H_
> >>> +#define _VIRTIO_BUS_H_
> >> Leading underscores are reserved by the C standard. VIRTIO_BUS_H will
> >> do fine.
> >>
> >>> +
> >>> +#include "qdev.h"
> >>> +#include "sysemu.h"
> >>> +#include "virtio.h"
> >>> +
> >>> +#define TYPE_VIRTIO_BUS "VIRTIO"
> >> Type names are generally "virtio-bus" by convention.
> >>
> >>> +#define BUS_NAME "virtio"
> >> I don't think you want this.
> >>
> >>
> >>> +typedef struct VirtioBus VirtioBus;
> >>> +typedef struct VirtioBusInfo VirtioBusInfo;
> >>> +
> >>> +struct VirtioBusInfo {
> >>> + void (*init_cb)(DeviceState *dev);
> >>> + void (*exit_cb)(DeviceState *dev);
> >>> + VirtIOBindings virtio_bindings;
> >> This doesn't look right -- VirtIOBindings is the
> >> old-style way for the transport to specify a bunch
> >> of function pointers for its specific implementation.
> >> Those function pointers should probably just be in
> >> the VirtioBus struct.
> >>
> >>> +};
> >> I was just talking to Anthony on IRC and he said SCSIBusInfo
> >> is really kind of a historical accident. So we can just fold
> >> this struct into VirtioBus. Sorry for misleading you earlier.
> >>
> >>> +struct VirtioBus {
> >>> + BusState qbus;
> >>> + int busnr;
> >> Why does the bus need to know what number it is?
> >>
> >>> + bool bus_in_use;
> >>> + uint16_t pci_device_id;
> >>> + uint16_t pci_class;
> >> This shouldn't be here -- VirtioBus should be transport
> >> independent, so no PCI related info.
> > Agreed - this should be a property of the bridge device.
> Yes, So I must add the virtiodevice identifier and put a switch case in
> the transport to set the right PCI IDs?
> In that case, the transport won't be device independent.
So these are values depending upon the virtio device type?
Can you calculate these from the virtio device?
>
> >>> + VirtIODevice *vdev;
> >> This could use a comment saying that we only ever have one
> >> child device on the bus.
> >>
> >> + const VirtioBusInfo *info;
> >> +};
> >> +
> >> +void virtio_bus_new(VirtioBus *bus, DeviceState *host,
> >> + const VirtioBusInfo *info);
> >> +void virtio_bus_bind_device(VirtioBus *bus);
> >> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *dev);
> >> +void virtio_bus_exit_cb(VirtioBus *bus);
> >> +
> >> +#endif
> >> --
> >> 1.7.11.7
> >> -- PMM
> >>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus.
2012-11-20 16:15 ` Cornelia Huck
@ 2012-11-20 16:45 ` KONRAD Frédéric
2012-11-20 17:27 ` Cornelia Huck
0 siblings, 1 reply; 15+ messages in thread
From: KONRAD Frédéric @ 2012-11-20 16:45 UTC (permalink / raw)
To: Cornelia Huck
Cc: Peter Maydell, Anthony Liguori, Evgeny Voevodin, mark.burton,
qemu-devel, Andreas Färber
On 20/11/2012 17:15, Cornelia Huck wrote:
> On Tue, 20 Nov 2012 15:30:35 +0100
> KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
>
>> On 20/11/2012 15:12, Cornelia Huck wrote:
>>> On Mon, 19 Nov 2012 17:33:01 +0000
>>> Peter Maydell <peter.maydell@linaro.org> wrote:
>>>
>>>> On 16 November 2012 15:35, <fred.konrad@greensocs.com> wrote:
>>>>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>>> You forgot to CC enough people...
>>>>
>>>>> This patch create a new VirtioBus, which can be added to Virtio transports like
>>>>> virtio-pci, virtio-mmio,...
>>>>>
>>>>> One VirtIODevice can be connected to this device, like virtio-blk in the 3rd
>>>>> patch.
>>>>>
>>>>> The VirtioBus shares through a VirtioBusInfo structure :
>>>>>
>>>>> * two callbacks with the transport : init_cb and exit_cb, which must be
>>>>> called by the VirtIODevice, after the initialization and before the
>>>>> destruction, to put the right PCI IDs and/or stop the event fd.
>>> Can we specify the general purpose of the {init,exit}_cb a bit better?
>>> Is it analogous to any of the functions performed by the transport
>>> busses today?
>> For example, look at the function static int
>> virtio_blk_init_pci(PCIDevice *pci_dev) in virtio-pci.c,
>>
>> it creates the VirtIODevice and then call virtio_init_pci(proxy, vdev);
>>
>> It is what I tryed to reproduce, and abstract it from the new device.
>>
>> eg : for the new virtio-blk I add, in the function static int
>> virtio_blk_qdev_init(DeviceState *qdev) in virtio-blk.c :
>>
>> it creates the VirtIODevice, create the virtiobus and call the
>> virtio_bus_init_cb(bus, vdev)
>> which call the virtio_init_pci callback when virtio-pci bridge is used.
>>
>> it is the same thing for the exit.
>>
>> Is it making sense ?
> Yes, I think I understand. It would probably make sense to add a
> comment like "transport specific, device type independent
> initialization" or so.
Ok, good idea I'll add that.
>>>>> * a VirtIOBindings structure.
>>>>>
>>>>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>>>>> ---
>>>>> hw/Makefile.objs | 1 +
>>>>> hw/virtio-bus.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>> hw/virtio-bus.h | 49 ++++++++++++++++++++++++
>>>>> 3 files changed, 161 insertions(+)
>>>>> create mode 100644 hw/virtio-bus.c
>>>>> create mode 100644 hw/virtio-bus.h
>>>>>
>>>>> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
>>>>> index af4ab0c..1b25d77 100644
>>>>> --- a/hw/Makefile.objs
>>>>> +++ b/hw/Makefile.objs
>>>>> @@ -1,6 +1,7 @@
>>>>> common-obj-y = usb/ ide/
>>>>> common-obj-y += loader.o
>>>>> common-obj-$(CONFIG_VIRTIO) += virtio-console.o
>>>>> +common-obj-$(CONFIG_VIRTIO) += virtio-bus.o
>>>>> common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
>>>>> common-obj-y += fw_cfg.o
>>>>> common-obj-$(CONFIG_PCI) += pci.o pci_bridge.o pci_bridge_dev.o
>>>>> diff --git a/hw/virtio-bus.c b/hw/virtio-bus.c
>>>>> new file mode 100644
>>>>> index 0000000..b2e7e9c
>>>>> --- /dev/null
>>>>> +++ b/hw/virtio-bus.c
>>>>> @@ -0,0 +1,111 @@
>>>>> +/*
>>>>> + * VirtioBus
>>>>> + *
>>>>> + * Copyright (C) 2012 : GreenSocs Ltd
>>>>> + * http://www.greensocs.com/ , email: info@greensocs.com
>>>>> + *
>>>>> + * Developed by :
>>>>> + * Frederic Konrad <fred.konrad@greensocs.com>
>>>>> + *
>>>>> + * This work is licensed under the terms of the GNU GPL, version 2. See
>>>>> + * the COPYING file in the top-level directory.
>>>> Unless you copied this code from an existing v2-only file, preferred
>>>> license for new code is v2-or-any-later-version.
>>>>
>>>>> + */
>>>>> +
>>>>> +#include "hw.h"
>>>>> +#include "qemu-error.h"
>>>>> +#include "qdev.h"
>>>>> +#include "virtio-bus.h"
>>>>> +#include "virtio.h"
>>>>> +
>>>>> +#define DEBUG_VIRTIO_BUS
>>>>> +
>>>>> +#ifdef DEBUG_VIRTIO_BUS
>>>>> +
>>>>> +#define DPRINTF(fmt, ...) \
>>>>> +do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
>>>>> +#else
>>>>> +#define DPRINTF(fmt, ...) do {} while (0)
>>>>> +#endif
>>>>> +
>>>>> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev);
>>>> Is this function necessary? I think your implementation
>>>> is doing the same as the default.
>>>>
>>>>> +static void virtio_bus_class_init(ObjectClass *klass, void *data)
>>>>> +{
>>>>> + BusClass *k = BUS_CLASS(klass);
>>>>> + k->get_fw_dev_path = virtio_bus_get_fw_dev_path;
>>>>> +}
>>>>> +
>>>>> +static const TypeInfo virtio_bus_info = {
>>>>> + .name = TYPE_VIRTIO_BUS,
>>>>> + .parent = TYPE_BUS,
>>>>> + .instance_size = sizeof(VirtioBus),
>>>>> + .class_init = virtio_bus_class_init,
>>>>> +};
>>>>> +
>>>>> +/* VirtioBus */
>>>>> +
>>>>> +static int next_virtio_bus;
>>>>> +
>>>>> +/* Create a virtio bus, and attach to transport. */
>>>>> +void virtio_bus_new(VirtioBus *bus, DeviceState *host,
>>>>> + const VirtioBusInfo *info)
>>>>> +{
>>>>> + /*
>>>>> + * Setting name to NULL return always "virtio.0"
>>>>> + * as bus name in info qtree.
>>>>> + */
>>>>> + char *bus_name = g_strdup_printf("%s.%d", BUS_NAME, next_virtio_bus);
>>>>> + qbus_create_inplace(&bus->qbus, TYPE_VIRTIO_BUS, host, bus_name);
>>>>> + bus->busnr = next_virtio_bus++;
>>>> This looks suspicious to me -- is keeping a count of the global
>>>> bus number really the right way to do this?
>>> If we want an unique number, we need to track usage of numbers, else we
>>> end up with duplicates on wraparound.
>> I put that because the number was all identical in "info qtree", is it
>> the right thing to do ?
> Not sure whether we need a unique name for each bus as each proxy
> device will have only one bus beneath it - but if we need it, it must
> stay unique when hot(un)plugging devices, even after walking the int
> space once.
Ok, it isn't necessary as we don't allow hotplugging.
>>>>> + bus->info = info;
>>>>> + /* no hotplug for the moment ? */
>>>>> + bus->qbus.allow_hotplug = 0;
>>>> Correct -- we don't need to hotplug the virtio backend.
>>>>
>>>>> + bus->bus_in_use = false;
>>>>> + DPRINTF("bus %s created\n", bus_name);
>>>>> +}
>>>>> +
>>>>> +/* Bind the VirtIODevice to the VirtioBus. */
>>>>> +void virtio_bus_bind_device(VirtioBus *bus)
>>>>> +{
>>>>> + assert(bus != NULL);
>>>>> + assert(bus->vdev != NULL);
>>>>> + virtio_bind_device(bus->vdev, &(bus->info->virtio_bindings),
>>>>> + bus->qbus.parent);
>>>> This looks wrong -- virtio_bind_device() is part of the
>>>> old-style transport API.
>>>>
>>
>>>>> +}
>>>>> +
>>>>> +/* This must be called to when the VirtIODevice init */
>>>>> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *vdev)
>>>>> +{
>>>>> + if (bus->bus_in_use == true) {
>>>>> + error_report("%s in use.\n", bus->qbus.name);
>>>>> + return -1;
>>>>> + }
>>>>> + assert(bus->info->init_cb != NULL);
>>>>> + /* keep the VirtIODevice in the VirtioBus */
>>>>> + bus->vdev = vdev;
>>>> This shouldn't be happening here, it should happen as
>>>> part of plugging the device into the bus.
>>>>
>>>>> + bus->info->init_cb(bus->qbus.parent);
>>>>> +
>>>>> + bus->bus_in_use = true;
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +/* This must be called when the VirtIODevice exit */
>>>>> +void virtio_bus_exit_cb(VirtioBus *bus)
>>>>> +{
>>>>> + assert(bus->info->exit_cb != NULL);
>>>>> + bus->info->exit_cb(bus->qbus.parent);
>>>>> + bus->bus_in_use = false;
>>>>> +}
>>>> These shouldn't be necessary -- the VirtioDevice should
>>>> have a pointer to the VirtioBus and can just invoke the
>>>> init/exit callbacks directly.
>>>>
>>>>> +
>>>>> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev)
>>>>> +{
>>>>> + return g_strdup_printf("%s", qdev_fw_name(dev));
>>>>> +}
>>>>> +
>>>>> +
>>>>> +static void virtio_register_types(void)
>>>>> +{
>>>>> + type_register_static(&virtio_bus_info);
>>>>> +}
>>>>> +
>>>>> +type_init(virtio_register_types)
>>>>> diff --git a/hw/virtio-bus.h b/hw/virtio-bus.h
>>>>> new file mode 100644
>>>>> index 0000000..6ea5035
>>>>> --- /dev/null
>>>>> +++ b/hw/virtio-bus.h
>>>>> @@ -0,0 +1,49 @@
>>>>> +/*
>>>>> + * VirtioBus
>>>>> + *
>>>>> + * Copyright (C) 2012 : GreenSocs Ltd
>>>>> + * http://www.greensocs.com/ , email: info@greensocs.com
>>>>> + *
>>>>> + * Developed by :
>>>>> + * Frederic Konrad <fred.konrad@greensocs.com>
>>>>> + *
>>>>> + * This work is licensed under the terms of the GNU GPL, version 2. See
>>>>> + * the COPYING file in the top-level directory.
>>>>> + */
>>>>> +
>>>>> +#ifndef _VIRTIO_BUS_H_
>>>>> +#define _VIRTIO_BUS_H_
>>>> Leading underscores are reserved by the C standard. VIRTIO_BUS_H will
>>>> do fine.
>>>>
>>>>> +
>>>>> +#include "qdev.h"
>>>>> +#include "sysemu.h"
>>>>> +#include "virtio.h"
>>>>> +
>>>>> +#define TYPE_VIRTIO_BUS "VIRTIO"
>>>> Type names are generally "virtio-bus" by convention.
>>>>
>>>>> +#define BUS_NAME "virtio"
>>>> I don't think you want this.
>>>>
>>>>
>>>>> +typedef struct VirtioBus VirtioBus;
>>>>> +typedef struct VirtioBusInfo VirtioBusInfo;
>>>>> +
>>>>> +struct VirtioBusInfo {
>>>>> + void (*init_cb)(DeviceState *dev);
>>>>> + void (*exit_cb)(DeviceState *dev);
>>>>> + VirtIOBindings virtio_bindings;
>>>> This doesn't look right -- VirtIOBindings is the
>>>> old-style way for the transport to specify a bunch
>>>> of function pointers for its specific implementation.
>>>> Those function pointers should probably just be in
>>>> the VirtioBus struct.
>>>>
>>>>> +};
>>>> I was just talking to Anthony on IRC and he said SCSIBusInfo
>>>> is really kind of a historical accident. So we can just fold
>>>> this struct into VirtioBus. Sorry for misleading you earlier.
>>>>
>>>>> +struct VirtioBus {
>>>>> + BusState qbus;
>>>>> + int busnr;
>>>> Why does the bus need to know what number it is?
>>>>
>>>>> + bool bus_in_use;
>>>>> + uint16_t pci_device_id;
>>>>> + uint16_t pci_class;
>>>> This shouldn't be here -- VirtioBus should be transport
>>>> independent, so no PCI related info.
>>> Agreed - this should be a property of the bridge device.
>> Yes, So I must add the virtiodevice identifier and put a switch case in
>> the transport to set the right PCI IDs?
>> In that case, the transport won't be device independent.
> So these are values depending upon the virtio device type?
> Can you calculate these from the virtio device?
Yes, it depends on the virtio device id, they are set for the virtio-x-pci
devices in the init class.
eg for virtio-block-pci in virtio-pci.c :
static void virtio_blk_class_init(ObjectClass *klass, void *data)
{
k->device_id = PCI_DEVICE_ID_VIRTIO_BLOCK;
...
k->class_id = PCI_CLASS_STORAGE_SCSI;
}
I think that the better solution is to put these value in a big switch
case :
eg :
Adding that in virtio-bus.c :
/* Return the virtio device id of the plugged device. */
uint16_t get_virtio_device_id(VirtioBus *bus)
{
return bus->vdev->device_id;
}
Using that in virtio-pci transport initialisation.
switch (get_virtio_device_id(&proxy->bus))
{
case VIRTIO_ID_BLOCK:
pci_config_set_device_id(proxy->pci_dev.config,
PCI_DEVICE_ID_VIRTIO_BLOCK);
pci_config_set_class(proxy->pci_dev.config,
PCI_CLASS_STORAGE_SCSI);
break;
default:
error_report("unknown device id\n");
break;
}
but the transport stay ( a little ) device dependent.
>>>>> + VirtIODevice *vdev;
>>>> This could use a comment saying that we only ever have one
>>>> child device on the bus.
>>>>
>>>> + const VirtioBusInfo *info;
>>>> +};
>>>> +
>>>> +void virtio_bus_new(VirtioBus *bus, DeviceState *host,
>>>> + const VirtioBusInfo *info);
>>>> +void virtio_bus_bind_device(VirtioBus *bus);
>>>> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *dev);
>>>> +void virtio_bus_exit_cb(VirtioBus *bus);
>>>> +
>>>> +#endif
>>>> --
>>>> 1.7.11.7
>>>> -- PMM
>>>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus.
2012-11-20 16:45 ` KONRAD Frédéric
@ 2012-11-20 17:27 ` Cornelia Huck
0 siblings, 0 replies; 15+ messages in thread
From: Cornelia Huck @ 2012-11-20 17:27 UTC (permalink / raw)
To: KONRAD Frédéric
Cc: Peter Maydell, Anthony Liguori, Evgeny Voevodin, mark.burton,
qemu-devel, Andreas Färber
On Tue, 20 Nov 2012 17:45:15 +0100
KONRAD Frédéric <fred.konrad@greensocs.com> wrote:
> eg for virtio-block-pci in virtio-pci.c :
> static void virtio_blk_class_init(ObjectClass *klass, void *data)
> {
> k->device_id = PCI_DEVICE_ID_VIRTIO_BLOCK;
> ...
> k->class_id = PCI_CLASS_STORAGE_SCSI;
> }
>
> I think that the better solution is to put these value in a big switch
> case :
>
> eg :
>
> Adding that in virtio-bus.c :
>
> /* Return the virtio device id of the plugged device. */
> uint16_t get_virtio_device_id(VirtioBus *bus)
> {
> return bus->vdev->device_id;
> }
Yes, we'll need this for virtio-ccw as well (the id is used as the CU
model).
>
> Using that in virtio-pci transport initialisation.
>
> switch (get_virtio_device_id(&proxy->bus))
> {
> case VIRTIO_ID_BLOCK:
> pci_config_set_device_id(proxy->pci_dev.config,
> PCI_DEVICE_ID_VIRTIO_BLOCK);
> pci_config_set_class(proxy->pci_dev.config,
> PCI_CLASS_STORAGE_SCSI);
> break;
> default:
> error_report("unknown device id\n");
> break;
> }
>
> but the transport stay ( a little ) device dependent.
Well, given that the device id _is_ device dependent, doing the device
id -> transport specific id transition in the transport code seems
sensible.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus.
2012-11-19 17:33 ` Peter Maydell
2012-11-20 14:12 ` Cornelia Huck
@ 2012-11-21 9:31 ` KONRAD Frédéric
1 sibling, 0 replies; 15+ messages in thread
From: KONRAD Frédéric @ 2012-11-21 9:31 UTC (permalink / raw)
To: Peter Maydell
Cc: Anthony Liguori, Evgeny Voevodin, mark.burton, qemu-devel,
Cornelia Huck, Andreas Färber
On 19/11/2012 18:33, Peter Maydell wrote:
> On 16 November 2012 15:35, <fred.konrad@greensocs.com> wrote:
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
> You forgot to CC enough people...
>
>> This patch create a new VirtioBus, which can be added to Virtio transports like
>> virtio-pci, virtio-mmio,...
>>
>> One VirtIODevice can be connected to this device, like virtio-blk in the 3rd
>> patch.
>>
>> The VirtioBus shares through a VirtioBusInfo structure :
>>
>> * two callbacks with the transport : init_cb and exit_cb, which must be
>> called by the VirtIODevice, after the initialization and before the
>> destruction, to put the right PCI IDs and/or stop the event fd.
>>
>> * a VirtIOBindings structure.
>>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> ---
>> hw/Makefile.objs | 1 +
>> hw/virtio-bus.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> hw/virtio-bus.h | 49 ++++++++++++++++++++++++
>> 3 files changed, 161 insertions(+)
>> create mode 100644 hw/virtio-bus.c
>> create mode 100644 hw/virtio-bus.h
>>
>> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
>> index af4ab0c..1b25d77 100644
>> --- a/hw/Makefile.objs
>> +++ b/hw/Makefile.objs
>> @@ -1,6 +1,7 @@
>> common-obj-y = usb/ ide/
>> common-obj-y += loader.o
>> common-obj-$(CONFIG_VIRTIO) += virtio-console.o
>> +common-obj-$(CONFIG_VIRTIO) += virtio-bus.o
>> common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
>> common-obj-y += fw_cfg.o
>> common-obj-$(CONFIG_PCI) += pci.o pci_bridge.o pci_bridge_dev.o
>> diff --git a/hw/virtio-bus.c b/hw/virtio-bus.c
>> new file mode 100644
>> index 0000000..b2e7e9c
>> --- /dev/null
>> +++ b/hw/virtio-bus.c
>> @@ -0,0 +1,111 @@
>> +/*
>> + * VirtioBus
>> + *
>> + * Copyright (C) 2012 : GreenSocs Ltd
>> + * http://www.greensocs.com/ , email: info@greensocs.com
>> + *
>> + * Developed by :
>> + * Frederic Konrad <fred.konrad@greensocs.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2. See
>> + * the COPYING file in the top-level directory.
> Unless you copied this code from an existing v2-only file, preferred
> license for new code is v2-or-any-later-version.
>
>> + */
>> +
>> +#include "hw.h"
>> +#include "qemu-error.h"
>> +#include "qdev.h"
>> +#include "virtio-bus.h"
>> +#include "virtio.h"
>> +
>> +#define DEBUG_VIRTIO_BUS
>> +
>> +#ifdef DEBUG_VIRTIO_BUS
>> +
>> +#define DPRINTF(fmt, ...) \
>> +do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
>> +#else
>> +#define DPRINTF(fmt, ...) do {} while (0)
>> +#endif
>> +
>> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev);
> Is this function necessary? I think your implementation
> is doing the same as the default.
>
>> +static void virtio_bus_class_init(ObjectClass *klass, void *data)
>> +{
>> + BusClass *k = BUS_CLASS(klass);
>> + k->get_fw_dev_path = virtio_bus_get_fw_dev_path;
>> +}
>> +
>> +static const TypeInfo virtio_bus_info = {
>> + .name = TYPE_VIRTIO_BUS,
>> + .parent = TYPE_BUS,
>> + .instance_size = sizeof(VirtioBus),
>> + .class_init = virtio_bus_class_init,
>> +};
>> +
>> +/* VirtioBus */
>> +
>> +static int next_virtio_bus;
>> +
>> +/* Create a virtio bus, and attach to transport. */
>> +void virtio_bus_new(VirtioBus *bus, DeviceState *host,
>> + const VirtioBusInfo *info)
>> +{
>> + /*
>> + * Setting name to NULL return always "virtio.0"
>> + * as bus name in info qtree.
>> + */
>> + char *bus_name = g_strdup_printf("%s.%d", BUS_NAME, next_virtio_bus);
>> + qbus_create_inplace(&bus->qbus, TYPE_VIRTIO_BUS, host, bus_name);
>> + bus->busnr = next_virtio_bus++;
> This looks suspicious to me -- is keeping a count of the global
> bus number really the right way to do this?
>
>> + bus->info = info;
>> + /* no hotplug for the moment ? */
>> + bus->qbus.allow_hotplug = 0;
> Correct -- we don't need to hotplug the virtio backend.
>
>> + bus->bus_in_use = false;
>> + DPRINTF("bus %s created\n", bus_name);
>> +}
>> +
>> +/* Bind the VirtIODevice to the VirtioBus. */
>> +void virtio_bus_bind_device(VirtioBus *bus)
>> +{
>> + assert(bus != NULL);
>> + assert(bus->vdev != NULL);
>> + virtio_bind_device(bus->vdev, &(bus->info->virtio_bindings),
>> + bus->qbus.parent);
> This looks wrong -- virtio_bind_device() is part of the
> old-style transport API.
it is part of the virtiodevice API, I don't think It is a good idea to
modify it ?
>> +}
>> +
>> +/* This must be called to when the VirtIODevice init */
>> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *vdev)
>> +{
>> + if (bus->bus_in_use == true) {
>> + error_report("%s in use.\n", bus->qbus.name);
>> + return -1;
>> + }
>> + assert(bus->info->init_cb != NULL);
>> + /* keep the VirtIODevice in the VirtioBus */
>> + bus->vdev = vdev;
> This shouldn't be happening here, it should happen as
> part of plugging the device into the bus.
What do you mean by plugging the device into the bus ?
I think that the device is already plugged when I call this function, I
don't find an
other idea to set maximum device per bus to 1.
>
>> + bus->info->init_cb(bus->qbus.parent);
>> +
>> + bus->bus_in_use = true;
>> + return 0;
>> +}
>> +
>> +/* This must be called when the VirtIODevice exit */
>> +void virtio_bus_exit_cb(VirtioBus *bus)
>> +{
>> + assert(bus->info->exit_cb != NULL);
>> + bus->info->exit_cb(bus->qbus.parent);
>> + bus->bus_in_use = false;
>> +}
> These shouldn't be necessary -- the VirtioDevice should
> have a pointer to the VirtioBus and can just invoke the
> init/exit callbacks directly.
>
>> +
>> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev)
>> +{
>> + return g_strdup_printf("%s", qdev_fw_name(dev));
>> +}
>> +
>> +
>> +static void virtio_register_types(void)
>> +{
>> + type_register_static(&virtio_bus_info);
>> +}
>> +
>> +type_init(virtio_register_types)
>> diff --git a/hw/virtio-bus.h b/hw/virtio-bus.h
>> new file mode 100644
>> index 0000000..6ea5035
>> --- /dev/null
>> +++ b/hw/virtio-bus.h
>> @@ -0,0 +1,49 @@
>> +/*
>> + * VirtioBus
>> + *
>> + * Copyright (C) 2012 : GreenSocs Ltd
>> + * http://www.greensocs.com/ , email: info@greensocs.com
>> + *
>> + * Developed by :
>> + * Frederic Konrad <fred.konrad@greensocs.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2. See
>> + * the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef _VIRTIO_BUS_H_
>> +#define _VIRTIO_BUS_H_
> Leading underscores are reserved by the C standard. VIRTIO_BUS_H will
> do fine.
>
>> +
>> +#include "qdev.h"
>> +#include "sysemu.h"
>> +#include "virtio.h"
>> +
>> +#define TYPE_VIRTIO_BUS "VIRTIO"
> Type names are generally "virtio-bus" by convention.
>
>> +#define BUS_NAME "virtio"
> I don't think you want this.
>
>
>> +typedef struct VirtioBus VirtioBus;
>> +typedef struct VirtioBusInfo VirtioBusInfo;
>> +
>> +struct VirtioBusInfo {
>> + void (*init_cb)(DeviceState *dev);
>> + void (*exit_cb)(DeviceState *dev);
>> + VirtIOBindings virtio_bindings;
> This doesn't look right -- VirtIOBindings is the
> old-style way for the transport to specify a bunch
> of function pointers for its specific implementation.
> Those function pointers should probably just be in
> the VirtioBus struct.
>
>> +};
> I was just talking to Anthony on IRC and he said SCSIBusInfo
> is really kind of a historical accident. So we can just fold
> this struct into VirtioBus. Sorry for misleading you earlier.
>
>> +struct VirtioBus {
>> + BusState qbus;
>> + int busnr;
> Why does the bus need to know what number it is?
>
>> + bool bus_in_use;
>> + uint16_t pci_device_id;
>> + uint16_t pci_class;
> This shouldn't be here -- VirtioBus should be transport
> independent, so no PCI related info.
>
>> + VirtIODevice *vdev;
> This could use a comment saying that we only ever have one
> child device on the bus.
>
>> + const VirtioBusInfo *info;
>> +};
>> +
>> +void virtio_bus_new(VirtioBus *bus, DeviceState *host,
>> + const VirtioBusInfo *info);
>> +void virtio_bus_bind_device(VirtioBus *bus);
>> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *dev);
>> +void virtio_bus_exit_cb(VirtioBus *bus);
>> +
>> +#endif
>> --
>> 1.7.11.7
> -- PMM
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus.
2012-11-16 15:35 ` [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus fred.konrad
2012-11-19 17:33 ` Peter Maydell
@ 2012-11-21 13:04 ` Andreas Färber
2012-11-21 14:05 ` KONRAD Frédéric
1 sibling, 1 reply; 15+ messages in thread
From: Andreas Färber @ 2012-11-21 13:04 UTC (permalink / raw)
To: fred.konrad
Cc: Peter Maydell, Evgeny Voevodin, mark.burton, qemu-devel,
Anthony Liguori, Cornelia Huck
Am 16.11.2012 16:35, schrieb fred.konrad@greensocs.com:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> This patch create a new VirtioBus, which can be added to Virtio transports like
> virtio-pci, virtio-mmio,...
>
> One VirtIODevice can be connected to this device, like virtio-blk in the 3rd
> patch.
>
> The VirtioBus shares through a VirtioBusInfo structure :
>
> * two callbacks with the transport : init_cb and exit_cb, which must be
> called by the VirtIODevice, after the initialization and before the
> destruction, to put the right PCI IDs and/or stop the event fd.
>
> * a VirtIOBindings structure.
>
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> ---
> hw/Makefile.objs | 1 +
> hw/virtio-bus.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> hw/virtio-bus.h | 49 ++++++++++++++++++++++++
> 3 files changed, 161 insertions(+)
> create mode 100644 hw/virtio-bus.c
> create mode 100644 hw/virtio-bus.h
>
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index af4ab0c..1b25d77 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -1,6 +1,7 @@
> common-obj-y = usb/ ide/
> common-obj-y += loader.o
> common-obj-$(CONFIG_VIRTIO) += virtio-console.o
> +common-obj-$(CONFIG_VIRTIO) += virtio-bus.o
> common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
> common-obj-y += fw_cfg.o
> common-obj-$(CONFIG_PCI) += pci.o pci_bridge.o pci_bridge_dev.o
> diff --git a/hw/virtio-bus.c b/hw/virtio-bus.c
> new file mode 100644
> index 0000000..b2e7e9c
> --- /dev/null
> +++ b/hw/virtio-bus.c
> @@ -0,0 +1,111 @@
> +/*
> + * VirtioBus
> + *
> + * Copyright (C) 2012 : GreenSocs Ltd
> + * http://www.greensocs.com/ , email: info@greensocs.com
> + *
> + * Developed by :
> + * Frederic Konrad <fred.konrad@greensocs.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
Any chance to use GPLv2 "or (at your option) any later version"? If not,
please mention in the commit message why.
> + */
> +
> +#include "hw.h"
> +#include "qemu-error.h"
> +#include "qdev.h"
> +#include "virtio-bus.h"
> +#include "virtio.h"
> +
> +#define DEBUG_VIRTIO_BUS
> +
> +#ifdef DEBUG_VIRTIO_BUS
> +
> +#define DPRINTF(fmt, ...) \
> +do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) do {} while (0)
> +#endif
We recently had a discussion about bitrotting DPRINTF() statements where
I suggested to use if (0) instead of a no-op macro like this that
doesn't reference fmt and the varargs.
> +
> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev);
> +
> +static void virtio_bus_class_init(ObjectClass *klass, void *data)
> +{
> + BusClass *k = BUS_CLASS(klass);
> + k->get_fw_dev_path = virtio_bus_get_fw_dev_path;
> +}
> +
> +static const TypeInfo virtio_bus_info = {
> + .name = TYPE_VIRTIO_BUS,
> + .parent = TYPE_BUS,
> + .instance_size = sizeof(VirtioBus),
> + .class_init = virtio_bus_class_init,
> +};
> +
> +/* VirtioBus */
> +
> +static int next_virtio_bus;
> +
> +/* Create a virtio bus, and attach to transport. */
> +void virtio_bus_new(VirtioBus *bus, DeviceState *host,
> + const VirtioBusInfo *info)
> +{
> + /*
> + * Setting name to NULL return always "virtio.0"
> + * as bus name in info qtree.
> + */
> + char *bus_name = g_strdup_printf("%s.%d", BUS_NAME, next_virtio_bus);
> + qbus_create_inplace(&bus->qbus, TYPE_VIRTIO_BUS, host, bus_name);
Accessing the qbus field directly is considered old-style. Please use
the BUS() macro to cast to a new BusState* variable and pass that here...
> + bus->busnr = next_virtio_bus++;
> + bus->info = info;
> + /* no hotplug for the moment ? */
> + bus->qbus.allow_hotplug = 0;
...and access its fields through it. More cases below.
> + bus->bus_in_use = false;
> + DPRINTF("bus %s created\n", bus_name);
> +}
> +
> +/* Bind the VirtIODevice to the VirtioBus. */
> +void virtio_bus_bind_device(VirtioBus *bus)
> +{
> + assert(bus != NULL);
> + assert(bus->vdev != NULL);
> + virtio_bind_device(bus->vdev, &(bus->info->virtio_bindings),
> + bus->qbus.parent);
> +}
> +
> +/* This must be called to when the VirtIODevice init */
"called when the ...Device inits" or "called during ...Device init"
> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *vdev)
> +{
> + if (bus->bus_in_use == true) {
Even if bus_in_use is of bool type, doing an explicit "true" comparison
is not a good idea in C since everything non-zero is to be considered
true, not just the "true" constant.
> + error_report("%s in use.\n", bus->qbus.name);
> + return -1;
> + }
> + assert(bus->info->init_cb != NULL);
> + /* keep the VirtIODevice in the VirtioBus */
> + bus->vdev = vdev;
> + bus->info->init_cb(bus->qbus.parent);
> +
> + bus->bus_in_use = true;
> + return 0;
> +}
> +
> +/* This must be called when the VirtIODevice exit */
Similar grammar issue as above.
> +void virtio_bus_exit_cb(VirtioBus *bus)
> +{
> + assert(bus->info->exit_cb != NULL);
> + bus->info->exit_cb(bus->qbus.parent);
> + bus->bus_in_use = false;
> +}
> +
> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev)
> +{
> + return g_strdup_printf("%s", qdev_fw_name(dev));
> +}
> +
> +
> +static void virtio_register_types(void)
> +{
> + type_register_static(&virtio_bus_info);
> +}
> +
> +type_init(virtio_register_types)
> diff --git a/hw/virtio-bus.h b/hw/virtio-bus.h
> new file mode 100644
> index 0000000..6ea5035
> --- /dev/null
> +++ b/hw/virtio-bus.h
> @@ -0,0 +1,49 @@
> +/*
> + * VirtioBus
> + *
> + * Copyright (C) 2012 : GreenSocs Ltd
> + * http://www.greensocs.com/ , email: info@greensocs.com
> + *
> + * Developed by :
> + * Frederic Konrad <fred.konrad@greensocs.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#ifndef _VIRTIO_BUS_H_
> +#define _VIRTIO_BUS_H_
> +
> +#include "qdev.h"
> +#include "sysemu.h"
> +#include "virtio.h"
> +
> +#define TYPE_VIRTIO_BUS "VIRTIO"
Is there a special reason for all-uppercase here? Is lowercase already
taken?
You should add QOM cast macros VIRTIO_BUS() et al. to access the
VirtioBus fields.
> +#define BUS_NAME "virtio"
> +
> +typedef struct VirtioBus VirtioBus;
> +typedef struct VirtioBusInfo VirtioBusInfo;
> +
> +struct VirtioBusInfo {
> + void (*init_cb)(DeviceState *dev);
> + void (*exit_cb)(DeviceState *dev);
> + VirtIOBindings virtio_bindings;
> +};
> +
> +struct VirtioBus {
> + BusState qbus;
You could name this field parent_obj to break the old qbus pattern.
> + int busnr;
> + bool bus_in_use;
> + uint16_t pci_device_id;
> + uint16_t pci_class;
pci_* in VirtioBus does not strike me as the best naming. Either this is
specific to the PCI backend and doesn't belong here, or it's not a
pci_device_id but a device_id.
> + VirtIODevice *vdev;
> + const VirtioBusInfo *info;
> +};
> +
> +void virtio_bus_new(VirtioBus *bus, DeviceState *host,
> + const VirtioBusInfo *info);
> +void virtio_bus_bind_device(VirtioBus *bus);
> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *dev);
> +void virtio_bus_exit_cb(VirtioBus *bus);
> +
> +#endif
>
Regards,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus.
2012-11-21 13:04 ` Andreas Färber
@ 2012-11-21 14:05 ` KONRAD Frédéric
2012-11-21 14:13 ` Andreas Färber
0 siblings, 1 reply; 15+ messages in thread
From: KONRAD Frédéric @ 2012-11-21 14:05 UTC (permalink / raw)
To: Andreas Färber
Cc: Peter Maydell, Evgeny Voevodin, mark.burton, qemu-devel,
Anthony Liguori, Cornelia Huck
On 21/11/2012 14:04, Andreas Färber wrote:
> Am 16.11.2012 16:35, schrieb fred.konrad@greensocs.com:
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> This patch create a new VirtioBus, which can be added to Virtio transports like
>> virtio-pci, virtio-mmio,...
>>
>> One VirtIODevice can be connected to this device, like virtio-blk in the 3rd
>> patch.
>>
>> The VirtioBus shares through a VirtioBusInfo structure :
>>
>> * two callbacks with the transport : init_cb and exit_cb, which must be
>> called by the VirtIODevice, after the initialization and before the
>> destruction, to put the right PCI IDs and/or stop the event fd.
>>
>> * a VirtIOBindings structure.
>>
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> ---
>> hw/Makefile.objs | 1 +
>> hw/virtio-bus.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> hw/virtio-bus.h | 49 ++++++++++++++++++++++++
>> 3 files changed, 161 insertions(+)
>> create mode 100644 hw/virtio-bus.c
>> create mode 100644 hw/virtio-bus.h
>>
>> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
>> index af4ab0c..1b25d77 100644
>> --- a/hw/Makefile.objs
>> +++ b/hw/Makefile.objs
>> @@ -1,6 +1,7 @@
>> common-obj-y = usb/ ide/
>> common-obj-y += loader.o
>> common-obj-$(CONFIG_VIRTIO) += virtio-console.o
>> +common-obj-$(CONFIG_VIRTIO) += virtio-bus.o
>> common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
>> common-obj-y += fw_cfg.o
>> common-obj-$(CONFIG_PCI) += pci.o pci_bridge.o pci_bridge_dev.o
>> diff --git a/hw/virtio-bus.c b/hw/virtio-bus.c
>> new file mode 100644
>> index 0000000..b2e7e9c
>> --- /dev/null
>> +++ b/hw/virtio-bus.c
>> @@ -0,0 +1,111 @@
>> +/*
>> + * VirtioBus
>> + *
>> + * Copyright (C) 2012 : GreenSocs Ltd
>> + * http://www.greensocs.com/ , email: info@greensocs.com
>> + *
>> + * Developed by :
>> + * Frederic Konrad <fred.konrad@greensocs.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2. See
>> + * the COPYING file in the top-level directory.
> Any chance to use GPLv2 "or (at your option) any later version"? If not,
> please mention in the commit message why.
>
Yes, I made the change.
>> + */
>> +
>> +#include "hw.h"
>> +#include "qemu-error.h"
>> +#include "qdev.h"
>> +#include "virtio-bus.h"
>> +#include "virtio.h"
>> +
>> +#define DEBUG_VIRTIO_BUS
>> +
>> +#ifdef DEBUG_VIRTIO_BUS
>> +
>> +#define DPRINTF(fmt, ...) \
>> +do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
>> +#else
>> +#define DPRINTF(fmt, ...) do {} while (0)
>> +#endif
> We recently had a discussion about bitrotting DPRINTF() statements where
> I suggested to use if (0) instead of a no-op macro like this that
> doesn't reference fmt and the varargs.
I don't understand what you suggested, can you point me to an example ?
>> +
>> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev);
>> +
>> +static void virtio_bus_class_init(ObjectClass *klass, void *data)
>> +{
>> + BusClass *k = BUS_CLASS(klass);
>> + k->get_fw_dev_path = virtio_bus_get_fw_dev_path;
>> +}
>> +
>> +static const TypeInfo virtio_bus_info = {
>> + .name = TYPE_VIRTIO_BUS,
>> + .parent = TYPE_BUS,
>> + .instance_size = sizeof(VirtioBus),
>> + .class_init = virtio_bus_class_init,
>> +};
>> +
>> +/* VirtioBus */
>> +
>> +static int next_virtio_bus;
>> +
>> +/* Create a virtio bus, and attach to transport. */
>> +void virtio_bus_new(VirtioBus *bus, DeviceState *host,
>> + const VirtioBusInfo *info)
>> +{
>> + /*
>> + * Setting name to NULL return always "virtio.0"
>> + * as bus name in info qtree.
>> + */
>> + char *bus_name = g_strdup_printf("%s.%d", BUS_NAME, next_virtio_bus);
>> + qbus_create_inplace(&bus->qbus, TYPE_VIRTIO_BUS, host, bus_name);
> Accessing the qbus field directly is considered old-style. Please use
> the BUS() macro to cast to a new BusState* variable and pass that here...
Ok, I'll look.
>> + bus->busnr = next_virtio_bus++;
>> + bus->info = info;
>> + /* no hotplug for the moment ? */
>> + bus->qbus.allow_hotplug = 0;
> ...and access its fields through it. More cases below.
>
>> + bus->bus_in_use = false;
>> + DPRINTF("bus %s created\n", bus_name);
>> +}
>> +
>> +/* Bind the VirtIODevice to the VirtioBus. */
>> +void virtio_bus_bind_device(VirtioBus *bus)
>> +{
>> + assert(bus != NULL);
>> + assert(bus->vdev != NULL);
>> + virtio_bind_device(bus->vdev, &(bus->info->virtio_bindings),
>> + bus->qbus.parent);
>> +}
>> +
>> +/* This must be called to when the VirtIODevice init */
> "called when the ...Device inits" or "called during ...Device init"
oops sorry for that.
>> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *vdev)
>> +{
>> + if (bus->bus_in_use == true) {
> Even if bus_in_use is of bool type, doing an explicit "true" comparison
> is not a good idea in C since everything non-zero is to be considered
> true, not just the "true" constant.
sure.
>> + error_report("%s in use.\n", bus->qbus.name);
>> + return -1;
>> + }
>> + assert(bus->info->init_cb != NULL);
>> + /* keep the VirtIODevice in the VirtioBus */
>> + bus->vdev = vdev;
>> + bus->info->init_cb(bus->qbus.parent);
>> +
>> + bus->bus_in_use = true;
>> + return 0;
>> +}
>> +
>> +/* This must be called when the VirtIODevice exit */
> Similar grammar issue as above.
>
>> +void virtio_bus_exit_cb(VirtioBus *bus)
>> +{
>> + assert(bus->info->exit_cb != NULL);
>> + bus->info->exit_cb(bus->qbus.parent);
>> + bus->bus_in_use = false;
>> +}
>> +
>> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev)
>> +{
>> + return g_strdup_printf("%s", qdev_fw_name(dev));
>> +}
>> +
>> +
>> +static void virtio_register_types(void)
>> +{
>> + type_register_static(&virtio_bus_info);
>> +}
>> +
>> +type_init(virtio_register_types)
>> diff --git a/hw/virtio-bus.h b/hw/virtio-bus.h
>> new file mode 100644
>> index 0000000..6ea5035
>> --- /dev/null
>> +++ b/hw/virtio-bus.h
>> @@ -0,0 +1,49 @@
>> +/*
>> + * VirtioBus
>> + *
>> + * Copyright (C) 2012 : GreenSocs Ltd
>> + * http://www.greensocs.com/ , email: info@greensocs.com
>> + *
>> + * Developed by :
>> + * Frederic Konrad <fred.konrad@greensocs.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2. See
>> + * the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef _VIRTIO_BUS_H_
>> +#define _VIRTIO_BUS_H_
>> +
>> +#include "qdev.h"
>> +#include "sysemu.h"
>> +#include "virtio.h"
>> +
>> +#define TYPE_VIRTIO_BUS "VIRTIO"
> Is there a special reason for all-uppercase here? Is lowercase already
> taken?
No, there are no reasons, it was the case for scsi-bus.
>
> You should add QOM cast macros VIRTIO_BUS() et al. to access the
> VirtioBus fields.
>
>> +#define BUS_NAME "virtio"
>> +
>> +typedef struct VirtioBus VirtioBus;
>> +typedef struct VirtioBusInfo VirtioBusInfo;
>> +
>> +struct VirtioBusInfo {
>> + void (*init_cb)(DeviceState *dev);
>> + void (*exit_cb)(DeviceState *dev);
>> + VirtIOBindings virtio_bindings;
>> +};
>> +
>> +struct VirtioBus {
>> + BusState qbus;
> You could name this field parent_obj to break the old qbus pattern.
>
>> + int busnr;
>> + bool bus_in_use;
>> + uint16_t pci_device_id;
>> + uint16_t pci_class;
> pci_* in VirtioBus does not strike me as the best naming. Either this is
> specific to the PCI backend and doesn't belong here, or it's not a
> pci_device_id but a device_id.
it is specific to the PCI backend, and I removed it.
>
>> + VirtIODevice *vdev;
>> + const VirtioBusInfo *info;
>> +};
>> +
>> +void virtio_bus_new(VirtioBus *bus, DeviceState *host,
>> + const VirtioBusInfo *info);
>> +void virtio_bus_bind_device(VirtioBus *bus);
>> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *dev);
>> +void virtio_bus_exit_cb(VirtioBus *bus);
>> +
>> +#endif
>>
> Regards,
> Andreas
>
Thanks,
Fred
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus.
2012-11-21 14:05 ` KONRAD Frédéric
@ 2012-11-21 14:13 ` Andreas Färber
2012-11-21 14:18 ` KONRAD Frédéric
0 siblings, 1 reply; 15+ messages in thread
From: Andreas Färber @ 2012-11-21 14:13 UTC (permalink / raw)
To: KONRAD Frédéric
Cc: Peter Maydell, Stefan Hajnoczi, Evgeny Voevodin, mark.burton,
qemu-devel, Anthony Liguori, Cornelia Huck
Am 21.11.2012 15:05, schrieb KONRAD Frédéric:
> On 21/11/2012 14:04, Andreas Färber wrote:
>> Am 16.11.2012 16:35, schrieb fred.konrad@greensocs.com:
>>> +#define DEBUG_VIRTIO_BUS
>>> +
>>> +#ifdef DEBUG_VIRTIO_BUS
>>> +
>>> +#define DPRINTF(fmt, ...) \
>>> +do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
>>> +#else
>>> +#define DPRINTF(fmt, ...) do {} while (0)
>>> +#endif
>> We recently had a discussion about bitrotting DPRINTF() statements where
>> I suggested to use if (0) instead of a no-op macro like this that
>> doesn't reference fmt and the varargs.
> I don't understand what you suggested, can you point me to an example ?
I don't have a link at hand, maybe Evgeny does. It was along the lines of:
#define DEBUG_VIRTIO_BUS 0
#define DPRINTF(fmt, ...) if (DEBUG_VIRTIO_BUS) { \
printf("virtio_bus: " fmt , ## __VA_ARGS__); \
}
The officially preferred alternative is to use tracepoints. ;)
Cheers,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus.
2012-11-21 14:13 ` Andreas Färber
@ 2012-11-21 14:18 ` KONRAD Frédéric
0 siblings, 0 replies; 15+ messages in thread
From: KONRAD Frédéric @ 2012-11-21 14:18 UTC (permalink / raw)
To: Andreas Färber
Cc: Peter Maydell, Stefan Hajnoczi, Evgeny Voevodin, mark.burton,
qemu-devel, Anthony Liguori, Cornelia Huck
On 21/11/2012 15:13, Andreas Färber wrote:
> Am 21.11.2012 15:05, schrieb KONRAD Frédéric:
>> On 21/11/2012 14:04, Andreas Färber wrote:
>>> Am 16.11.2012 16:35, schrieb fred.konrad@greensocs.com:
>>>> +#define DEBUG_VIRTIO_BUS
>>>> +
>>>> +#ifdef DEBUG_VIRTIO_BUS
>>>> +
>>>> +#define DPRINTF(fmt, ...) \
>>>> +do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
>>>> +#else
>>>> +#define DPRINTF(fmt, ...) do {} while (0)
>>>> +#endif
>>> We recently had a discussion about bitrotting DPRINTF() statements where
>>> I suggested to use if (0) instead of a no-op macro like this that
>>> doesn't reference fmt and the varargs.
>> I don't understand what you suggested, can you point me to an example ?
> I don't have a link at hand, maybe Evgeny does. It was along the lines of:
>
> #define DEBUG_VIRTIO_BUS 0
>
> #define DPRINTF(fmt, ...) if (DEBUG_VIRTIO_BUS) { \
> printf("virtio_bus: " fmt , ## __VA_ARGS__); \
> }
>
> The officially preferred alternative is to use tracepoints. ;)
>
> Cheers,
> Andreas
>
ok, thanks.
Fred
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2012-11-21 14:18 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-16 15:35 [Qemu-devel] [RFC PATCH 0/3] Virtio refactoring fred.konrad
2012-11-16 15:35 ` [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus fred.konrad
2012-11-19 17:33 ` Peter Maydell
2012-11-20 14:12 ` Cornelia Huck
2012-11-20 14:30 ` KONRAD Frédéric
2012-11-20 16:15 ` Cornelia Huck
2012-11-20 16:45 ` KONRAD Frédéric
2012-11-20 17:27 ` Cornelia Huck
2012-11-21 9:31 ` KONRAD Frédéric
2012-11-21 13:04 ` Andreas Färber
2012-11-21 14:05 ` KONRAD Frédéric
2012-11-21 14:13 ` Andreas Färber
2012-11-21 14:18 ` KONRAD Frédéric
2012-11-16 15:35 ` [Qemu-devel] [RFC PATCH 2/3] virtio-pci : Add a virtio-bus interface fred.konrad
2012-11-16 15:35 ` [Qemu-devel] [RFC PATCH 3/3] virtio-blk : add the virtio-blk device fred.konrad
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.