Re: [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus.

[Cornelia Huck]

On Tue, 20 Nov 2012 15:30:35 +0100
KONRAD Frédéric <address@hidden> wrote:

> On 20/11/2012 15:12, Cornelia Huck wrote:
> > On Mon, 19 Nov 2012 17:33:01 +0000
> > Peter Maydell <address@hidden> wrote:
> >
> >> On 16 November 2012 15:35,  <address@hidden> wrote:
> >>> From: KONRAD Frederic <address@hidden>
> >> You forgot to CC enough people...
> >>
> >>> 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.
> > 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 used.
> 
> 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.

> 
> >
> >>>      * a VirtIOBindings structure.
> >>>
> >>> Signed-off-by: KONRAD Frederic <address@hidden>
> >>> ---
> >>>   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 = usb/ ide/
> >>>   common-obj-y += loader.o
> >>>   common-obj-$(CONFIG_VIRTIO) += virtio-console.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..b2e7e9c
> >>> --- /dev/null
> >>> +++ b/hw/virtio-bus.c
> >>> @@ -0,0 +1,111 @@
> >>> +/*
> >>> + * VirtioBus
> >>> + *
> >>> + *  Copyright (C) 2012 : GreenSocs Ltd
> >>> + *      http://www.greensocs.com/ , email: address@hidden
> >>> + *
> >>> + *  Developed by :
> >>> + *  Frederic Konrad   <address@hidden>
> >>> + *
> >>> + * 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 = BUS_CLASS(klass);
> >>> +    k->get_fw_dev_path = virtio_bus_get_fw_dev_path;
> >>> +}
> >>> +
> >>> +static const TypeInfo virtio_bus_info = {
> >>> +    .name = TYPE_VIRTIO_BUS,
> >>> +    .parent = TYPE_BUS,
> >>> +    .instance_size = sizeof(VirtioBus),
> >>> +    .class_init = 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 = g_strdup_printf("%s.%d", BUS_NAME, next_virtio_bus);
> >>> +    qbus_create_inplace(&bus->qbus, TYPE_VIRTIO_BUS, host, bus_name);
> >>> +    bus->busnr = 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.

> 
> >>> +    bus->info = info;
> >>> +    /* no hotplug for the moment ? */
> >>> +    bus->qbus.allow_hotplug = 0;
> >> Correct -- we don't need to hotplug the virtio backend.
> >>
> >>> +    bus->bus_in_use = false;
> >>> +    DPRINTF("bus %s created\n", bus_name);
> >>> +}
> >>> +
> >>> +/* Bind the VirtIODevice to the VirtioBus. */
> >>> +void virtio_bus_bind_device(VirtioBus *bus)
> >>> +{
> >>> +    assert(bus != NULL);
> >>> +    assert(bus->vdev != 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 == true) {
> >>> +        error_report("%s in use.\n", bus->qbus.name);
> >>> +        return -1;
> >>> +    }
> >>> +    assert(bus->info->init_cb != NULL);
> >>> +    /* keep the VirtIODevice in the VirtioBus */
> >>> +    bus->vdev = 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 = true;
> >>> +    return 0;
> >>> +}
> >>> +
> >>> +/* This must be called when the VirtIODevice exit */
> >>> +void virtio_bus_exit_cb(VirtioBus *bus)
> >>> +{
> >>> +    assert(bus->info->exit_cb != NULL);
> >>> +    bus->info->exit_cb(bus->qbus.parent);
> >>> +    bus->bus_in_use = 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: address@hidden
> >>> + *
> >>> + *  Developed by :
> >>> + *  Frederic Konrad   <address@hidden>
> >>> + *
> >>> + * 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 
> 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?

> 
> >>> +    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
> >>
>