From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56291) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Td0gF-0000u8-QM for qemu-devel@nongnu.org; Mon, 26 Nov 2012 10:33:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Td0gD-00047p-Mk for qemu-devel@nongnu.org; Mon, 26 Nov 2012 10:33:07 -0500 Received: from greensocs.com ([87.106.252.221]:49901 helo=s15328186.onlinehome-server.info) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Td0gD-00047j-Ci for qemu-devel@nongnu.org; Mon, 26 Nov 2012 10:33:05 -0500 Message-ID: <50B38BC0.1030402@greensocs.com> Date: Mon, 26 Nov 2012 16:33:20 +0100 From: Konrad Frederic MIME-Version: 1.0 References: <1353595852-30776-1-git-send-email-fred.konrad@greensocs.com> <1353595852-30776-2-git-send-email-fred.konrad@greensocs.com> <87k3t8qvrg.fsf@codemonkey.ws> In-Reply-To: <87k3t8qvrg.fsf@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH v2 1/3] virtio-bus : Introduce VirtioBus. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: peter.maydell@linaro.org, e.voevodin@samsung.com, mark.burton@greensocs.com, qemu-devel@nongnu.org, stefanha@redhat.com, cornelia.huck@de.ibm.com, afaerber@suse.de On 26/11/2012 15:33, Anthony Liguori wrote: > fred.konrad@greensocs.com writes: > >> From: KONRAD Frederic >> >> 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 | 148 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> hw/virtio-bus.h | 58 ++++++++++++++++++++++ >> 3 files changed, 207 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 ea46f81..bd14d1b 100644 >> --- a/hw/Makefile.objs >> +++ b/hw/Makefile.objs >> @@ -2,6 +2,7 @@ common-obj-y = usb/ ide/ >> common-obj-y += loader.o >> common-obj-$(CONFIG_VIRTIO) += virtio-console.o >> common-obj-$(CONFIG_VIRTIO) += virtio-rng.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..991b6f5 >> --- /dev/null >> +++ b/hw/virtio-bus.c >> @@ -0,0 +1,148 @@ >> +/* >> + * VirtioBus >> + * >> + * Copyright (C) 2012 : GreenSocs Ltd >> + * http://www.greensocs.com/ , email: info@greensocs.com >> + * >> + * Developed by : >> + * Frederic Konrad >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation, either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License along >> + * with this program; if not, see. >> + * >> + */ >> + >> +#include "hw.h" >> +#include "qemu-error.h" >> +#include "qdev.h" >> +#include "virtio-bus.h" >> +#include "virtio.h" >> + >> +#define DEBUG_VIRTIO_BUS 1 >> + >> +#define DPRINTF(fmt, ...) if (DEBUG_VIRTIO_BUS) { \ >> + printf("virtio_bus: " fmt , ## __VA_ARGS__); \ >> + } > #ifdef DEBUG_VIRTIO_BUS > #define DPRINTF(fmt, ...) ... > #else > #define DPRINTF(fmt, ...) do { } while (0) > #endif > > You're leaving a dangling if clause which can do very strange things if > used before an else statement. > >> + >> +static void virtio_bus_init_cb(VirtioBus *bus); >> +static int virtio_bus_reset(BusState *qbus); > You should rearrange the code to avoid forward declarations of static functions. > >> + >> +static void virtio_bus_class_init(ObjectClass *klass, void *data) >> +{ >> + BusClass *k = BUS_CLASS(klass); >> + k->reset = virtio_bus_reset; >> +} >> + >> +static TypeInfo virtio_bus_info = { >> + .name = TYPE_VIRTIO_BUS, >> + .parent = TYPE_BUS, >> + .instance_size = sizeof(VirtioBus), >> + .class_init = virtio_bus_class_init, >> +}; >> + >> +/* Reset the bus */ >> +static int virtio_bus_reset(BusState *qbus) >> +{ >> + VirtioBus *bus = VIRTIO_BUS(qbus); >> + if (bus->bus_in_use) { >> + virtio_reset(bus->vdev); >> + } >> + return 1; >> +} >> + >> +/* Plug the VirtIODevice */ >> +int virtio_bus_plug_device(VirtioBus *bus, VirtIODevice *vdev) >> +{ >> + BusState *qbus = BUS(bus); >> + /* >> + * This is a problem, when bus= option is not set, the last created >> + * virtio-bus is used. So it give the following error. >> + */ >> + DPRINTF("plug device into %s.\n", qbus->name); >> + if (bus->bus_in_use) { >> + error_report("%s in use.\n", qbus->name); >> + return -1; >> + } >> + bus->bus_in_use = true; >> + >> + /* keep the VirtIODevice in the VirtioBus. */ >> + bus->vdev = vdev; >> + >> + /* call the "transport" callback. */ >> + virtio_bus_init_cb(bus); >> + return 0; >> +} > I think the desired semantics here could be achieved by modifying > qbus_find_recursive() to take an optional argument which then causes it > to only return a "!full" bus. You can then add a member to BusState to > indicate whether the bus is full or not. > > You can then set the parameter qdev_device_add() and it should Just Work. Ok, so I need to modify qdev_monitor.c to add an option ( in command line ) to set this optional argument ? >> + >> +/* Create a virtio bus. */ >> +VirtioBus *virtio_bus_new(DeviceState *host, const VirtioBusInfo *info) >> +{ >> + /* >> + * This is needed, as we want to have different names for each virtio-bus. >> + * If we don't do that, we can't add more than one VirtIODevice. >> + */ >> + static int next_virtio_bus; >> + char *bus_name = g_strdup_printf("virtio-bus.%d", >> next_virtio_bus++); > It's not needed.. qdev will do this automagically as it turns out. Really ? If I use qbus_create(TYPE_VIRTIO_BUS, host, NULL) and I add two virtio-pci in qemu monitor, I have two virtio-bus named "virtio-bus.0". Did I miss something ? >> + >> + BusState *qbus = qbus_create(TYPE_VIRTIO_BUS, host, bus_name); >> + VirtioBus *bus = VIRTIO_BUS(qbus); >> + bus->info = info; >> + qbus->allow_hotplug = 0; >> + bus->bus_in_use = false; >> + DPRINTF("%s bus created\n", bus_name); >> + return bus; >> +} >> + >> +/* Bind the VirtIODevice to the VirtioBus. */ >> +void virtio_bus_bind_device(VirtioBus *bus) >> +{ >> + BusState *qbus = BUS(bus); >> + assert(bus != NULL); >> + assert(bus->vdev != NULL); >> + virtio_bind_device(bus->vdev,&(bus->info->virtio_bindings), qbus->parent); >> +} >> + >> +/* >> + * Transport independent init. >> + * This must be called after VirtIODevice initialization. >> + */ >> +static void virtio_bus_init_cb(VirtioBus *bus) >> +{ >> + BusState *qbus = BUS(bus); >> + assert(bus->info->init_cb != NULL); >> + bus->info->init_cb(qbus->parent); >> +} >> + >> +/* >> + * Transport independent exit. >> + * This must be called by the VirtIODevice before destroying it. >> + */ >> +void virtio_bus_exit_cb(VirtioBus *bus) >> +{ >> + BusState *qbus = BUS(bus); >> + assert(bus->info->exit_cb != NULL); >> + bus->info->exit_cb(qbus->parent); >> + bus->bus_in_use = false; >> +} >> + >> +/* Return the virtio device id of the plugged device. */ >> +uint16_t get_virtio_device_id(VirtioBus *bus) >> +{ >> + return bus->vdev->device_id; >> +} >> + >> +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..31aad53 >> --- /dev/null >> +++ b/hw/virtio-bus.h >> @@ -0,0 +1,58 @@ >> +/* >> + * VirtioBus >> + * >> + * Copyright (C) 2012 : GreenSocs Ltd >> + * http://www.greensocs.com/ , email: info@greensocs.com >> + * >> + * Developed by : >> + * Frederic Konrad >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation, either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License along >> + * with this program; if not, see. >> + * >> + */ >> + >> +#ifndef VIRTIO_BUS_H >> +#define VIRTIO_BUS_H >> + >> +#include "qdev.h" >> +#include "sysemu.h" >> +#include "virtio.h" >> + >> +#define TYPE_VIRTIO_BUS "virtio-bus" >> +#define VIRTIO_BUS(obj) OBJECT_CHECK(VirtioBus, (obj), TYPE_VIRTIO_BUS) >> + >> +typedef struct VirtioBus VirtioBus; >> +typedef struct VirtioBusInfo VirtioBusInfo; >> + >> +struct VirtioBusInfo { >> + void (*init_cb)(DeviceState *dev); >> + void (*exit_cb)(DeviceState *dev); > Don't suffix methods with '_cb'. > > VirtioBusInfo is not a great name. This is a proxy class that allows > for a device to implement the virtio bus interface. > > This could be done as an interface but since nothing else uses > interfaces, I'm okay with something like this. But the first argument > ought to be an opaque for all methods. > > But it should be called something like VirtioBusProxy. > > BTW, comments describing the role of this struct would be very helpful. > >> + VirtIOBindings virtio_bindings; > Everything that's in VirtIOBindings should be methods of the > VirtioBusClass (which needs to be defined here). Ok, so I can put all the virtio_bindings "methods" in this VirtioBusProxy ? > > Regards, > > Anthony Liguori Thanks, Fred > >> +}; >> + >> +struct VirtioBus { >> + BusState qbus; >> + bool bus_in_use; >> + /* Only one VirtIODevice can be plugged on the bus. */ >> + VirtIODevice *vdev; >> + const VirtioBusInfo *info; >> +}; >> + >> +VirtioBus *virtio_bus_new(DeviceState *host, const VirtioBusInfo *info); >> +void virtio_bus_bind_device(VirtioBus *bus); >> +int virtio_bus_plug_device(VirtioBus *bus, VirtIODevice *vdev); >> +void virtio_bus_exit_cb(VirtioBus *bus); >> +uint16_t get_virtio_device_id(VirtioBus *bus); >> + >> +#endif /* VIRTIO_BUS_H */ >> -- >> 1.7.11.7