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. > > * 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? > + 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. > + 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
