| Message ID | 1353595852-30776-2-git-send-email-fred.konrad@greensocs.com |
|---|---|
| State | New |
| Headers | show |
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?
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?
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
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
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 >
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
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
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 ?
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.
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
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
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
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
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
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
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
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
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
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 >
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
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
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
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
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 */