|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 1/2] Add device listener interface
> -----Original Message----- > From: Igor Mammedov [mailto:imammedo@xxxxxxxxxx] > Sent: 16 October 2014 13:41 > To: Paul Durrant > Cc: qemu-devel@xxxxxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxxx; Michael S. > Tsirkin; Andreas Faerber > Subject: Re: [PATCH v3 1/2] Add device listener interface > > On Wed, 15 Oct 2014 10:05:32 +0000 > Paul Durrant <Paul.Durrant@xxxxxxxxxx> wrote: > > > > > > > > -----Original Message----- > > > From: Igor Mammedov [mailto:imammedo@xxxxxxxxxx] > > > Sent: 15 October 2014 10:54 > > > To: Paul Durrant > > > Cc: qemu-devel@xxxxxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxxx; Michael > > > S. Tsirkin; Andreas Faerber"; Paolo Bonzini; Peter Crosthwaite; Igor > > > Mammedov; Markus Armbruster; Thomas Huth; Christian Borntraeger > > > Subject: Re: [PATCH v3 1/2] Add device listener interface > > > > > > On Wed, 15 Oct 2014 10:16:38 +0100 > > > Paul Durrant <paul.durrant@xxxxxxxxxx> wrote: > > > > > > > The Xen ioreq-server API, introduced in Xen 4.5, requires that PCI > > > > device models explicitly register with Xen for config space > > > > accesses. This patch adds a listener interface into qdev-core > > > > which can be used by the Xen interface code to monitor for > > > > arrival and departure of PCI devices. > > > > > > If you need only one listener handler for your case, why you > > > couldn't use hotplug interface instead of listerners? > > > to me it looks like it should work for this case. > > > To make it work xen code would need to override default plug/unplug > > > handlers on PCI bus and do register/unregister from there. > > > > > > > That sounds a bit ugly. A pci or now qdev listener interface seems > > more elegant and generally useful. > Even if suggested listener is usefull it could be better if handlers > had error handling as well, so that errors in callback could be > reported up to the caller. That's not necessary for current use and not consistent with the memory listener interface. What sort of thing would you envisage the caller doing with such an error? > > > > > > One thing is that unplug handler is not defined for PCI bus yet, > > > to get it work one would need to refactor pcihp/bridge/pcie unplug > > > path to call unplug handler before object_unparent(). > > > > > > > Yes - that makes it a much more intrusive modification. > It' isn't if you don't consolidate unplug handler from > pcihp/bridge/pcie. It would be ~3-4 LOC per each. > Ok, but doing specific modifications like that still seems ugly. Also Xen needs to know about all PCI devices, not just hotplugged ones. I know that all PCI devices in QEMU are hotplugged at the moment but if that ever changed it would then break on Xen if I were to choose this method of callback. Paul > > > > Paul > > > > > > > > > > > > > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx> > > > > Cc: Michael S. Tsirkin <mst@xxxxxxxxxx> > > > > Cc: Andreas Faerber" <afaerber@xxxxxxx> > > > > Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx> > > > > Cc: Peter Crosthwaite <peter.crosthwaite@xxxxxxxxxx> > > > > Cc: Igor Mammedov <imammedo@xxxxxxxxxx> > > > > Cc: Markus Armbruster <armbru@xxxxxxxxxx> > > > > Cc: Thomas Huth <thuth@xxxxxxxxxxxxxxxxxx> > > > > Cc: Christian Borntraeger <borntraeger@xxxxxxxxxx> > > > > --- > > > > hw/core/qdev.c | 54 > > > > +++++++++++++++++++++++++++++++++++++++++++++++ > > > > include/hw/qdev-core.h | 10 +++++++++ include/qemu/typedefs.h | > > > > 1 + 3 files changed, 65 insertions(+) > > > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > > > index fcb1638..4a9c1f6 100644 > > > > --- a/hw/core/qdev.c > > > > +++ b/hw/core/qdev.c > > > > @@ -175,6 +175,56 @@ int qdev_init(DeviceState *dev) > > > > return 0; > > > > } > > > > > > > > +static QTAILQ_HEAD(qdev_listeners, DeviceListener) qdev_listeners > > > > + = QTAILQ_HEAD_INITIALIZER(qdev_listeners); > > > > + > > > > +enum ListenerDirection { Forward, Reverse }; > > > > + > > > > +#define QDEV_LISTENER_CALL(_callback, _direction, _args...) \ > > > > + do { \ > > > > + DeviceListener *_listener; \ > > > > + \ > > > > + switch (_direction) { \ > > > > + case Forward: \ > > > > + QTAILQ_FOREACH(_listener, &qdev_listeners, link) { \ > > > > + if (_listener->_callback) { \ > > > > + _listener->_callback(_listener, ##_args); \ > > > > + } \ > > > > + } \ > > > > + break; \ > > > > + case Reverse: \ > > > > + QTAILQ_FOREACH_REVERSE(_listener, &qdev_listeners, \ > > > > + qdev_listeners, link) { \ > > > > + if (_listener->_callback) { \ > > > > + _listener->_callback(_listener, ##_args); \ > > > > + } \ > > > > + } \ > > > > + break; \ > > > > + default: \ > > > > + abort(); \ > > > > + } \ > > > > + } while (0) > > > > + > > > > +static int qdev_listener_add(DeviceState *dev, void *opaque) > > > > +{ > > > > + QDEV_LISTENER_CALL(realize, Forward, dev); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +void qdev_listener_register(DeviceListener *listener) > > > > +{ > > > > + QTAILQ_INSERT_TAIL(&qdev_listeners, listener, link); > > > > + > > > > + qbus_walk_children(sysbus_get_default(), NULL, NULL, > > > > qdev_listener_add, > > > > + NULL, NULL); > > > > +} > > > > + > > > > +void qdev_listener_unregister(DeviceListener *listener) > > > > +{ > > > > + QTAILQ_REMOVE(&qdev_listeners, listener, link); > > > > +} > > > > + > > > > static void device_realize(DeviceState *dev, Error **errp) > > > > { > > > > DeviceClass *dc = DEVICE_GET_CLASS(dev); > > > > @@ -186,12 +236,16 @@ static void device_realize(DeviceState *dev, > > > > Error **errp) return; > > > > } > > > > } > > > > + > > > > + QDEV_LISTENER_CALL(realize, Forward, dev); > > > > } > > > > > > > > static void device_unrealize(DeviceState *dev, Error **errp) > > > > { > > > > DeviceClass *dc = DEVICE_GET_CLASS(dev); > > > > > > > > + QDEV_LISTENER_CALL(unrealize, Reverse, dev); > > > > + > > > > if (dc->exit) { > > > > int rc = dc->exit(dev); > > > > if (rc < 0) { > > > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > > > > index 178fee2..f2dc267 100644 > > > > --- a/include/hw/qdev-core.h > > > > +++ b/include/hw/qdev-core.h > > > > @@ -167,6 +167,12 @@ struct DeviceState { > > > > int alias_required_for_version; > > > > }; > > > > > > > > +struct DeviceListener { > > > > + void (*realize)(DeviceListener *listener, DeviceState *dev); > > > > + void (*unrealize)(DeviceListener *listener, DeviceState > > > > *dev); > > > > + QTAILQ_ENTRY(DeviceListener) link; > > > > +}; > > > > + > > > > #define TYPE_BUS "bus" > > > > #define BUS(obj) OBJECT_CHECK(BusState, (obj), TYPE_BUS) > > > > #define BUS_CLASS(klass) OBJECT_CLASS_CHECK(BusClass, (klass), > > > > TYPE_BUS) @@ -368,4 +374,8 @@ static inline void > > > > qbus_set_hotplug_handler(BusState *bus, DeviceState *handler, > > > > QDEV_HOTPLUG_HANDLER_PROPERTY, errp); bus->allow_hotplug = 1; > > > > } > > > > + > > > > +void qdev_listener_register(DeviceListener *listener); > > > > +void qdev_listener_unregister(DeviceListener *listener); > > > > + > > > > #endif > > > > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h > > > > index 04df51b..e32bca2 100644 > > > > --- a/include/qemu/typedefs.h > > > > +++ b/include/qemu/typedefs.h > > > > @@ -20,6 +20,7 @@ typedef struct Property Property; > > > > typedef struct PropertyInfo PropertyInfo; > > > > typedef struct CompatProperty CompatProperty; > > > > typedef struct DeviceState DeviceState; > > > > +typedef struct DeviceListener DeviceListener; > > > > typedef struct BusState BusState; > > > > typedef struct BusClass BusClass; > > > > > > _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |