From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:36613) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TaqU2-0000GT-Iy for qemu-devel@nongnu.org; Tue, 20 Nov 2012 11:15:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TaqTx-0005uK-55 for qemu-devel@nongnu.org; Tue, 20 Nov 2012 11:15:34 -0500 Received: from e06smtp14.uk.ibm.com ([195.75.94.110]:36758) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TaqTw-0005rv-Mx for qemu-devel@nongnu.org; Tue, 20 Nov 2012 11:15:29 -0500 Received: from /spool/local by e06smtp14.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 20 Nov 2012 16:15:24 -0000 Received: from d06av09.portsmouth.uk.ibm.com (d06av09.portsmouth.uk.ibm.com [9.149.37.250]) by b06cxnps3075.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id qAKGF7Mk33620122 for ; Tue, 20 Nov 2012 16:15:07 GMT Received: from d06av09.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av09.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id qAKGFEOh008676 for ; Tue, 20 Nov 2012 09:15:14 -0700 Date: Tue, 20 Nov 2012 17:15:12 +0100 From: Cornelia Huck Message-ID: <20121120171512.3293afc3@BR9GNB5Z> In-Reply-To: <50AB940B.306@greensocs.com> References: <1353080159-10536-1-git-send-email-fred.konrad@greensocs.com> <1353080159-10536-2-git-send-email-fred.konrad@greensocs.com> <20121120151201.3886aeaf@BR9GNB5Z> <50AB940B.306@greensocs.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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: KONRAD =?UTF-8?B?RnLDqWTDqXJpYw==?= Cc: Peter Maydell , Anthony Liguori , Evgeny Voevodin , mark.burton@greensocs.com, qemu-devel@nongnu.org, Andreas =?UTF-8?B?RsOkcmJlcg==?= On Tue, 20 Nov 2012 15:30:35 +0100 KONRAD Fr=C3=A9d=C3=A9ric wrote: > 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 trans= ports 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 befor= e 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=20 > virtio_blk_init_pci(PCIDevice *pci_dev) in virtio-pci.c, >=20 > it creates the VirtIODevice and then call virtio_init_pci(proxy, vdev); >=20 > It is what I tryed to reproduce, and abstract it from the new device. >=20 > eg : for the new virtio-blk I add, in the function static int=20 > virtio_blk_qdev_init(DeviceState *qdev) in virtio-blk.c : >=20 > it creates the VirtIODevice, create the virtiobus and call the=20 > virtio_bus_init_cb(bus, vdev) > which call the virtio_init_pci callback when virtio-pci bridge is used. >=20 > it is the same thing for the exit. >=20 > 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. >=20 > > > >>> * 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 =3D usb/ ide/ > >>> common-obj-y +=3D loader.o > >>> common-obj-$(CONFIG_VIRTIO) +=3D virtio-console.o > >>> +common-obj-$(CONFIG_VIRTIO) +=3D virtio-bus.o > >>> common-obj-$(CONFIG_VIRTIO_PCI) +=3D virtio-pci.o > >>> common-obj-y +=3D fw_cfg.o > >>> common-obj-$(CONFIG_PCI) +=3D 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 =3D BUS_CLASS(klass); > >>> + k->get_fw_dev_path =3D virtio_bus_get_fw_dev_path; > >>> +} > >>> + > >>> +static const TypeInfo virtio_bus_info =3D { > >>> + .name =3D TYPE_VIRTIO_BUS, > >>> + .parent =3D TYPE_BUS, > >>> + .instance_size =3D sizeof(VirtioBus), > >>> + .class_init =3D 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 =3D g_strdup_printf("%s.%d", BUS_NAME, next_virti= o_bus); > >>> + qbus_create_inplace(&bus->qbus, TYPE_VIRTIO_BUS, host, bus_name); > >>> + bus->busnr =3D 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=20 > 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. >=20 > >>> + bus->info =3D info; > >>> + /* no hotplug for the moment ? */ > >>> + bus->qbus.allow_hotplug =3D 0; > >> Correct -- we don't need to hotplug the virtio backend. > >> > >>> + bus->bus_in_use =3D false; > >>> + DPRINTF("bus %s created\n", bus_name); > >>> +} > >>> + > >>> +/* Bind the VirtIODevice to the VirtioBus. */ > >>> +void virtio_bus_bind_device(VirtioBus *bus) > >>> +{ > >>> + assert(bus !=3D NULL); > >>> + assert(bus->vdev !=3D 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. > >> >=20 >=20 > >>> +} > >>> + > >>> +/* This must be called to when the VirtIODevice init */ > >>> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *vdev) > >>> +{ > >>> + if (bus->bus_in_use =3D=3D true) { > >>> + error_report("%s in use.\n", bus->qbus.name); > >>> + return -1; > >>> + } > >>> + assert(bus->info->init_cb !=3D NULL); > >>> + /* keep the VirtIODevice in the VirtioBus */ > >>> + bus->vdev =3D 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 =3D true; > >>> + return 0; > >>> +} > >>> + > >>> +/* This must be called when the VirtIODevice exit */ > >>> +void virtio_bus_exit_cb(VirtioBus *bus) > >>> +{ > >>> + assert(bus->info->exit_cb !=3D NULL); > >>> + bus->info->exit_cb(bus->qbus.parent); > >>> + bus->bus_in_use =3D 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=20 > 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? >=20 > >>> + 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 > >> >=20