From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44362) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TbAvx-0004Ln-DM for qemu-devel@nongnu.org; Wed, 21 Nov 2012 09:05:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TbAvs-00060t-E3 for qemu-devel@nongnu.org; Wed, 21 Nov 2012 09:05:45 -0500 Received: from greensocs.com ([87.106.252.221]:42720 helo=s15328186.onlinehome-server.info) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TbAvs-00060f-3m for qemu-devel@nongnu.org; Wed, 21 Nov 2012 09:05:40 -0500 Message-ID: <50ACDFA8.4040504@greensocs.com> Date: Wed, 21 Nov 2012 15:05:28 +0100 From: =?ISO-8859-15?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> <50ACD157.6010100@suse.de> In-Reply-To: <50ACD157.6010100@suse.de> Content-Type: text/plain; charset=ISO-8859-15; format=flowed 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: =?ISO-8859-15?Q?Andreas_F=E4rber?= Cc: Peter Maydell , Evgeny Voevodin , mark.burton@greensocs.com, qemu-devel@nongnu.org, Anthony Liguori , Cornelia Huck On 21/11/2012 14:04, Andreas F=E4rber wrote: > Am 16.11.2012 16:35, schrieb fred.konrad@greensocs.com: >> From: KONRAD Frederic >> >> This patch create a new VirtioBus, which can be added to Virtio transp= orts like >> virtio-pci, virtio-mmio,... >> >> One VirtIODevice can be connected to this device, like virtio-blk in t= he 3rd >> patch. >> >> The VirtioBus shares through a VirtioBusInfo structure : >> >> * two callbacks with the transport : init_cb and exit_cb, which m= ust 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 =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. > 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 wher= e > 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 =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.= .. Ok, I'll look. >> + 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" oops sorry for that. >> +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. sure. >> + 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. = 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 i= s > 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