From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37190) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tb9yi-000438-Pf for qemu-devel@nongnu.org; Wed, 21 Nov 2012 08:04:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tb9yg-0003ky-Ml for qemu-devel@nongnu.org; Wed, 21 Nov 2012 08:04:32 -0500 Received: from cantor2.suse.de ([195.135.220.15]:52238 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tb9yg-0003jg-9h for qemu-devel@nongnu.org; Wed, 21 Nov 2012 08:04:30 -0500 Message-ID: <50ACD157.6010100@suse.de> Date: Wed, 21 Nov 2012 14:04:23 +0100 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= 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: <1353080159-10536-2-git-send-email-fred.konrad@greensocs.com> Content-Type: text/plain; charset=ISO-8859-15 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: fred.konrad@greensocs.com Cc: Peter Maydell , Evgeny Voevodin , mark.burton@greensocs.com, qemu-devel@nongnu.org, Anthony Liguori , Cornelia Huck Am 16.11.2012 16:35, schrieb fred.konrad@greensocs.com: > From: KONRAD Frederic >=20 > This patch create a new VirtioBus, which can be added to Virtio transpo= rts like > virtio-pci, virtio-mmio,... >=20 > One VirtIODevice can be connected to this device, like virtio-blk in th= e 3rd > patch. >=20 > The VirtioBus shares through a VirtioBusInfo structure : >=20 > * two callbacks with the transport : init_cb and exit_cb, which mus= t be > called by the VirtIODevice, after the initialization and before t= he > destruction, to put the right PCI IDs and/or stop the event fd. >=20 > * a VirtIOBindings structure. >=20 > 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 >=20 > 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. S= ee > + * 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 =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_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 =3D next_virtio_bus++; > + bus->info =3D info; > + /* no hotplug for the moment ? */ > + bus->qbus.allow_hotplug =3D 0; ...and access its fields through it. More cases below. > + 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 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 =3D=3D 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 !=3D NULL); > + /* keep the VirtIODevice in the VirtioBus */ > + bus->vdev =3D vdev; > + bus->info->init_cb(bus->qbus.parent); > + > + bus->bus_in_use =3D 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 !=3D NULL); > + bus->info->exit_cb(bus->qbus.parent); > + bus->bus_in_use =3D 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 > + * > + * This work is licensed under the terms of the GNU GPL, version 2. S= ee > + * 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 >=20 Regards, Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg