diff mbox

[v3,1/2] Add device listener interface

Message ID 1413364599-7582-2-git-send-email-paul.durrant@citrix.com
State New
Headers show

Commit Message

Paul Durrant Oct. 15, 2014, 9:16 a.m. UTC
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.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Andreas Faerber" <afaerber@suse.de>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Thomas Huth <thuth@linux.vnet.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/core/qdev.c          |   54 +++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/qdev-core.h  |   10 +++++++++
 include/qemu/typedefs.h |    1 +
 3 files changed, 65 insertions(+)

Comments

Igor Mammedov Oct. 15, 2014, 9:54 a.m. UTC | #1
On Wed, 15 Oct 2014 10:16:38 +0100
Paul Durrant <paul.durrant@citrix.com> 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.

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


> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Andreas Faerber" <afaerber@suse.de>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Thomas Huth <thuth@linux.vnet.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  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;
>
Paul Durrant Oct. 15, 2014, 10:05 a.m. UTC | #2
> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: 15 October 2014 10:54
> To: Paul Durrant
> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; 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@citrix.com> 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.

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

  Paul

> 
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Andreas Faerber" <afaerber@suse.de>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > Cc: Markus Armbruster <armbru@redhat.com>
> > Cc: Thomas Huth <thuth@linux.vnet.ibm.com>
> > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > ---
> >  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;
> >
Igor Mammedov Oct. 16, 2014, 12:41 p.m. UTC | #3
On Wed, 15 Oct 2014 10:05:32 +0000
Paul Durrant <Paul.Durrant@citrix.com> wrote:

> 
> 
> > -----Original Message-----
> > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > Sent: 15 October 2014 10:54
> > To: Paul Durrant
> > Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; 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@citrix.com> 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@citrix.com>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Andreas Faerber" <afaerber@suse.de>
> > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> > > Cc: Igor Mammedov <imammedo@redhat.com>
> > > Cc: Markus Armbruster <armbru@redhat.com>
> > > Cc: Thomas Huth <thuth@linux.vnet.ibm.com>
> > > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > > ---
> > >  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;
> > >
>
Paul Durrant Oct. 16, 2014, 12:54 p.m. UTC | #4
> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: 16 October 2014 13:41
> To: Paul Durrant
> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; 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@citrix.com> wrote:
> 
> >
> >
> > > -----Original Message-----
> > > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > > Sent: 15 October 2014 10:54
> > > To: Paul Durrant
> > > Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; 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@citrix.com> 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@citrix.com>
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: Andreas Faerber" <afaerber@suse.de>
> > > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > > Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> > > > Cc: Igor Mammedov <imammedo@redhat.com>
> > > > Cc: Markus Armbruster <armbru@redhat.com>
> > > > Cc: Thomas Huth <thuth@linux.vnet.ibm.com>
> > > > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > > > ---
> > > >  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;
> > > >
> >
diff mbox

Patch

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;