Patchwork [RFC,v2,1/3] virtio-bus : Introduce VirtioBus.

login
register
mail settings
Submitter fred.konrad@greensocs.com
Date Nov. 22, 2012, 2:50 p.m.
Message ID <1353595852-30776-2-git-send-email-fred.konrad@greensocs.com>
Download mbox | patch
Permalink /patch/201075/
State New
Headers show

Comments

fred.konrad@greensocs.com - Nov. 22, 2012, 2:50 p.m.
From: KONRAD Frederic <fred.konrad@greensocs.com>

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 <fred.konrad@greensocs.com>
---
 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
Cornelia Huck - Nov. 23, 2012, 12:08 p.m.
On Thu, 22 Nov 2012 15:50:50 +0100
fred.konrad@greensocs.com wrote:


> +/* 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++);

This still has the overflow/id-reuse problem, hasn't it?

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

Don't you need a way to destroy the bus again when the proxy device is
hotunplugged?
Stefan Hajnoczi - Nov. 23, 2012, 12:23 p.m.
On Thu, Nov 22, 2012 at 03:50:50PM +0100, fred.konrad@greensocs.com wrote:
> +/* 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);
> +}

Should plug and bind be together in a single function?  Binding the
device seems like an internal step that the bus takes when you plug the
device.

> +struct VirtioBusInfo {

This is defining an ad-hoc interface.  QOM has support for interfaces so
that a virtio-pci adapter brovides a VirtioBindingInterface which
VirtioBus can talk to intead of using VirtioBusInfo.

> +    void (*init_cb)(DeviceState *dev);
> +    void (*exit_cb)(DeviceState *dev);

Can _cb be dropped from the name?  Structs with function pointers always
provide "callbacks" so the _cb is unnecessary.  For example, QOM methods
like BusClass->reset() don't include _cb either.

> +    VirtIOBindings virtio_bindings;
> +};
> +
> +struct VirtioBus {
> +    BusState qbus;
> +    bool bus_in_use;

Should bus_in_use basically be bus->vdev != NULL?
fred.konrad@greensocs.com - Nov. 23, 2012, 2:12 p.m.
On 23/11/2012 13:08, Cornelia Huck wrote:
> On Thu, 22 Nov 2012 15:50:50 +0100
> fred.konrad@greensocs.com wrote:
>
>
>> +/* 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++);
> This still has the overflow/id-reuse problem, hasn't it?
What do you mean by overflow problem ?

>
>> +
>> +    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;
>> +}
> Don't you need a way to destroy the bus again when the proxy device is
> hotunplugged?
>
Yes, you're right I must add a way to destroy the bus, and the devices 
when the proxy is hot unplugged!

Thanks,

Fred
fred.konrad@greensocs.com - Nov. 23, 2012, 2:21 p.m.
On 23/11/2012 13:23, Stefan Hajnoczi wrote:
> On Thu, Nov 22, 2012 at 03:50:50PM +0100, fred.konrad@greensocs.com wrote:
>> +/* 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);
>> +}
> Should plug and bind be together in a single function?  Binding the
> device seems like an internal step that the bus takes when you plug the
> device.
Maybe we can, but we must init the "transport" device before bind the 
device.

>
>> +struct VirtioBusInfo {
> This is defining an ad-hoc interface.  QOM has support for interfaces so
> that a virtio-pci adapter brovides a VirtioBindingInterface which
> VirtioBus can talk to intead of using VirtioBusInfo.
Can you point me to example in the tree to see how this QOM interfaces 
work ?

>> +    void (*init_cb)(DeviceState *dev);
>> +    void (*exit_cb)(DeviceState *dev);
> Can _cb be dropped from the name?  Structs with function pointers always
> provide "callbacks" so the _cb is unnecessary.  For example, QOM methods
> like BusClass->reset() don't include _cb either.
Ok.
>> +    VirtIOBindings virtio_bindings;
>> +};
>> +
>> +struct VirtioBus {
>> +    BusState qbus;
>> +    bool bus_in_use;
> Should bus_in_use basically be bus->vdev != NULL?
Yes I can do that. :).

Thanks,

Fred
Cornelia Huck - Nov. 23, 2012, 2:35 p.m.
On Fri, 23 Nov 2012 15:12:45 +0100
Konrad Frederic <fred.konrad@greensocs.com> wrote:

> On 23/11/2012 13:08, Cornelia Huck wrote:
> > On Thu, 22 Nov 2012 15:50:50 +0100
> > fred.konrad@greensocs.com wrote:
> >
> >
> >> +/* 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++);
> > This still has the overflow/id-reuse problem, hasn't it?
> What do you mean by overflow problem ?

If you do a lot of hotplugs (and hotunplugs), at some point
next_virtio_bus will overflow and the code will start to create
virtio-bus.<existing number>. It's a bit of a pathological case, but I
can see it happening on machines with lots of devices that change
rapidly (such as somebody running a test script doing
device_add/device_del).

Maybe use something like the ida stuff the virtio kernel code is using
(don't know whether something similar exists in qemu).

> 
> >
> >> +
> >> +    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;
> >> +}
> > Don't you need a way to destroy the bus again when the proxy device is
> > hotunplugged?
> >
> Yes, you're right I must add a way to destroy the bus, and the devices 
> when the proxy is hot unplugged!
> 
> Thanks,
> 
> Fred
>
Stefan Hajnoczi - Nov. 23, 2012, 4:13 p.m.
On Fri, Nov 23, 2012 at 03:21:30PM +0100, Konrad Frederic wrote:
> On 23/11/2012 13:23, Stefan Hajnoczi wrote:
> >On Thu, Nov 22, 2012 at 03:50:50PM +0100, fred.konrad@greensocs.com wrote:
> >>+struct VirtioBusInfo {
> >This is defining an ad-hoc interface.  QOM has support for interfaces so
> >that a virtio-pci adapter brovides a VirtioBindingInterface which
> >VirtioBus can talk to intead of using VirtioBusInfo.
> Can you point me to example in the tree to see how this QOM
> interfaces work ?

hw/stream.h and hw/stream.c

A StreamSlave has one method:

    void (*push)(StreamSlave *obj, unsigned char *buf,
                 size_t len, uint32_t *app);

The xlnx.axi-ethernet device in hw/xilinx_axienet.c is an example of a
class that implements the StreamSlave interface.

Stefan
Andreas Färber - Nov. 24, 2012, 10:29 p.m.
Am 22.11.2012 15:50, schrieb fred.konrad@greensocs.com:
> 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
[...]
> +#define DEBUG_VIRTIO_BUS 1

We probably want to disable debug output by default as done elsewhere?

> +
> +#define DPRINTF(fmt, ...) if (DEBUG_VIRTIO_BUS) {                        \
> +                            printf("virtio_bus: " fmt , ## __VA_ARGS__); \
> +                          }
> +
> +static void virtio_bus_init_cb(VirtioBus *bus);
> +static int virtio_bus_reset(BusState *qbus);
> +
> +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 = {

Somehow you lost "const" here since v1.

> +    .name = TYPE_VIRTIO_BUS,
> +    .parent = TYPE_BUS,
> +    .instance_size = sizeof(VirtioBus),
> +    .class_init = virtio_bus_class_init,
> +};

The BUS()-related changes look good, thanks!

Andreas
fred.konrad@greensocs.com - Nov. 26, 2012, 1:55 p.m.
On 23/11/2012 13:08, Cornelia Huck wrote:
> On Thu, 22 Nov 2012 15:50:50 +0100
> fred.konrad@greensocs.com wrote:
>
>
>> +/* 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++);
> This still has the overflow/id-reuse problem, hasn't it?
>
>> +
>> +    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;
>> +}
> Don't you need a way to destroy the bus again when the proxy device is
> hotunplugged?
>
Is the virtio-pci-* proxy currently supporting hotunplugging ?
Cornelia Huck - Nov. 26, 2012, 2:03 p.m.
On Mon, 26 Nov 2012 14:55:56 +0100
Konrad Frederic <fred.konrad@greensocs.com> wrote:

> On 23/11/2012 13:08, Cornelia Huck wrote:
> > On Thu, 22 Nov 2012 15:50:50 +0100
> > fred.konrad@greensocs.com wrote:
> >
> >
> >> +/* 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++);
> > This still has the overflow/id-reuse problem, hasn't it?
> >
> >> +
> >> +    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;
> >> +}
> > Don't you need a way to destroy the bus again when the proxy device is
> > hotunplugged?
> >
> Is the virtio-pci-* proxy currently supporting hotunplugging ?
> 

No idea about virtio-pci devices, but since virtio-ccw will support
hotunplugging, we'll certainly need an interface for that.
Anthony Liguori - Nov. 26, 2012, 2:33 p.m.
fred.konrad@greensocs.com writes:

> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> 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 <fred.konrad@greensocs.com>
> ---
>  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   <fred.konrad@greensocs.com>
> + *
> + * 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 <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#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.

> +
> +/* 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.

> +
> +    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   <fred.konrad@greensocs.com>
> + *
> + * 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 <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#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).

Regards,

Anthony Liguori

> +};
> +
> +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
Peter Maydell - Nov. 26, 2012, 2:37 p.m.
On 26 November 2012 14:33, Anthony Liguori <aliguori@us.ibm.com> wrote:
> 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.

We have at least one user of Interface in the tree IIRC.
I'd much rather we did this the right way -- the only reason
it's the way Fred has coded it is that there's no obvious
body of code in the tree to copy, so we're thrashing around
a bit. If you tell us what the correct set of structs/classes/
interfaces/etc is then we can implement it :-)

-- PMM
Andreas Färber - Nov. 26, 2012, 2:45 p.m.
Am 26.11.2012 15:33, schrieb Anthony Liguori:
> fred.konrad@greensocs.com writes:
>> +#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.

If you look at the change history, this was a change in a reaction to me
pointing to a proposal by Samsung. I see your point with the else
statement, that can be circumvented by adding else {}. The reason is to
avoid DPRINTF()s bitrotting because your "do { } while (0)" performs no
compile-tests on "fmt, ..." arguments.

Andreas
fred.konrad@greensocs.com - Nov. 26, 2012, 3:33 p.m.
On 26/11/2012 15:33, Anthony Liguori wrote:
> fred.konrad@greensocs.com writes:
>
>> From: KONRAD Frederic<fred.konrad@greensocs.com>
>>
>> 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<fred.konrad@greensocs.com>
>> ---
>>   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<fred.konrad@greensocs.com>
>> + *
>> + * 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<http://www.gnu.org/licenses/>.
>> + *
>> + */
>> +
>> +#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<fred.konrad@greensocs.com>
>> + *
>> + * 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<http://www.gnu.org/licenses/>.
>> + *
>> + */
>> +
>> +#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
Stefan Hajnoczi - Nov. 26, 2012, 3:40 p.m.
On Mon, Nov 26, 2012 at 08:33:23AM -0600, Anthony Liguori wrote:
> fred.konrad@greensocs.com writes:
> 
> > From: KONRAD Frederic <fred.konrad@greensocs.com>
> >
> > 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 <fred.konrad@greensocs.com>
> > ---
> >  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   <fred.konrad@greensocs.com>
> > + *
> > + * 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 <http://www.gnu.org/licenses/>.
> > + *
> > + */
> > +
> > +#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.

