From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46524) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Taqwz-0006lb-1K for qemu-devel@nongnu.org; Tue, 20 Nov 2012 11:45:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Taqwq-0001VH-RV for qemu-devel@nongnu.org; Tue, 20 Nov 2012 11:45:28 -0500 Received: from greensocs.com ([87.106.252.221]:46477 helo=s15328186.onlinehome-server.info) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Taqwq-0001VD-Ef for qemu-devel@nongnu.org; Tue, 20 Nov 2012 11:45:20 -0500 Message-ID: <50ABB39B.8000101@greensocs.com> Date: Tue, 20 Nov 2012 17:45:15 +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> <20121120151201.3886aeaf@BR9GNB5Z> <50AB940B.306@greensocs.com> <20121120171512.3293afc3@BR9GNB5Z> In-Reply-To: <20121120171512.3293afc3@BR9GNB5Z> Content-Type: text/plain; charset=UTF-8; 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: Cornelia Huck Cc: Peter Maydell , Anthony Liguori , Evgeny Voevodin , mark.burton@greensocs.com, qemu-devel@nongnu.org, =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= On 20/11/2012 17:15, Cornelia Huck wrote: > 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 tra= nsports like >>>>> virtio-pci, virtio-mmio,... >>>>> >>>>> One VirtIODevice can be connected to this device, like virtio-blk i= n the 3rd >>>>> patch. >>>>> >>>>> The VirtioBus shares through a VirtioBusInfo structure : >>>>> >>>>> * two callbacks with the transport : init_cb and exit_cb, whi= ch must be >>>>> called by the VirtIODevice, after the initialization and be= fore 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 >> virtio_blk_init_pci(PCIDevice *pci_dev) in virtio-pci.c, >> >> it creates the VirtIODevice and then call virtio_init_pci(proxy, vdev)= ; >> >> It is what I tryed to reproduce, and abstract it from the new device. >> >> eg : for the new virtio-blk I add, in the function static int >> virtio_blk_qdev_init(DeviceState *qdev) in virtio-blk.c : >> >> it creates the VirtIODevice, create the virtiobus and call the >> virtio_bus_init_cb(bus, vdev) >> which call the virtio_init_pci callback when virtio-pci bridge is u= sed. >> >> it is the same thing for the exit. >> >> 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. Ok, good idea I'll add that. >>>>> * 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_vir= tio_bus); >>>>> + qbus_create_inplace(&bus->qbus, TYPE_VIRTIO_BUS, host, bus_nam= e); >>>>> + 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 >> 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. Ok, it isn't necessary as we don't allow hotplugging. >>>>> + 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. >>>> >> >>>>> +} >>>>> + >>>>> +/* 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 wil= l >>>> 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 i= n >> 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? Yes, it depends on the virtio device id, they are set for the virtio-x-pc= i devices in the init class. eg for virtio-block-pci in virtio-pci.c : static void virtio_blk_class_init(ObjectClass *klass, void *data) { k->device_id =3D PCI_DEVICE_ID_VIRTIO_BLOCK; ... k->class_id =3D PCI_CLASS_STORAGE_SCSI; } I think that the better solution is to put these value in a big switch=20 case : eg : Adding that in virtio-bus.c : /* Return the virtio device id of the plugged device. */ uint16_t get_virtio_device_id(VirtioBus *bus) { return bus->vdev->device_id; } Using that in virtio-pci transport initialisation. switch (get_virtio_device_id(&proxy->bus)) { case VIRTIO_ID_BLOCK: pci_config_set_device_id(proxy->pci_dev.config, PCI_DEVICE_ID_VIRTIO_BLOCK); pci_config_set_class(proxy->pci_dev.config,=20 PCI_CLASS_STORAGE_SCSI); break; default: error_report("unknown device id\n"); break; } but the transport stay ( a little ) device dependent. >>>>> + 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 >>>>