| Message ID | 1353080159-10536-2-git-send-email-fred.konrad@greensocs.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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 >>
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 > >> >
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 >>>>
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.
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
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
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
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
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
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