This should be:

#define DEBUG_VIRTIO_BUS 1

#define DPRINTF(fmt, ...) \
    do { \
        if (DEBUG_VIRTIO_BUS) { \
            printf("virtio_bus: " fmt , ## __VA_ARGS__); \
        } \
    } while (0)

This way there's no dangling else statement problem.  But it also
ensures the we always compile the debug print, thereby avoiding bitrot
you get from #ifdef ... #else ... #endif.

Stefan
Anthony Liguori - Nov. 26, 2012, 4:55 p.m.
Andreas Färber <afaerber@suse.de> writes:

> Am 26.11.2012 15:33, schrieb Anthony Liguori:
>> fred.konrad@greensocs.com writes:
>>> +#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.
>
> If you look at the change history, this was a change in a reaction to me
> pointing to a proposal by Samsung. I see your point with the else
> statement, that can be circumvented by adding else {}. The reason is to
> avoid DPRINTF()s bitrotting because your "do { } while (0)" performs no
> compile-tests on "fmt, ..." arguments.

This is a well-used idiom in QEMU.  We shouldn't try to change idioms in
random patch series.

If you want to change the way we do DPRINTF(), it should be done
globally in its own series.

Regards,

Anthony Liguori

>
> Andreas
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Anthony Liguori - Nov. 26, 2012, 4:59 p.m.
Peter Maydell <peter.maydell@linaro.org> writes:

> On 26 November 2012 14:33, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> 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.
>
> We have at least one user of Interface in the tree IIRC.
> I'd much rather we did this the right way -- the only reason
> it's the way Fred has coded it is that there's no obvious
> body of code in the tree to copy, so we're thrashing around
> a bit. If you tell us what the correct set of structs/classes/
> interfaces/etc is then we can implement it :-)

