qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: KONRAD Frédéric
Subject: Re: [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus.
Date: Wed, 21 Nov 2012 10:31:44 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:16.0) Gecko/20121029 Thunderbird/16.0.2

On 19/11/2012 18:33, Peter Maydell wrote:
On 16 November 2012 15:35,  <address@hidden> wrote:
From: KONRAD Frederic <address@hidden>
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 <address@hidden>
---
  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: address@hidden
+ *
+ *  Developed by :
+ *  Frederic Konrad   <address@hidden>
+ *
+ * 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: address@hidden
+ *
+ *  Developed by :
+ *  Frederic Konrad   <address@hidden>
+ *
+ * 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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]