From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56560) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tb6ey-0000o3-Jy for qemu-devel@nongnu.org; Wed, 21 Nov 2012 04:32:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tb6es-0001ck-5S for qemu-devel@nongnu.org; Wed, 21 Nov 2012 04:31:56 -0500 Received: from greensocs.com ([87.106.252.221]:35780 helo=s15328186.onlinehome-server.info) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tb6er-0001cW-Rf for qemu-devel@nongnu.org; Wed, 21 Nov 2012 04:31:50 -0500 Message-ID: <50AC9F80.6000203@greensocs.com> Date: Wed, 21 Nov 2012 10:31:44 +0100 From: =?UTF-8?B?S09OUkFEIEZyw6lkw6lyaWM=?= 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> In-Reply-To: Content-Type: text/plain; charset=UTF-8; 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: Peter Maydell Cc: Anthony Liguori , Evgeny Voevodin , mark.burton@greensocs.com, qemu-devel@nongnu.org, Cornelia Huck , =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= On 19/11/2012 18:33, 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. >> >> * 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? > >> + 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 >> + * >> + * 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