I really think extending virtio-bus to a virtio-pci-bus and then
initializing it with a link to the PCI device is the best approach.

It's by far the simpliest approach in terms of coding.

Did I explain it adequately?  To recap:

virtio-bus extends bus-state
 - implements everything that VirtIOBindings implements as methods

virtio-pci-bus extends virtio-bus
 - is constructed with a pointer to a PCIDevice
 - implements the methods necessary to be a virtio bus

virtio-device extends device-state
 - implements methods used by virtio-bus

Regards,

Anthony Liguori

>
> -- PMM
fred.konrad@greensocs.com - Nov. 29, 2012, 12:37 p.m.
On 26/11/2012 17:59, Anthony Liguori wrote:
> Peter Maydell<peter.maydell@linaro.org>  writes:
>
>> On 26 November 2012 14:33, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>>> 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.
>> We have at least one user of Interface in the tree IIRC.
>> I'd much rather we did this the right way -- the only reason
>> it's the way Fred has coded it is that there's no obvious
>> body of code in the tree to copy, so we're thrashing around
>> a bit. If you tell us what the correct set of structs/classes/
>> interfaces/etc is then we can implement it :-)
> I really think extending virtio-bus to a virtio-pci-bus and then
> initializing it with a link to the PCI device is the best approach.
>
> It's by far the simpliest approach in terms of coding.
>
> Did I explain it adequately?  To recap:
>
> virtio-bus extends bus-state
>   - implements everything that VirtIOBindings implements as methods
>
> virtio-pci-bus extends virtio-bus
>   - is constructed with a pointer to a PCIDevice
>   - implements the methods necessary to be a virtio bus
I still have trouble with that ^^.

