From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:33625) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Taoqk-0001Bx-GT for qemu-devel@nongnu.org; Tue, 20 Nov 2012 09:31:00 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Taoqd-0003uD-Kf for qemu-devel@nongnu.org; Tue, 20 Nov 2012 09:30:54 -0500 Received: from greensocs.com ([87.106.252.221]:39938 helo=s15328186.onlinehome-server.info) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Taoqd-0003u1-1R for qemu-devel@nongnu.org; Tue, 20 Nov 2012 09:30:47 -0500 Message-ID: <50AB940B.306@greensocs.com> Date: Tue, 20 Nov 2012 15:30:35 +0100 From: =?ISO-8859-1?Q?KONRAD_Fr=E9d=E9ric?= MIME-Version: 1.0 References: <1353080159-10536-1-git-send-email-fred.konrad@greensocs.com> <1353080159-10536-2-git-send-email-fred.konrad@greensocs.com> <20121120151201.3886aeaf@BR9GNB5Z> In-Reply-To: <20121120151201.3886aeaf@BR9GNB5Z> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cornelia Huck Cc: Peter Maydell , Anthony Liguori , Evgeny Voevodin , mark.burton@greensocs.com, qemu-devel@nongnu.org, =?ISO-8859-1?Q?Andreas_F=E4rber?= On 20/11/2012 15:12, Cornelia Huck wrote: > On Mon, 19 Nov 2012 17:33:01 +0000 > Peter Maydell wrote: > >> On 16 November 2012 15:35, wrote: >>> From: KONRAD Frederic >> 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 >>> --- >>> 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 >>> + * >>> + * 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 >>> + * >>> + * 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 >>