On 20/11/2012 17:15, Cornelia Huck wrote:
On Tue, 20 Nov 2012 15:30:35 +0100
KONRAD Frédéric <fred.kon...@greensocs.com> wrote:
On 20/11/2012 15:12, Cornelia Huck wrote:
On Mon, 19 Nov 2012 17:33:01 +0000
Peter Maydell <peter.mayd...@linaro.org> wrote:
On 16 November 2012 15:35, <fred.kon...@greensocs.com> wrote:
From: KONRAD Frederic <fred.kon...@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.kon...@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: i...@greensocs.com
+ *
+ * Developed by :
+ * Frederic Konrad <fred.kon...@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: i...@greensocs.com
+ *
+ * Developed by :
+ * Frederic Konrad <fred.kon...@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