virtio-pci-bus extends virtio-bus so I put something like that :

static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
{
     BusClass *bc = BUS_CLASS(klass);
     VirtioBusClass *k = VIRTIO_BUS_CLASS(klass);
     /* VirtIOBindings */
     k->notify = virtio_pci_notify;
     k->save_config = virtio_pci_save_config;
     k->load_config = virtio_pci_load_config;
     k->save_queue = virtio_pci_save_queue;
     k->load_queue = virtio_pci_load_queue;
     k->get_features = virtio_pci_get_features;
     k->query_guest_notifiers = virtio_pci_query_guest_notifiers;
     k->set_host_notifier = virtio_pci_set_host_notifier;
     k->set_guest_notifiers = virtio_pci_set_guest_notifiers;
     k->vmstate_change = virtio_pci_vmstate_change;
     /*
      * TODO : Init and exit function.
      * void (*init)(void *opaque);
      * void (*exit)(void *opaque);
      */
}

static TypeInfo virtio_pci_bus_info = {
     .name          = TYPE_VIRTIO_PCI_BUS,
     .parent        = TYPE_VIRTIO_BUS,
     .class_init    = virtio_pci_bus_class_init,
};

and I have VirtioDevice which extends DeviceState like that :

static void virtio_device_class_init(ObjectClass *klass, void *data)
{
     /* Set the default value here. */
     DeviceClass *dc = DEVICE_CLASS(klass);
     dc->bus_type = TYPE_VIRTIO_BUS;
     dc->init = virtio_device_init;
}

static TypeInfo virtio_device_info = {
     .name = TYPE_VIRTIO_DEVICE,
     .parent = TYPE_DEVICE,
     .instance_size = sizeof(VirtioDeviceState),
     /* Abstract the virtio-device */
     .class_init = virtio_device_class_init,
     .abstract = true,
     .class_size = sizeof(VirtioDeviceClass),
};

The problem is that the virtio devices can't be connected to the 
virtio-pci-bus even if it extends virtio-bus because TYPE_VIRTIO_BUS != 
TYPE_VIRTIO_PCI_BUS.

Did I miss something ?

Thanks,

Fred


> virtio-device extends device-state
>   - implements methods used by virtio-bus
>
> Regards,
>
> Anthony Liguori
>
>> -- PMM
Peter Maydell - Nov. 29, 2012, 1:09 p.m.
On 29 November 2012 12:37, Konrad Frederic <fred.konrad@greensocs.com> wrote:
> On 26/11/2012 17:59, Anthony Liguori wrote:
>> virtio-pci-bus extends virtio-bus
>>   - is constructed with a pointer to a PCIDevice
>>   - implements the methods necessary to be a virtio bus
>
> I still have trouble with that ^^.

> The problem is that the virtio devices can't be connected to the
> virtio-pci-bus even if it extends virtio-bus because TYPE_VIRTIO_BUS !=
> TYPE_VIRTIO_PCI_BUS.

Conceptually it ought to work I think: if the bus is-a TYPE_VIRTIO_BUS
then we should permit plugging in even if the actual bus object happens
to be an instance of a subclass.

I suspect that qbus_find_recursive should be doing an
object_class_dynamic_cast() to check that the bus is of a suitable
type, rather than the
    (strcmp(object_get_typename(OBJECT(bus)), bus_typename) != 0)
which it does at the moment.

-- PMM
fred.konrad@greensocs.com - Nov. 29, 2012, 1:47 p.m.
On 29/11/2012 14:09, Peter Maydell wrote:
> On 29 November 2012 12:37, Konrad Frederic<fred.konrad@greensocs.com>  wrote:
>> On 26/11/2012 17:59, Anthony Liguori wrote:
>>> virtio-pci-bus extends virtio-bus
>>>    - is constructed with a pointer to a PCIDevice
>>>    - implements the methods necessary to be a virtio bus
>> I still have trouble with that ^^.
>> The problem is that the virtio devices can't be connected to the
>> virtio-pci-bus even if it extends virtio-bus because TYPE_VIRTIO_BUS !=
>> TYPE_VIRTIO_PCI_BUS.
> Conceptually it ought to work I think: if the bus is-a TYPE_VIRTIO_BUS
> then we should permit plugging in even if the actual bus object happens
> to be an instance of a subclass.
Yes, is my implementation doing the right thing ?
I mean is the virtio-pci-bus a virtio-bus ?

>
> I suspect that qbus_find_recursive should be doing an
> object_class_dynamic_cast() to check that the bus is of a suitable
> type, rather than the
>      (strcmp(object_get_typename(OBJECT(bus)), bus_typename) != 0)
> which it does at the moment.
Yes, but we can cast VIRTIO_BUS in BUS no ?
So in this case we could plug VirtioDevice in BUS and that's not what we 
want ?

>
> -- PMM
>
Anthony Liguori - Nov. 29, 2012, 1:52 p.m.
Konrad Frederic <fred.konrad@greensocs.com> writes:

