Am 16.11.2012 16:35, schrieb fred.kon...@greensocs.com:
> From: KONRAD Frederic <fred.kon...@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.kon...@greensocs.com>
> ---
> 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: i...@greensocs.com
> + *
> + * Developed by :
> + * Frederic Konrad <fred.kon...@greensocs.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
Any chance to use GPLv2 "or (at your option) any later version"? If not,
please mention in the commit message why.
> + */
> +
> +#include "hw.h"
> +#include "qemu-error.h"
> +#include "qdev.h"
> +#include "virtio-bus.h"
> +#include "virtio.h"
> +
> +#define DEBUG_VIRTIO_BUS
> +
> +#ifdef DEBUG_VIRTIO_BUS
> +
> +#define DPRINTF(fmt, ...) \
> +do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) do {} while (0)
> +#endif
We recently had a discussion about bitrotting DPRINTF() statements where
I suggested to use if (0) instead of a no-op macro like this that
doesn't reference fmt and the varargs.
> +
> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev);
> +
> +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);
Accessing the qbus field directly is considered old-style. Please use
the BUS() macro to cast to a new BusState* variable and pass that here...
> + bus->busnr = next_virtio_bus++;
> + bus->info = info;
> + /* no hotplug for the moment ? */
> + bus->qbus.allow_hotplug = 0;
...and access its fields through it. More cases below.
> + 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 must be called to when the VirtIODevice init */
"called when the ...Device inits" or "called during ...Device init"
> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *vdev)
> +{
> + if (bus->bus_in_use == true) {
Even if bus_in_use is of bool type, doing an explicit "true" comparison
is not a good idea in C since everything non-zero is to be considered
true, not just the "true" constant.
> + 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;
> + bus->info->init_cb(bus->qbus.parent);
> +
> + bus->bus_in_use = true;
> + return 0;
> +}
> +
> +/* This must be called when the VirtIODevice exit */
Similar grammar issue as above.
> +void virtio_bus_exit_cb(VirtioBus *bus)
> +{
> + assert(bus->info->exit_cb != NULL);
> + bus->info->exit_cb(bus->qbus.parent);
> + bus->bus_in_use = false;
> +}
> +
> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev)
> +{
> + return g_strdup_printf("%s", qdev_fw_name(dev));
> +}
> +
> +
> +static void virtio_register_types(void)
> +{
> + type_register_static(&virtio_bus_info);
> +}
> +
> +type_init(virtio_register_types)
> diff --git a/hw/virtio-bus.h b/hw/virtio-bus.h
> new file mode 100644
> index 0000000..6ea5035
> --- /dev/null
> +++ b/hw/virtio-bus.h
> @@ -0,0 +1,49 @@
> +/*
> + * VirtioBus
> + *
> + * Copyright (C) 2012 : GreenSocs Ltd
> + * http://www.greensocs.com/ , email: i...@greensocs.com
> + *
> + * Developed by :
> + * Frederic Konrad <fred.kon...@greensocs.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#ifndef _VIRTIO_BUS_H_
> +#define _VIRTIO_BUS_H_
> +
> +#include "qdev.h"
> +#include "sysemu.h"
> +#include "virtio.h"
> +
> +#define TYPE_VIRTIO_BUS "VIRTIO"
Is there a special reason for all-uppercase here? Is lowercase already
taken?
You should add QOM cast macros VIRTIO_BUS() et al. to access the
VirtioBus fields.
> +#define BUS_NAME "virtio"
> +
> +typedef struct VirtioBus VirtioBus;
> +typedef struct VirtioBusInfo VirtioBusInfo;
> +
> +struct VirtioBusInfo {
> + void (*init_cb)(DeviceState *dev);
> + void (*exit_cb)(DeviceState *dev);
> + VirtIOBindings virtio_bindings;
> +};
> +
> +struct VirtioBus {
> + BusState qbus;
You could name this field parent_obj to break the old qbus pattern.
> + int busnr;
> + bool bus_in_use;
> + uint16_t pci_device_id;
> + uint16_t pci_class;
pci_* in VirtioBus does not strike me as the best naming. Either this is
specific to the PCI backend and doesn't belong here, or it's not a
pci_device_id but a device_id.
> + VirtIODevice *vdev;
> + const VirtioBusInfo *info;
> +};
> +
> +void virtio_bus_new(VirtioBus *bus, DeviceState *host,
> + const VirtioBusInfo *info);
> +void virtio_bus_bind_device(VirtioBus *bus);
> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *dev);
> +void virtio_bus_exit_cb(VirtioBus *bus);
> +
> +#endif
>
Regards,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
