Re: [Xen-devel] [PATCH v3 1/2] Add device listener interface

[Igor Mammedov <imammedo@xxxxxxxxxx>]

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.

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

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