> On 26/11/2012 17:59, Anthony Liguori wrote:
>> Peter Maydell<peter.maydell@linaro.org>  writes:
>>
>>> On 26 November 2012 14:33, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>>>> 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.
>>> We have at least one user of Interface in the tree IIRC.
>>> I'd much rather we did this the right way -- the only reason
>>> it's the way Fred has coded it is that there's no obvious
>>> body of code in the tree to copy, so we're thrashing around
>>> a bit. If you tell us what the correct set of structs/classes/
>>> interfaces/etc is then we can implement it :-)
>> I really think extending virtio-bus to a virtio-pci-bus and then
>> initializing it with a link to the PCI device is the best approach.
>>
>> It's by far the simpliest approach in terms of coding.
>>
>> Did I explain it adequately?  To recap:
>>
>> virtio-bus extends bus-state
>>   - implements everything that VirtIOBindings implements as methods
>>
>> virtio-pci-bus extends virtio-bus
>>   - is constructed with a pointer to a PCIDevice
>>   - implements the methods necessary to be a virtio bus
> I still have trouble with that ^^.
>
> virtio-pci-bus extends virtio-bus so I put something like that :
>
> static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
> {
>      BusClass *bc = BUS_CLASS(klass);
>      VirtioBusClass *k = VIRTIO_BUS_CLASS(klass);
>      /* VirtIOBindings */
>      k->notify = virtio_pci_notify;
>      k->save_config = virtio_pci_save_config;
>      k->load_config = virtio_pci_load_config;
>      k->save_queue = virtio_pci_save_queue;
>      k->load_queue = virtio_pci_load_queue;
>      k->get_features = virtio_pci_get_features;
>      k->query_guest_notifiers = virtio_pci_query_guest_notifiers;
>      k->set_host_notifier = virtio_pci_set_host_notifier;
>      k->set_guest_notifiers = virtio_pci_set_guest_notifiers;
>      k->vmstate_change = virtio_pci_vmstate_change;
>      /*
>       * TODO : Init and exit function.
>       * void (*init)(void *opaque);
>       * void (*exit)(void *opaque);
>       */
> }
>
> static TypeInfo virtio_pci_bus_info = {
>      .name          = TYPE_VIRTIO_PCI_BUS,
>      .parent        = TYPE_VIRTIO_BUS,
>      .class_init    = virtio_pci_bus_class_init,
> };
>
> and I have VirtioDevice which extends DeviceState like that :
>
> static void virtio_device_class_init(ObjectClass *klass, void *data)
> {
>      /* Set the default value here. */
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      dc->bus_type = TYPE_VIRTIO_BUS;
>      dc->init = virtio_device_init;
> }
>
> static TypeInfo virtio_device_info = {
>      .name = TYPE_VIRTIO_DEVICE,
>      .parent = TYPE_DEVICE,
>      .instance_size = sizeof(VirtioDeviceState),
>      /* Abstract the virtio-device */
>      .class_init = virtio_device_class_init,
>      .abstract = true,
>      .class_size = sizeof(VirtioDeviceClass),
> };
>
> The problem is that the virtio devices can't be connected to the 
> virtio-pci-bus even if it extends virtio-bus because TYPE_VIRTIO_BUS != 
> TYPE_VIRTIO_PCI_BUS.

That's just a bug.  See the patch I just sent out to fix it.

The type check is too strict in qdev_device_add().

Regards,

Anthony Liguori

>
> Did I miss something ?
>
> Thanks,
>
> Fred
>
>
>> virtio-device extends device-state
>>   - implements methods used by virtio-bus
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>> -- PMM
Peter Maydell - Nov. 29, 2012, 1:53 p.m.
On 29 November 2012 13:47, Konrad Frederic <fred.konrad@greensocs.com> wrote:
> On 29/11/2012 14:09, Peter Maydell wrote:
>> I suspect that qbus_find_recursive should be doing an
>> object_class_dynamic_cast() to check that the bus is of a suitable
>> type, rather than the
>>      (strcmp(object_get_typename(OBJECT(bus)), bus_typename) != 0)
>> which it does at the moment.
>
> Yes, but we can cast VIRTIO_BUS in BUS no ?
> So in this case we could plug VirtioDevice in BUS and that's not what we
> want ?

I don't understand what you're asking. qbus_find_recursive()
looks for a bus which might be specified either by name or by
bus type. At the moment it insists on an exact type match
(so if you ask for a virtio-bus you have to provide a virtio-bus
object, not a subclass of virtio-bus); it should do a dynamic
cast, so anything which is a virtio-bus or a subclass of that
will do. Yes, if you say "please find me any bus which is a
bus of any kind" then you'll get something wrong back -- so
don't do that. In particular, since VirtioDevice sets its
bus_type to TYPE_VIRTIO_BUS then we will ask for that, and
anything which isn't a virtio bus (or subclass) won't be found.

-- PMM
Andreas Färber - Nov. 29, 2012, 1:55 p.m.
Am 29.11.2012 14:47, schrieb Konrad Frederic:
> On 29/11/2012 14:09, Peter Maydell wrote:
>> On 29 November 2012 12:37, Konrad Frederic<fred.konrad@greensocs.com> 
>> wrote:
>>> On 26/11/2012 17:59, Anthony Liguori wrote:
>>>> virtio-pci-bus extends virtio-bus
>>>>    - is constructed with a pointer to a PCIDevice
>>>>    - implements the methods necessary to be a virtio bus
>>> I still have trouble with that ^^.
>>> The problem is that the virtio devices can't be connected to the
>>> virtio-pci-bus even if it extends virtio-bus because TYPE_VIRTIO_BUS !=
>>> TYPE_VIRTIO_PCI_BUS.
>> Conceptually it ought to work I think: if the bus is-a TYPE_VIRTIO_BUS
>> then we should permit plugging in even if the actual bus object happens
>> to be an instance of a subclass.
> Yes, is my implementation doing the right thing ?
> I mean is the virtio-pci-bus a virtio-bus ?

In your v3 patchset no. In the inline code yes, via
virtio_pci_bus_info's .parent.

>> I suspect that qbus_find_recursive should be doing an
>> object_class_dynamic_cast() to check that the bus is of a suitable
>> type, rather than the
>>      (strcmp(object_get_typename(OBJECT(bus)), bus_typename) != 0)
>> which it does at the moment.
> Yes, but we can cast VIRTIO_BUS in BUS no ?
> So in this case we could plug VirtioDevice in BUS and that's not what we
> want ?

You would want to check whether object_class_dynamic_cast(OBJECT(bus),
TYPE_VIRTIO_BUS) == NULL. This should evaluate to true if the bus is not
a virtio-bus.

Andreas
fred.konrad@greensocs.com - Nov. 29, 2012, 2:28 p.m.
On 29/11/2012 14:55, Andreas Färber wrote:
> Am 29.11.2012 14:47, schrieb Konrad Frederic:
>> On 29/11/2012 14:09, Peter Maydell wrote:
>>> On 29 November 2012 12:37, Konrad Frederic<fred.konrad@greensocs.com>
>>> wrote:
>>>> On 26/11/2012 17:59, Anthony Liguori wrote:
>>>>> virtio-pci-bus extends virtio-bus
>>>>>     - is constructed with a pointer to a PCIDevice
>>>>>     - implements the methods necessary to be a virtio bus
>>>> I still have trouble with that ^^.
>>>> The problem is that the virtio devices can't be connected to the
>>>> virtio-pci-bus even if it extends virtio-bus because TYPE_VIRTIO_BUS !=
>>>> TYPE_VIRTIO_PCI_BUS.
>>> Conceptually it ought to work I think: if the bus is-a TYPE_VIRTIO_BUS
>>> then we should permit plugging in even if the actual bus object happens
>>> to be an instance of a subclass.
>> Yes, is my implementation doing the right thing ?
>> I mean is the virtio-pci-bus a virtio-bus ?
> In your v3 patchset no. In the inline code yes, via
> virtio_pci_bus_info's .parent.
>
>>> I suspect that qbus_find_recursive should be doing an
>>> object_class_dynamic_cast() to check that the bus is of a suitable
>>> type, rather than the
>>>       (strcmp(object_get_typename(OBJECT(bus)), bus_typename) != 0)
>>> which it does at the moment.
>> Yes, but we can cast VIRTIO_BUS in BUS no ?
>> So in this case we could plug VirtioDevice in BUS and that's not what we
>> want ?
> You would want to check whether object_class_dynamic_cast(OBJECT(bus),
> TYPE_VIRTIO_BUS) == NULL. This should evaluate to true if the bus is not
> a virtio-bus.
Yes, ok I got it!

Thanks,

Fred

Patch

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   <fred.konrad@greensocs.com>
+ *
+ * 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 <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#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__); \
+                          }
+
+static void virtio_bus_init_cb(VirtioBus *bus);
+static int virtio_bus_reset(BusState *qbus);
+
+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;
+}
+
+/* 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++);
+
+    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   <fred.konrad@greensocs.com>
+ *
+ * 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 <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#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);
+    VirtIOBindings virtio_bindings;
+};
+
+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 */