[Qemu-devel] [PATCH v3 0/2] Xen: Use ioreq-server API

[Qemu-devel] [PATCH v3 0/2] Xen: Use ioreq-server API

[Paul Durrant]

This patch series is v3 of what was originally the single patch
"Xen: Use the ioreq-server API when available".

v2 of the series moved the code that added the PCI bus listener
to patch #1 and the remainder of the changes to patch #2. Patch #2
was then re-worked to constrain the #ifdefing to xen_common.h, as
requested by Stefano.

v3 of the series modifies patch #1 to add the listener interface
into the core qdev, rather than the PCI bus code. This change only
requires trivial modification to patch #2, to only act on realize/
unrealize of PCI devices. Patch #2 was also modified at Stefano's
request to remove an extra identity check of memory sections
against the ram region.

[Qemu-devel] [PATCH v3 1/2] Add device listener interface

[Paul Durrant]

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

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

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

[Igor Mammedov]

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

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

[Paul Durrant]

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

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

[Igor Mammedov]

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

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

[Paul Durrant]

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

[Qemu-devel] [PATCH v3 2/2] Xen: Use the ioreq-server API when available

[Paul Durrant]

The ioreq-server API added to Xen 4.5 offers better security than
the existing Xen/QEMU interface because the shared pages that are
used to pass emulation request/results back and forth are removed
from the guest's memory space before any requests are serviced.
This prevents the guest from mapping these pages (they are in a
well known location) and attempting to attack QEMU by synthesizing
its own request structures. Hence, this patch modifies configure
to detect whether the API is available, and adds the necessary
code to use the API if it is.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Michael Tokarev <mjt@tls.msk.ru>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Stefan Weil <sw@weilnetz.de>
Cc: Olaf Hering <olaf@aepfle.de>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
Cc: Alexander Graf <agraf@suse.de>
---
 configure                   |   29 ++++++
 include/hw/xen/xen_common.h |  222 +++++++++++++++++++++++++++++++++++++++++++
 trace-events                |    8 ++
 xen-hvm.c                   |  174 +++++++++++++++++++++++++++++----
 4 files changed, 412 insertions(+), 21 deletions(-)

diff --git a/configure b/configure
index 9ac2600..c2db574 100755
--- a/configure
+++ b/configure
@@ -1876,6 +1876,32 @@ int main(void) {
   xc_gnttab_open(NULL, 0);
   xc_domain_add_to_physmap(0, 0, XENMAPSPACE_gmfn, 0, 0);
   xc_hvm_inject_msi(xc, 0, 0xf0000000, 0x00000000);
+  xc_hvm_create_ioreq_server(xc, 0, 0, NULL);
+  return 0;
+}
+EOF
+      compile_prog "" "$xen_libs"
+    then
+    xen_ctrl_version=450
+    xen=yes
+
+  elif
+      cat > $TMPC <<EOF &&
+#include <xenctrl.h>
+#include <xenstore.h>
+#include <stdint.h>
+#include <xen/hvm/hvm_info_table.h>
+#if !defined(HVM_MAX_VCPUS)
+# error HVM_MAX_VCPUS not defined
+#endif
+int main(void) {
+  xc_interface *xc;
+  xs_daemon_open();
+  xc = xc_interface_open(0, 0, 0);
+  xc_hvm_set_mem_type(0, 0, HVMMEM_ram_ro, 0, 0);
+  xc_gnttab_open(NULL, 0);
+  xc_domain_add_to_physmap(0, 0, XENMAPSPACE_gmfn, 0, 0);
+  xc_hvm_inject_msi(xc, 0, 0xf0000000, 0x00000000);
   return 0;
 }
 EOF
@@ -4282,6 +4308,9 @@ if test -n "$sparc_cpu"; then
     echo "Target Sparc Arch $sparc_cpu"
 fi
 echo "xen support       $xen"
+if test "$xen" = "yes" ; then
+  echo "xen ctrl version  $xen_ctrl_version"
+fi
 echo "brlapi support    $brlapi"
 echo "bluez  support    $bluez"
 echo "Documentation     $docs"
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 07731b9..7040506 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -16,7 +16,9 @@
 
 #include "hw/hw.h"
 #include "hw/xen/xen.h"
+#include "hw/pci/pci.h"
 #include "qemu/queue.h"
+#include "trace.h"
 
 /*
  * We don't support Xen prior to 3.3.0.
@@ -164,4 +166,224 @@ void destroy_hvm_domain(bool reboot);
 /* shutdown/destroy current domain because of an error */
 void xen_shutdown_fatal_error(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 
+/* Xen before 4.5 */
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 450
+
+#ifndef HVM_PARAM_BUFIOREQ_EVTCHN
+#define HVM_PARAM_BUFIOREQ_EVTCHN 26
+#endif
+
+#define IOREQ_TYPE_PCI_CONFIG 2
+
+typedef uint32_t ioservid_t;
+
+static inline void xen_map_memory_section(XenXC xc, domid_t dom,
+                                          ioservid_t ioservid,
+                                          MemoryRegionSection *section)
+{
+}
+
+static inline void xen_unmap_memory_section(XenXC xc, domid_t dom,
+                                            ioservid_t ioservid,
+                                            MemoryRegionSection *section)
+{
+}
+
+static inline void xen_map_io_section(XenXC xc, domid_t dom,
+                                      ioservid_t ioservid,
+                                      MemoryRegionSection *section)
+{
+}
+
+static inline void xen_unmap_io_section(XenXC xc, domid_t dom,
+                                        ioservid_t ioservid,
+                                        MemoryRegionSection *section)
+{
+}
+
+static inline void xen_map_pcidev(XenXC xc, domid_t dom,
+                                  ioservid_t ioservid,
+                                  PCIDevice *pci_dev)
+{
+}
+
+static inline void xen_unmap_pcidev(XenXC xc, domid_t dom,
+                                    ioservid_t ioservid,
+                                    PCIDevice *pci_dev)
+{
+}
+
+static inline int xen_create_ioreq_server(XenXC xc, domid_t dom,
+                                          ioservid_t *ioservid)
+{
+    return 0;
+}
+
+static inline void xen_destroy_ioreq_server(XenXC xc, domid_t dom,
+                                            ioservid_t ioservid)
+{
+}
+
+static inline int xen_get_ioreq_server_info(XenXC xc, domid_t dom,
+                                            ioservid_t ioservid,
+                                            xen_pfn_t *ioreq_pfn,
+                                            xen_pfn_t *bufioreq_pfn,
+                                            evtchn_port_t *bufioreq_evtchn)
+{
+    unsigned long param;
+    int rc;
+
+    rc = xc_get_hvm_param(xc, dom, HVM_PARAM_IOREQ_PFN, &param);
+    if (rc < 0) {
+        fprintf(stderr, "failed to get HVM_PARAM_IOREQ_PFN\n");
+        return -1;
+    }
+
+    *ioreq_pfn = param;
+
+    rc = xc_get_hvm_param(xc, dom, HVM_PARAM_BUFIOREQ_PFN, &param);
+    if (rc < 0) {
+        fprintf(stderr, "failed to get HVM_PARAM_BUFIOREQ_PFN\n");
+        return -1;
+    }
+
+    *bufioreq_pfn = param;
+
+    rc = xc_get_hvm_param(xc, dom, HVM_PARAM_BUFIOREQ_EVTCHN,
+                          &param);
+    if (rc < 0) {
+        fprintf(stderr, "failed to get HVM_PARAM_BUFIOREQ_EVTCHN\n");
+        return -1;
+    }
+
+    *bufioreq_evtchn = param;
+
+    return 0;
+}
+
+static inline int xen_set_ioreq_server_state(XenXC xc, domid_t dom,
+                                             ioservid_t ioservid,
+                                             bool enable)
+{
+    return 0;
+}
+
+/* Xen 4.5 */
+#else
+
+static inline void xen_map_memory_section(XenXC xc, domid_t dom,
+                                          ioservid_t ioservid,
+                                          MemoryRegionSection *section)
+{
+    hwaddr start_addr = section->offset_within_address_space;
+    ram_addr_t size = int128_get64(section->size);
+    hwaddr end_addr = start_addr + size - 1;
+
+    trace_xen_map_mmio_range(ioservid, start_addr, end_addr);
+    xc_hvm_map_io_range_to_ioreq_server(xc, dom, ioservid, 1,
+                                        start_addr, end_addr);
+}
+
+static inline void xen_unmap_memory_section(XenXC xc, domid_t dom,
+                                            ioservid_t ioservid,
+                                            MemoryRegionSection *section)
+{
+    hwaddr start_addr = section->offset_within_address_space;
+    ram_addr_t size = int128_get64(section->size);
+    hwaddr end_addr = start_addr + size - 1;
+
+    trace_xen_unmap_mmio_range(ioservid, start_addr, end_addr);
+    xc_hvm_unmap_io_range_from_ioreq_server(xc, dom, ioservid, 1,
+                                            start_addr, end_addr);
+}
+
+static inline void xen_map_io_section(XenXC xc, domid_t dom,
+                                      ioservid_t ioservid,
+                                      MemoryRegionSection *section)
+{
+    hwaddr start_addr = section->offset_within_address_space;
+    ram_addr_t size = int128_get64(section->size);
+    hwaddr end_addr = start_addr + size - 1;
+
+    trace_xen_map_portio_range(ioservid, start_addr, end_addr);
+    xc_hvm_map_io_range_to_ioreq_server(xc, dom, ioservid, 0,
+                                        start_addr, end_addr);
+}
+
+static inline void xen_unmap_io_section(XenXC xc, domid_t dom,
+                                        ioservid_t ioservid,
+                                        MemoryRegionSection *section)
+{
+    hwaddr start_addr = section->offset_within_address_space;
+    ram_addr_t size = int128_get64(section->size);
+    hwaddr end_addr = start_addr + size - 1;
+
+    trace_xen_unmap_portio_range(ioservid, start_addr, end_addr);
+    xc_hvm_unmap_io_range_from_ioreq_server(xc, dom, ioservid, 0,
+                                            start_addr, end_addr);
+}
+
+static inline void xen_map_pcidev(XenXC xc, domid_t dom,
+                                  ioservid_t ioservid,
+                                  PCIDevice *pci_dev)
+{
+    trace_xen_map_pcidev(ioservid, pci_bus_num(pci_dev->bus),
+                         PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn));
+    xc_hvm_map_pcidev_to_ioreq_server(xc, dom, ioservid,
+                                      0, pci_bus_num(pci_dev->bus),
+                                      PCI_SLOT(pci_dev->devfn),
+                                      PCI_FUNC(pci_dev->devfn));
+}
+
+static inline void xen_unmap_pcidev(XenXC xc, domid_t dom,
+                                    ioservid_t ioservid,
+                                    PCIDevice *pci_dev)
+{
+    trace_xen_unmap_pcidev(ioservid, pci_bus_num(pci_dev->bus),
+                           PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn));
+    xc_hvm_unmap_pcidev_from_ioreq_server(xc, dom, ioservid,
+                                          0, pci_bus_num(pci_dev->bus),
+                                          PCI_SLOT(pci_dev->devfn),
+                                          PCI_FUNC(pci_dev->devfn));
+}
+
+static inline int xen_create_ioreq_server(XenXC xc, domid_t dom,
+                                          ioservid_t *ioservid)
+{
+    int rc = xc_hvm_create_ioreq_server(xc, dom, 1, ioservid);
+
+    if (rc == 0) {
+        trace_xen_ioreq_server_create(*ioservid);
+    }
+
+    return rc;
+}
+
+static inline void xen_destroy_ioreq_server(XenXC xc, domid_t dom,
+                                            ioservid_t ioservid)
+{
+    trace_xen_ioreq_server_destroy(ioservid);
+    xc_hvm_destroy_ioreq_server(xc, dom, ioservid);
+}
+
+static inline int xen_get_ioreq_server_info(XenXC xc, domid_t dom,
+                                            ioservid_t ioservid,
+                                            xen_pfn_t *ioreq_pfn,
+                                            xen_pfn_t *bufioreq_pfn,
+                                            evtchn_port_t *bufioreq_evtchn)
+{
+    return xc_hvm_get_ioreq_server_info(xc, dom, ioservid,
+                                        ioreq_pfn, bufioreq_pfn,
+                                        bufioreq_evtchn);
+}
+
+static inline int xen_set_ioreq_server_state(XenXC xc, domid_t dom,
+                                             ioservid_t ioservid,
+                                             bool enable)
+{
+    return xc_hvm_set_ioreq_server_state(xc, dom, ioservid, enable);
+}
+
+#endif
+
 #endif /* QEMU_HW_XEN_COMMON_H */
diff --git a/trace-events b/trace-events
index 011d105..3efcff7 100644
--- a/trace-events
+++ b/trace-events
@@ -895,6 +895,14 @@ pvscsi_tx_rings_num_pages(const char* label, uint32_t num) "Number of %s pages:
 # xen-hvm.c
 xen_ram_alloc(unsigned long ram_addr, unsigned long size) "requested: %#lx, size %#lx"
 xen_client_set_memory(uint64_t start_addr, unsigned long size, bool log_dirty) "%#"PRIx64" size %#lx, log_dirty %i"
+xen_ioreq_server_create(uint32_t id) "id: %u"
+xen_ioreq_server_destroy(uint32_t id) "id: %u"
+xen_map_mmio_range(uint32_t id, uint64_t start_addr, uint64_t end_addr) "id: %u start: %#"PRIx64" end: %#"PRIx64
+xen_unmap_mmio_range(uint32_t id, uint64_t start_addr, uint64_t end_addr) "id: %u start: %#"PRIx64" end: %#"PRIx64
+xen_map_portio_range(uint32_t id, uint64_t start_addr, uint64_t end_addr) "id: %u start: %#"PRIx64" end: %#"PRIx64
+xen_unmap_portio_range(uint32_t id, uint64_t start_addr, uint64_t end_addr) "id: %u start: %#"PRIx64" end: %#"PRIx64
+xen_map_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: %u bdf: %02x.%02x.%02x"
+xen_unmap_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: %u bdf: %02x.%02x.%02x"
 
 # xen-mapcache.c
 xen_map_cache(uint64_t phys_addr) "want %#"PRIx64
diff --git a/xen-hvm.c b/xen-hvm.c
index 05e522c..0bbbf2a 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -62,9 +62,6 @@ static inline ioreq_t *xen_vcpu_ioreq(shared_iopage_t *shared_page, int vcpu)
 }
 #  define FMT_ioreq_size "u"
 #endif
-#ifndef HVM_PARAM_BUFIOREQ_EVTCHN
-#define HVM_PARAM_BUFIOREQ_EVTCHN 26
-#endif
 
 #define BUFFER_IO_MAX_DELAY  100
 
@@ -78,6 +75,7 @@ typedef struct XenPhysmap {
 } XenPhysmap;
 
 typedef struct XenIOState {
+    ioservid_t ioservid;
     shared_iopage_t *shared_page;
     buffered_iopage_t *buffered_io_page;
     QEMUTimer *buffered_io_timer;
@@ -92,6 +90,8 @@ typedef struct XenIOState {
 
     struct xs_handle *xenstore;
     MemoryListener memory_listener;
+    MemoryListener io_listener;
+    DeviceListener device_listener;
     QLIST_HEAD(, XenPhysmap) physmap;
     hwaddr free_phys_offset;
     const XenPhysmap *log_for_dirtybit;
@@ -442,12 +442,23 @@ static void xen_set_memory(struct MemoryListener *listener,
     bool log_dirty = memory_region_is_logging(section->mr);
     hvmmem_type_t mem_type;
 
+    if (section->mr == &ram_memory) {
+        return;
+    } else {
+        if (add) {
+            xen_map_memory_section(xen_xc, xen_domid, state->ioservid,
+                                   section);
+        } else {
+            xen_unmap_memory_section(xen_xc, xen_domid, state->ioservid,
+                                     section);
+        }
+    }
+
     if (!memory_region_is_ram(section->mr)) {
         return;
     }
 
-    if (!(section->mr != &ram_memory
-          && ( (log_dirty && add) || (!log_dirty && !add)))) {
+    if (!(log_dirty && add) && !(!log_dirty && !add)) {
         return;
     }
 
@@ -480,6 +491,7 @@ static void xen_region_add(MemoryListener *listener,
                            MemoryRegionSection *section)
 {
     memory_region_ref(section->mr);
+
     xen_set_memory(listener, section, true);
 }
 
@@ -487,9 +499,54 @@ static void xen_region_del(MemoryListener *listener,
                            MemoryRegionSection *section)
 {
     xen_set_memory(listener, section, false);
+
     memory_region_unref(section->mr);
 }
 
+static void xen_io_add(MemoryListener *listener,
+                       MemoryRegionSection *section)
+{
+    XenIOState *state = container_of(listener, XenIOState, io_listener);
+
+    memory_region_ref(section->mr);
+
+    xen_map_io_section(xen_xc, xen_domid, state->ioservid, section);
+}
+
+static void xen_io_del(MemoryListener *listener,
+                       MemoryRegionSection *section)
+{
+    XenIOState *state = container_of(listener, XenIOState, io_listener);
+
+    xen_unmap_io_section(xen_xc, xen_domid, state->ioservid, section);
+
+    memory_region_unref(section->mr);
+}
+
+static void xen_device_realize(DeviceListener *listener,
+			       DeviceState *dev)
+{
+    XenIOState *state = container_of(listener, XenIOState, device_listener);
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+        PCIDevice *pci_dev = PCI_DEVICE(dev);
+
+        xen_map_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev);
+    }
+}
+
+static void xen_device_unrealize(DeviceListener *listener,
+				 DeviceState *dev)
+{
+    XenIOState *state = container_of(listener, XenIOState, device_listener);
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+        PCIDevice *pci_dev = PCI_DEVICE(dev);
+
+        xen_unmap_pcidev(xen_xc, xen_domid, state->ioservid, pci_dev);
+    }
+}
+
 static void xen_sync_dirty_bitmap(XenIOState *state,
                                   hwaddr start_addr,
                                   ram_addr_t size)
@@ -590,6 +647,17 @@ static MemoryListener xen_memory_listener = {
     .priority = 10,
 };
 
+static MemoryListener xen_io_listener = {
+    .region_add = xen_io_add,
+    .region_del = xen_io_del,
+    .priority = 10,
+};
+
+static DeviceListener xen_device_listener = {
+    .realize = xen_device_realize,
+    .unrealize = xen_device_unrealize,
+};
+
 /* get the ioreq packets from share mem */
 static ioreq_t *cpu_get_ioreq_from_shared_memory(XenIOState *state, int vcpu)
 {
@@ -792,6 +860,27 @@ static void handle_ioreq(ioreq_t *req)
         case IOREQ_TYPE_INVALIDATE:
             xen_invalidate_map_cache();
             break;
+        case IOREQ_TYPE_PCI_CONFIG: {
+            uint32_t sbdf = req->addr >> 32;
+            uint32_t val;
+
+            /* Fake a write to port 0xCF8 so that
+             * the config space access will target the
+             * correct device model.
+             */
+            val = (1u << 31) |
+                  ((req->addr & 0x0f00) << 16) |
+                  ((sbdf & 0xffff) << 8) |
+                  (req->addr & 0xfc);
+            do_outp(0xcf8, 4, val);
+
+            /* Now issue the config space access via
+             * port 0xCFC
+             */
+            req->addr = 0xcfc | (req->addr & 0x03);
+            cpu_ioreq_pio(req);
+            break;
+        }
         default:
             hw_error("Invalid ioreq type 0x%x\n", req->type);
     }
@@ -979,13 +1068,33 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
     xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 0);
 }
 
+static void xen_hvm_pre_save(void *opaque)
+{
+    XenIOState *state = opaque;
+
+    /* Stop servicing emulation requests */
+    xen_set_ioreq_server_state(xen_xc, xen_domid, state->ioservid, 0);
+    xen_destroy_ioreq_server(xen_xc, xen_domid, state->ioservid);
+}
+
+static const VMStateDescription vmstate_xen_hvm = {
+    .name = "xen-hvm",
+    .version_id = 4,
+    .minimum_version_id = 4,
+    .pre_save = xen_hvm_pre_save,
+    .fields = (VMStateField[]) {
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 /* return 0 means OK, or -1 means critical issue -- will exit(1) */
 int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
                  MemoryRegion **ram_memory)
 {
     int i, rc;
-    unsigned long ioreq_pfn;
-    unsigned long bufioreq_evtchn;
+    xen_pfn_t ioreq_pfn;
+    xen_pfn_t bufioreq_pfn;
+    evtchn_port_t bufioreq_evtchn;
     XenIOState *state;
 
     state = g_malloc0(sizeof (XenIOState));
@@ -1002,6 +1111,12 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
         return -1;
     }
 
+    rc = xen_create_ioreq_server(xen_xc, xen_domid, &state->ioservid);
+    if (rc < 0) {
+        perror("xen: ioreq server create");
+        return -1;
+    }
+
     state->exit.notify = xen_exit_notifier;
     qemu_add_exit_notifier(&state->exit);
 
@@ -1011,8 +1126,18 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
     state->wakeup.notify = xen_wakeup_notifier;
     qemu_register_wakeup_notifier(&state->wakeup);
 
-    xc_get_hvm_param(xen_xc, xen_domid, HVM_PARAM_IOREQ_PFN, &ioreq_pfn);
+    rc = xen_get_ioreq_server_info(xen_xc, xen_domid, state->ioservid,
+                                   &ioreq_pfn, &bufioreq_pfn,
+                                   &bufioreq_evtchn);
+    if (rc < 0) {
+        hw_error("failed to get ioreq server info: error %d handle=" XC_INTERFACE_FMT,
+                 errno, xen_xc);
+    }
+
     DPRINTF("shared page at pfn %lx\n", ioreq_pfn);
+    DPRINTF("buffered io page at pfn %lx\n", bufioreq_pfn);
+    DPRINTF("buffered io evtchn is %x\n", bufioreq_evtchn);
+
     state->shared_page = xc_map_foreign_range(xen_xc, xen_domid, XC_PAGE_SIZE,
                                               PROT_READ|PROT_WRITE, ioreq_pfn);
     if (state->shared_page == NULL) {
@@ -1020,14 +1145,20 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
                  errno, xen_xc);
     }
 
-    xc_get_hvm_param(xen_xc, xen_domid, HVM_PARAM_BUFIOREQ_PFN, &ioreq_pfn);
-    DPRINTF("buffered io page at pfn %lx\n", ioreq_pfn);
-    state->buffered_io_page = xc_map_foreign_range(xen_xc, xen_domid, XC_PAGE_SIZE,
-                                                   PROT_READ|PROT_WRITE, ioreq_pfn);
+    state->buffered_io_page = xc_map_foreign_range(xen_xc, xen_domid,
+                                                   XC_PAGE_SIZE,
+                                                   PROT_READ|PROT_WRITE,
+                                                   bufioreq_pfn);
     if (state->buffered_io_page == NULL) {
         hw_error("map buffered IO page returned error %d", errno);
     }
 
+    rc = xen_set_ioreq_server_state(xen_xc, xen_domid, state->ioservid, true);
+    if (rc < 0) {
+        hw_error("failed to enable ioreq server info: error %d handle=" XC_INTERFACE_FMT,
+                 errno, xen_xc);
+    }
+
     state->ioreq_local_port = g_malloc0(max_cpus * sizeof (evtchn_port_t));
 
     /* FIXME: how about if we overflow the page here? */
@@ -1035,22 +1166,16 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
         rc = xc_evtchn_bind_interdomain(state->xce_handle, xen_domid,
                                         xen_vcpu_eport(state->shared_page, i));
         if (rc == -1) {
-            fprintf(stderr, "bind interdomain ioctl error %d\n", errno);
+            fprintf(stderr, "shared evtchn %d bind error %d\n", i, errno);
             return -1;
         }
         state->ioreq_local_port[i] = rc;
     }
 
-    rc = xc_get_hvm_param(xen_xc, xen_domid, HVM_PARAM_BUFIOREQ_EVTCHN,
-            &bufioreq_evtchn);
-    if (rc < 0) {
-        fprintf(stderr, "failed to get HVM_PARAM_BUFIOREQ_EVTCHN\n");
-        return -1;
-    }
     rc = xc_evtchn_bind_interdomain(state->xce_handle, xen_domid,
-            (uint32_t)bufioreq_evtchn);
+                                    bufioreq_evtchn);
     if (rc == -1) {
-        fprintf(stderr, "bind interdomain ioctl error %d\n", errno);
+        fprintf(stderr, "buffered evtchn bind error %d\n", errno);
         return -1;
     }
     state->bufioreq_local_port = rc;
@@ -1060,12 +1185,19 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
     xen_ram_init(below_4g_mem_size, above_4g_mem_size, ram_size, ram_memory);
 
     qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
+    vmstate_register(NULL, 0, &vmstate_xen_hvm, state);
 
     state->memory_listener = xen_memory_listener;
     QLIST_INIT(&state->physmap);
     memory_listener_register(&state->memory_listener, &address_space_memory);
     state->log_for_dirtybit = NULL;
 
+    state->io_listener = xen_io_listener;
+    memory_listener_register(&state->io_listener, &address_space_io);
+
+    state->device_listener = xen_device_listener;
+    qdev_listener_register(&state->device_listener);
+
     /* Initialize backend core & drivers */
     if (xen_be_init() != 0) {
         fprintf(stderr, "%s: xen backend core setup failed\n", __FUNCTION__);
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH v3 2/2] Xen: Use the ioreq-server API when available

[Stefano Stabellini]

On Wed, 15 Oct 2014, Paul Durrant wrote:
> The ioreq-server API added to Xen 4.5 offers better security than
> the existing Xen/QEMU interface because the shared pages that are
> used to pass emulation request/results back and forth are removed
> from the guest's memory space before any requests are serviced.
> This prevents the guest from mapping these pages (they are in a
> well known location) and attempting to attack QEMU by synthesizing
> its own request structures. Hence, this patch modifies configure
> to detect whether the API is available, and adds the necessary
> code to use the API if it is.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

The patch is OK, so you can add my Acked-by.
I have a couple of minor comments below. If you need to repost it then
would be nice if you could address them.


> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Michael Tokarev <mjt@tls.msk.ru>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Stefan Weil <sw@weilnetz.de>
> Cc: Olaf Hering <olaf@aepfle.de>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> Cc: Alexander Graf <agraf@suse.de>
> ---
>  configure                   |   29 ++++++
>  include/hw/xen/xen_common.h |  222 +++++++++++++++++++++++++++++++++++++++++++
>  trace-events                |    8 ++
>  xen-hvm.c                   |  174 +++++++++++++++++++++++++++++----
>  4 files changed, 412 insertions(+), 21 deletions(-)
> 

[...]

> diff --git a/xen-hvm.c b/xen-hvm.c
> index 05e522c..0bbbf2a 100644
> --- a/xen-hvm.c
> +++ b/xen-hvm.c
> @@ -62,9 +62,6 @@ static inline ioreq_t *xen_vcpu_ioreq(shared_iopage_t *shared_page, int vcpu)
>  }
>  #  define FMT_ioreq_size "u"
>  #endif
> -#ifndef HVM_PARAM_BUFIOREQ_EVTCHN
> -#define HVM_PARAM_BUFIOREQ_EVTCHN 26
> -#endif
>  
>  #define BUFFER_IO_MAX_DELAY  100
>  
> @@ -78,6 +75,7 @@ typedef struct XenPhysmap {
>  } XenPhysmap;
>  
>  typedef struct XenIOState {
> +    ioservid_t ioservid;
>      shared_iopage_t *shared_page;
>      buffered_iopage_t *buffered_io_page;
>      QEMUTimer *buffered_io_timer;
> @@ -92,6 +90,8 @@ typedef struct XenIOState {
>  
>      struct xs_handle *xenstore;
>      MemoryListener memory_listener;
> +    MemoryListener io_listener;
> +    DeviceListener device_listener;
>      QLIST_HEAD(, XenPhysmap) physmap;
>      hwaddr free_phys_offset;
>      const XenPhysmap *log_for_dirtybit;
> @@ -442,12 +442,23 @@ static void xen_set_memory(struct MemoryListener *listener,
>      bool log_dirty = memory_region_is_logging(section->mr);
>      hvmmem_type_t mem_type;
>  
> +    if (section->mr == &ram_memory) {
> +        return;
> +    } else {
> +        if (add) {
> +            xen_map_memory_section(xen_xc, xen_domid, state->ioservid,
> +                                   section);
> +        } else {
> +            xen_unmap_memory_section(xen_xc, xen_domid, state->ioservid,
> +                                     section);
> +        }
> +    }
>      if (!memory_region_is_ram(section->mr)) {
>          return;
>      }
>  
> -    if (!(section->mr != &ram_memory
> -          && ( (log_dirty && add) || (!log_dirty && !add)))) {
> +    if (!(log_dirty && add) && !(!log_dirty && !add)) {
>          return;

if (!((log_dirty && add) || (!log_dirty && !add)))



>      }
>  
> @@ -480,6 +491,7 @@ static void xen_region_add(MemoryListener *listener,
>                             MemoryRegionSection *section)
>  {
>      memory_region_ref(section->mr);
> +
>      xen_set_memory(listener, section, true);
>  }
>  
> @@ -487,9 +499,54 @@ static void xen_region_del(MemoryListener *listener,
>                             MemoryRegionSection *section)
>  {
>      xen_set_memory(listener, section, false);
> +
>      memory_region_unref(section->mr);
>  }

Useless changes?

Re: [Qemu-devel] [PATCH v3 2/2] Xen: Use the ioreq-server API when available

[Paul Durrant]

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: 15 October 2014 15:38
> To: Paul Durrant
> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; Stefano
> Stabellini; Peter Maydell; Paolo Bonzini; Michael Tokarev; Stefan Hajnoczi;
> Stefan Weil; Olaf Hering; Gerd Hoffmann; Alexey Kardashevskiy; Alexander
> Graf
> Subject: Re: [PATCH v3 2/2] Xen: Use the ioreq-server API when available
> 
> On Wed, 15 Oct 2014, Paul Durrant wrote:
> > The ioreq-server API added to Xen 4.5 offers better security than
> > the existing Xen/QEMU interface because the shared pages that are
> > used to pass emulation request/results back and forth are removed
> > from the guest's memory space before any requests are serviced.
> > This prevents the guest from mapping these pages (they are in a
> > well known location) and attempting to attack QEMU by synthesizing
> > its own request structures. Hence, this patch modifies configure
> > to detect whether the API is available, and adds the necessary
> > code to use the API if it is.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> 
> The patch is OK, so you can add my Acked-by.
> I have a couple of minor comments below. If you need to repost it then
> would be nice if you could address them.
> 
> 
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Michael Tokarev <mjt@tls.msk.ru>
> > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > Cc: Stefan Weil <sw@weilnetz.de>
> > Cc: Olaf Hering <olaf@aepfle.de>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> > Cc: Alexander Graf <agraf@suse.de>
> > ---
> >  configure                   |   29 ++++++
> >  include/hw/xen/xen_common.h |  222
> +++++++++++++++++++++++++++++++++++++++++++
> >  trace-events                |    8 ++
> >  xen-hvm.c                   |  174 +++++++++++++++++++++++++++++----
> >  4 files changed, 412 insertions(+), 21 deletions(-)
> >
> 
> [...]
> 
> > diff --git a/xen-hvm.c b/xen-hvm.c
> > index 05e522c..0bbbf2a 100644
> > --- a/xen-hvm.c
> > +++ b/xen-hvm.c
> > @@ -62,9 +62,6 @@ static inline ioreq_t
> *xen_vcpu_ioreq(shared_iopage_t *shared_page, int vcpu)
> >  }
> >  #  define FMT_ioreq_size "u"
> >  #endif
> > -#ifndef HVM_PARAM_BUFIOREQ_EVTCHN
> > -#define HVM_PARAM_BUFIOREQ_EVTCHN 26
> > -#endif
> >
> >  #define BUFFER_IO_MAX_DELAY  100
> >
> > @@ -78,6 +75,7 @@ typedef struct XenPhysmap {
> >  } XenPhysmap;
> >
> >  typedef struct XenIOState {
> > +    ioservid_t ioservid;
> >      shared_iopage_t *shared_page;
> >      buffered_iopage_t *buffered_io_page;
> >      QEMUTimer *buffered_io_timer;
> > @@ -92,6 +90,8 @@ typedef struct XenIOState {
> >
> >      struct xs_handle *xenstore;
> >      MemoryListener memory_listener;
> > +    MemoryListener io_listener;
> > +    DeviceListener device_listener;
> >      QLIST_HEAD(, XenPhysmap) physmap;
> >      hwaddr free_phys_offset;
> >      const XenPhysmap *log_for_dirtybit;
> > @@ -442,12 +442,23 @@ static void xen_set_memory(struct
> MemoryListener *listener,
> >      bool log_dirty = memory_region_is_logging(section->mr);
> >      hvmmem_type_t mem_type;
> >
> > +    if (section->mr == &ram_memory) {
> > +        return;
> > +    } else {
> > +        if (add) {
> > +            xen_map_memory_section(xen_xc, xen_domid, state->ioservid,
> > +                                   section);
> > +        } else {
> > +            xen_unmap_memory_section(xen_xc, xen_domid, state->ioservid,
> > +                                     section);
> > +        }
> > +    }
> >      if (!memory_region_is_ram(section->mr)) {
> >          return;
> >      }
> >
> > -    if (!(section->mr != &ram_memory
> > -          && ( (log_dirty && add) || (!log_dirty && !add)))) {
> > +    if (!(log_dirty && add) && !(!log_dirty && !add)) {
> >          return;
> 
> if (!((log_dirty && add) || (!log_dirty && !add)))
> 

I'm not sure which is better TBH.

> 
> 
> >      }
> >
> > @@ -480,6 +491,7 @@ static void xen_region_add(MemoryListener
> *listener,
> >                             MemoryRegionSection *section)
> >  {
> >      memory_region_ref(section->mr);
> > +
> >      xen_set_memory(listener, section, true);
> >  }
> >
> > @@ -487,9 +499,54 @@ static void xen_region_del(MemoryListener
> *listener,
> >                             MemoryRegionSection *section)
> >  {
> >      xen_set_memory(listener, section, false);
> > +
> >      memory_region_unref(section->mr);
> >  }
> 
> Useless changes?

Yes. Unfortunately I only noticed after posting. I will definitely clean up if I re-post.

  Paul

Re: [Qemu-devel] [Xen-devel] [PATCH v3 2/2] Xen: Use the ioreq-server API when available

[Andrew Cooper]

On 15/10/14 15:51, Paul Durrant wrote:
>> -----Original Message-----
>> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
>> Sent: 15 October 2014 15:38
>> To: Paul Durrant
>> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; Stefano
>> Stabellini; Peter Maydell; Paolo Bonzini; Michael Tokarev; Stefan Hajnoczi;
>> Stefan Weil; Olaf Hering; Gerd Hoffmann; Alexey Kardashevskiy; Alexander
>> Graf
>> Subject: Re: [PATCH v3 2/2] Xen: Use the ioreq-server API when available
>>
>> On Wed, 15 Oct 2014, Paul Durrant wrote:
>>> The ioreq-server API added to Xen 4.5 offers better security than
>>> the existing Xen/QEMU interface because the shared pages that are
>>> used to pass emulation request/results back and forth are removed
>>> from the guest's memory space before any requests are serviced.
>>> This prevents the guest from mapping these pages (they are in a
>>> well known location) and attempting to attack QEMU by synthesizing
>>> its own request structures. Hence, this patch modifies configure
>>> to detect whether the API is available, and adds the necessary
>>> code to use the API if it is.
>>>
>>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>> The patch is OK, so you can add my Acked-by.
>> I have a couple of minor comments below. If you need to repost it then
>> would be nice if you could address them.
>>
>>
>>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Michael Tokarev <mjt@tls.msk.ru>
>>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>>> Cc: Stefan Weil <sw@weilnetz.de>
>>> Cc: Olaf Hering <olaf@aepfle.de>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> Cc: Alexander Graf <agraf@suse.de>
>>> ---
>>>  configure                   |   29 ++++++
>>>  include/hw/xen/xen_common.h |  222
>> +++++++++++++++++++++++++++++++++++++++++++
>>>  trace-events                |    8 ++
>>>  xen-hvm.c                   |  174 +++++++++++++++++++++++++++++----
>>>  4 files changed, 412 insertions(+), 21 deletions(-)
>>>
>> [...]
>>
>>> diff --git a/xen-hvm.c b/xen-hvm.c
>>> index 05e522c..0bbbf2a 100644
>>> --- a/xen-hvm.c
>>> +++ b/xen-hvm.c
>>> @@ -62,9 +62,6 @@ static inline ioreq_t
>> *xen_vcpu_ioreq(shared_iopage_t *shared_page, int vcpu)
>>>  }
>>>  #  define FMT_ioreq_size "u"
>>>  #endif
>>> -#ifndef HVM_PARAM_BUFIOREQ_EVTCHN
>>> -#define HVM_PARAM_BUFIOREQ_EVTCHN 26
>>> -#endif
>>>
>>>  #define BUFFER_IO_MAX_DELAY  100
>>>
>>> @@ -78,6 +75,7 @@ typedef struct XenPhysmap {
>>>  } XenPhysmap;
>>>
>>>  typedef struct XenIOState {
>>> +    ioservid_t ioservid;
>>>      shared_iopage_t *shared_page;
>>>      buffered_iopage_t *buffered_io_page;
>>>      QEMUTimer *buffered_io_timer;
>>> @@ -92,6 +90,8 @@ typedef struct XenIOState {
>>>
>>>      struct xs_handle *xenstore;
>>>      MemoryListener memory_listener;
>>> +    MemoryListener io_listener;
>>> +    DeviceListener device_listener;
>>>      QLIST_HEAD(, XenPhysmap) physmap;
>>>      hwaddr free_phys_offset;
>>>      const XenPhysmap *log_for_dirtybit;
>>> @@ -442,12 +442,23 @@ static void xen_set_memory(struct
>> MemoryListener *listener,
>>>      bool log_dirty = memory_region_is_logging(section->mr);
>>>      hvmmem_type_t mem_type;
>>>
>>> +    if (section->mr == &ram_memory) {
>>> +        return;
>>> +    } else {
>>> +        if (add) {
>>> +            xen_map_memory_section(xen_xc, xen_domid, state->ioservid,
>>> +                                   section);
>>> +        } else {
>>> +            xen_unmap_memory_section(xen_xc, xen_domid, state->ioservid,
>>> +                                     section);
>>> +        }
>>> +    }
>>>      if (!memory_region_is_ram(section->mr)) {
>>>          return;
>>>      }
>>>
>>> -    if (!(section->mr != &ram_memory
>>> -          && ( (log_dirty && add) || (!log_dirty && !add)))) {
>>> +    if (!(log_dirty && add) && !(!log_dirty && !add)) {
>>>          return;
>> if (!((log_dirty && add) || (!log_dirty && !add)))
>>
> I'm not sure which is better TBH.

I set simplification problems like this to my Part 1a digital
electronics supervisees.

This is "if (!(log_dirty ^ add))" as they are both booleans, and reads
rather more easily that either of the above.

~Andrew

Re: [Qemu-devel] [Xen-devel] [PATCH v3 2/2] Xen: Use the ioreq-server API when available

[Stefano Stabellini]

On Wed, 15 Oct 2014, Andrew Cooper wrote:
> On 15/10/14 15:51, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> >> Sent: 15 October 2014 15:38
> >> To: Paul Durrant
> >> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; Stefano
> >> Stabellini; Peter Maydell; Paolo Bonzini; Michael Tokarev; Stefan Hajnoczi;
> >> Stefan Weil; Olaf Hering; Gerd Hoffmann; Alexey Kardashevskiy; Alexander
> >> Graf
> >> Subject: Re: [PATCH v3 2/2] Xen: Use the ioreq-server API when available
> >>
> >> On Wed, 15 Oct 2014, Paul Durrant wrote:
> >>> The ioreq-server API added to Xen 4.5 offers better security than
> >>> the existing Xen/QEMU interface because the shared pages that are
> >>> used to pass emulation request/results back and forth are removed
> >>> from the guest's memory space before any requests are serviced.
> >>> This prevents the guest from mapping these pages (they are in a
> >>> well known location) and attempting to attack QEMU by synthesizing
> >>> its own request structures. Hence, this patch modifies configure
> >>> to detect whether the API is available, and adds the necessary
> >>> code to use the API if it is.
> >>>
> >>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> >> The patch is OK, so you can add my Acked-by.
> >> I have a couple of minor comments below. If you need to repost it then
> >> would be nice if you could address them.
> >>
> >>
> >>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >>> Cc: Peter Maydell <peter.maydell@linaro.org>
> >>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >>> Cc: Michael Tokarev <mjt@tls.msk.ru>
> >>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> >>> Cc: Stefan Weil <sw@weilnetz.de>
> >>> Cc: Olaf Hering <olaf@aepfle.de>
> >>> Cc: Gerd Hoffmann <kraxel@redhat.com>
> >>> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>> Cc: Alexander Graf <agraf@suse.de>
> >>> ---
> >>>  configure                   |   29 ++++++
> >>>  include/hw/xen/xen_common.h |  222
> >> +++++++++++++++++++++++++++++++++++++++++++
> >>>  trace-events                |    8 ++
> >>>  xen-hvm.c                   |  174 +++++++++++++++++++++++++++++----
> >>>  4 files changed, 412 insertions(+), 21 deletions(-)
> >>>
> >> [...]
> >>
> >>> diff --git a/xen-hvm.c b/xen-hvm.c
> >>> index 05e522c..0bbbf2a 100644
> >>> --- a/xen-hvm.c
> >>> +++ b/xen-hvm.c
> >>> @@ -62,9 +62,6 @@ static inline ioreq_t
> >> *xen_vcpu_ioreq(shared_iopage_t *shared_page, int vcpu)
> >>>  }
> >>>  #  define FMT_ioreq_size "u"
> >>>  #endif
> >>> -#ifndef HVM_PARAM_BUFIOREQ_EVTCHN
> >>> -#define HVM_PARAM_BUFIOREQ_EVTCHN 26
> >>> -#endif
> >>>
> >>>  #define BUFFER_IO_MAX_DELAY  100
> >>>
> >>> @@ -78,6 +75,7 @@ typedef struct XenPhysmap {
> >>>  } XenPhysmap;
> >>>
> >>>  typedef struct XenIOState {
> >>> +    ioservid_t ioservid;
> >>>      shared_iopage_t *shared_page;
> >>>      buffered_iopage_t *buffered_io_page;
> >>>      QEMUTimer *buffered_io_timer;
> >>> @@ -92,6 +90,8 @@ typedef struct XenIOState {
> >>>
> >>>      struct xs_handle *xenstore;
> >>>      MemoryListener memory_listener;
> >>> +    MemoryListener io_listener;
> >>> +    DeviceListener device_listener;
> >>>      QLIST_HEAD(, XenPhysmap) physmap;
> >>>      hwaddr free_phys_offset;
> >>>      const XenPhysmap *log_for_dirtybit;
> >>> @@ -442,12 +442,23 @@ static void xen_set_memory(struct
> >> MemoryListener *listener,
> >>>      bool log_dirty = memory_region_is_logging(section->mr);
> >>>      hvmmem_type_t mem_type;
> >>>
> >>> +    if (section->mr == &ram_memory) {
> >>> +        return;
> >>> +    } else {
> >>> +        if (add) {
> >>> +            xen_map_memory_section(xen_xc, xen_domid, state->ioservid,
> >>> +                                   section);
> >>> +        } else {
> >>> +            xen_unmap_memory_section(xen_xc, xen_domid, state->ioservid,
> >>> +                                     section);
> >>> +        }
> >>> +    }
> >>>      if (!memory_region_is_ram(section->mr)) {
> >>>          return;
> >>>      }
> >>>
> >>> -    if (!(section->mr != &ram_memory
> >>> -          && ( (log_dirty && add) || (!log_dirty && !add)))) {
> >>> +    if (!(log_dirty && add) && !(!log_dirty && !add)) {
> >>>          return;
> >> if (!((log_dirty && add) || (!log_dirty && !add)))
> >>
> > I'm not sure which is better TBH.
> 
> I set simplification problems like this to my Part 1a digital
> electronics supervisees.
> 
> This is "if (!(log_dirty ^ add))" as they are both booleans, and reads
> rather more easily that either of the above.
 
you are funny

Re: [Qemu-devel] [PATCH v3 2/2] Xen: Use the ioreq-server API when available

[Paul Durrant]

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: 15 October 2014 15:38
> To: Paul Durrant
> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; Stefano
> Stabellini; Peter Maydell; Paolo Bonzini; Michael Tokarev; Stefan Hajnoczi;
> Stefan Weil; Olaf Hering; Gerd Hoffmann; Alexey Kardashevskiy; Alexander
> Graf
> Subject: Re: [PATCH v3 2/2] Xen: Use the ioreq-server API when available
> 
> On Wed, 15 Oct 2014, Paul Durrant wrote:
> > The ioreq-server API added to Xen 4.5 offers better security than
> > the existing Xen/QEMU interface because the shared pages that are
> > used to pass emulation request/results back and forth are removed
> > from the guest's memory space before any requests are serviced.
> > This prevents the guest from mapping these pages (they are in a
> > well known location) and attempting to attack QEMU by synthesizing
> > its own request structures. Hence, this patch modifies configure
> > to detect whether the API is available, and adds the necessary
> > code to use the API if it is.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> 
> The patch is OK, so you can add my Acked-by.
> I have a couple of minor comments below. If you need to repost it then
> would be nice if you could address them.
> 
> 
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Michael Tokarev <mjt@tls.msk.ru>
> > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > Cc: Stefan Weil <sw@weilnetz.de>
> > Cc: Olaf Hering <olaf@aepfle.de>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> > Cc: Alexander Graf <agraf@suse.de>
> > ---
> >  configure                   |   29 ++++++
> >  include/hw/xen/xen_common.h |  222
> +++++++++++++++++++++++++++++++++++++++++++
> >  trace-events                |    8 ++
> >  xen-hvm.c                   |  174 +++++++++++++++++++++++++++++----
> >  4 files changed, 412 insertions(+), 21 deletions(-)
> >
> 
> [...]
> 
> > diff --git a/xen-hvm.c b/xen-hvm.c
> > index 05e522c..0bbbf2a 100644
> > --- a/xen-hvm.c
> > +++ b/xen-hvm.c
> > @@ -62,9 +62,6 @@ static inline ioreq_t
> *xen_vcpu_ioreq(shared_iopage_t *shared_page, int vcpu)
> >  }
> >  #  define FMT_ioreq_size "u"
> >  #endif
> > -#ifndef HVM_PARAM_BUFIOREQ_EVTCHN
> > -#define HVM_PARAM_BUFIOREQ_EVTCHN 26
> > -#endif
> >
> >  #define BUFFER_IO_MAX_DELAY  100
> >
> > @@ -78,6 +75,7 @@ typedef struct XenPhysmap {
> >  } XenPhysmap;
> >
> >  typedef struct XenIOState {
> > +    ioservid_t ioservid;
> >      shared_iopage_t *shared_page;
> >      buffered_iopage_t *buffered_io_page;
> >      QEMUTimer *buffered_io_timer;
> > @@ -92,6 +90,8 @@ typedef struct XenIOState {
> >
> >      struct xs_handle *xenstore;
> >      MemoryListener memory_listener;
> > +    MemoryListener io_listener;
> > +    DeviceListener device_listener;
> >      QLIST_HEAD(, XenPhysmap) physmap;
> >      hwaddr free_phys_offset;
> >      const XenPhysmap *log_for_dirtybit;
> > @@ -442,12 +442,23 @@ static void xen_set_memory(struct
> MemoryListener *listener,
> >      bool log_dirty = memory_region_is_logging(section->mr);
> >      hvmmem_type_t mem_type;
> >
> > +    if (section->mr == &ram_memory) {
> > +        return;
> > +    } else {
> > +        if (add) {
> > +            xen_map_memory_section(xen_xc, xen_domid, state->ioservid,
> > +                                   section);
> > +        } else {
> > +            xen_unmap_memory_section(xen_xc, xen_domid, state->ioservid,
> > +                                     section);
> > +        }
> > +    }
> >      if (!memory_region_is_ram(section->mr)) {
> >          return;
> >      }
> >
> > -    if (!(section->mr != &ram_memory
> > -          && ( (log_dirty && add) || (!log_dirty && !add)))) {
> > +    if (!(log_dirty && add) && !(!log_dirty && !add)) {
> >          return;
> 
> if (!((log_dirty && add) || (!log_dirty && !add)))
> 

Thinking some more about what Andrew said, this is even more simply expressed as

if (add != log_dirty)

is it not?

  Paul

> 
> 
> >      }
> >
> > @@ -480,6 +491,7 @@ static void xen_region_add(MemoryListener
> *listener,
> >                             MemoryRegionSection *section)
> >  {
> >      memory_region_ref(section->mr);
> > +
> >      xen_set_memory(listener, section, true);
> >  }
> >
> > @@ -487,9 +499,54 @@ static void xen_region_del(MemoryListener
> *listener,
> >                             MemoryRegionSection *section)
> >  {
> >      xen_set_memory(listener, section, false);
> > +
> >      memory_region_unref(section->mr);
> >  }
> 
> Useless changes?

Re: [Qemu-devel] [PATCH v3 2/2] Xen: Use the ioreq-server API when available

[Stefano Stabellini]

On Thu, 16 Oct 2014, Paul Durrant wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > Sent: 15 October 2014 15:38
> > To: Paul Durrant
> > Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; Stefano
> > Stabellini; Peter Maydell; Paolo Bonzini; Michael Tokarev; Stefan Hajnoczi;
> > Stefan Weil; Olaf Hering; Gerd Hoffmann; Alexey Kardashevskiy; Alexander
> > Graf
> > Subject: Re: [PATCH v3 2/2] Xen: Use the ioreq-server API when available
> > 
> > On Wed, 15 Oct 2014, Paul Durrant wrote:
> > > The ioreq-server API added to Xen 4.5 offers better security than
> > > the existing Xen/QEMU interface because the shared pages that are
> > > used to pass emulation request/results back and forth are removed
> > > from the guest's memory space before any requests are serviced.
> > > This prevents the guest from mapping these pages (they are in a
> > > well known location) and attempting to attack QEMU by synthesizing
> > > its own request structures. Hence, this patch modifies configure
> > > to detect whether the API is available, and adds the necessary
> > > code to use the API if it is.
> > >
> > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > 
> > The patch is OK, so you can add my Acked-by.
> > I have a couple of minor comments below. If you need to repost it then
> > would be nice if you could address them.
> > 
> > 
> > > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > Cc: Peter Maydell <peter.maydell@linaro.org>
> > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > Cc: Michael Tokarev <mjt@tls.msk.ru>
> > > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > > Cc: Stefan Weil <sw@weilnetz.de>
> > > Cc: Olaf Hering <olaf@aepfle.de>
> > > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > > Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> > > Cc: Alexander Graf <agraf@suse.de>
> > > ---
> > >  configure                   |   29 ++++++
> > >  include/hw/xen/xen_common.h |  222
> > +++++++++++++++++++++++++++++++++++++++++++
> > >  trace-events                |    8 ++
> > >  xen-hvm.c                   |  174 +++++++++++++++++++++++++++++----
> > >  4 files changed, 412 insertions(+), 21 deletions(-)
> > >
> > 
> > [...]
> > 
> > > diff --git a/xen-hvm.c b/xen-hvm.c
> > > index 05e522c..0bbbf2a 100644
> > > --- a/xen-hvm.c
> > > +++ b/xen-hvm.c
> > > @@ -62,9 +62,6 @@ static inline ioreq_t
> > *xen_vcpu_ioreq(shared_iopage_t *shared_page, int vcpu)
> > >  }
> > >  #  define FMT_ioreq_size "u"
> > >  #endif
> > > -#ifndef HVM_PARAM_BUFIOREQ_EVTCHN
> > > -#define HVM_PARAM_BUFIOREQ_EVTCHN 26
> > > -#endif
> > >
> > >  #define BUFFER_IO_MAX_DELAY  100
> > >
> > > @@ -78,6 +75,7 @@ typedef struct XenPhysmap {
> > >  } XenPhysmap;
> > >
> > >  typedef struct XenIOState {
> > > +    ioservid_t ioservid;
> > >      shared_iopage_t *shared_page;
> > >      buffered_iopage_t *buffered_io_page;
> > >      QEMUTimer *buffered_io_timer;
> > > @@ -92,6 +90,8 @@ typedef struct XenIOState {
> > >
> > >      struct xs_handle *xenstore;
> > >      MemoryListener memory_listener;
> > > +    MemoryListener io_listener;
> > > +    DeviceListener device_listener;
> > >      QLIST_HEAD(, XenPhysmap) physmap;
> > >      hwaddr free_phys_offset;
> > >      const XenPhysmap *log_for_dirtybit;
> > > @@ -442,12 +442,23 @@ static void xen_set_memory(struct
> > MemoryListener *listener,
> > >      bool log_dirty = memory_region_is_logging(section->mr);
> > >      hvmmem_type_t mem_type;
> > >
> > > +    if (section->mr == &ram_memory) {
> > > +        return;
> > > +    } else {
> > > +        if (add) {
> > > +            xen_map_memory_section(xen_xc, xen_domid, state->ioservid,
> > > +                                   section);
> > > +        } else {
> > > +            xen_unmap_memory_section(xen_xc, xen_domid, state->ioservid,
> > > +                                     section);
> > > +        }
> > > +    }
> > >      if (!memory_region_is_ram(section->mr)) {
> > >          return;
> > >      }
> > >
> > > -    if (!(section->mr != &ram_memory
> > > -          && ( (log_dirty && add) || (!log_dirty && !add)))) {
> > > +    if (!(log_dirty && add) && !(!log_dirty && !add)) {
> > >          return;
> > 
> > if (!((log_dirty && add) || (!log_dirty && !add)))
> > 
> 
> Thinking some more about what Andrew said, this is even more simply expressed as
> 
> if (add != log_dirty)
> 
> is it not?

Yes, I think that should work.

Re: [Qemu-devel] [PATCH v3 2/2] Xen: Use the ioreq-server API when available

[Peter Maydell]

On 15 October 2014 11:16, Paul Durrant <paul.durrant@citrix.com> wrote:
> The ioreq-server API added to Xen 4.5 offers better security than
> the existing Xen/QEMU interface because the shared pages that are
> used to pass emulation request/results back and forth are removed
> from the guest's memory space before any requests are serviced.
> This prevents the guest from mapping these pages (they are in a
> well known location) and attempting to attack QEMU by synthesizing
> its own request structures. Hence, this patch modifies configure
> to detect whether the API is available, and adds the necessary
> code to use the API if it is.

This commit message doesn't mention it, but presumably this is
all x86-specific given it's in a file which is only used for
x86 Xen?

> +static void xen_hvm_pre_save(void *opaque)
> +{
> +    XenIOState *state = opaque;
> +
> +    /* Stop servicing emulation requests */
> +    xen_set_ioreq_server_state(xen_xc, xen_domid, state->ioservid, 0);
> +    xen_destroy_ioreq_server(xen_xc, xen_domid, state->ioservid);
> +}
> +
> +static const VMStateDescription vmstate_xen_hvm = {
> +    .name = "xen-hvm",
> +    .version_id = 4,
> +    .minimum_version_id = 4,

This is new in upstream so why's it starting at version 4?

> +    .pre_save = xen_hvm_pre_save,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_END_OF_LIST()
> +    },

A vmstate which doesn't actually save any state? This seems
rather suspicious...

> @@ -1060,12 +1185,19 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
>      xen_ram_init(below_4g_mem_size, above_4g_mem_size, ram_size, ram_memory);
>
>      qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
> +    vmstate_register(NULL, 0, &vmstate_xen_hvm, state);

Is the new use of vmstate_register() really necessary?
Usually the state you're saving corresponds to some QOM
device whose vmsd field you can use instead.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 2/2] Xen: Use the ioreq-server API when available

[Paolo Bonzini]

Il 15/10/2014 19:30, Peter Maydell ha scritto:
> On 15 October 2014 11:16, Paul Durrant <paul.durrant@citrix.com> wrote:
>> The ioreq-server API added to Xen 4.5 offers better security than
>> the existing Xen/QEMU interface because the shared pages that are
>> used to pass emulation request/results back and forth are removed
>> from the guest's memory space before any requests are serviced.
>> This prevents the guest from mapping these pages (they are in a
>> well known location) and attempting to attack QEMU by synthesizing
>> its own request structures. Hence, this patch modifies configure
>> to detect whether the API is available, and adds the necessary
>> code to use the API if it is.
> 
> This commit message doesn't mention it, but presumably this is
> all x86-specific given it's in a file which is only used for
> x86 Xen?
> 
>> +static void xen_hvm_pre_save(void *opaque)
>> +{
>> +    XenIOState *state = opaque;
>> +
>> +    /* Stop servicing emulation requests */
>> +    xen_set_ioreq_server_state(xen_xc, xen_domid, state->ioservid, 0);
>> +    xen_destroy_ioreq_server(xen_xc, xen_domid, state->ioservid);
>> +}
>> +
>> +static const VMStateDescription vmstate_xen_hvm = {
>> +    .name = "xen-hvm",
>> +    .version_id = 4,
>> +    .minimum_version_id = 4,
> 
> This is new in upstream so why's it starting at version 4?
> 
>> +    .pre_save = xen_hvm_pre_save,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_END_OF_LIST()
>> +    },
> 
> A vmstate which doesn't actually save any state? This seems
> rather suspicious...
> 
>> @@ -1060,12 +1185,19 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
>>      xen_ram_init(below_4g_mem_size, above_4g_mem_size, ram_size, ram_memory);
>>
>>      qemu_add_vm_change_state_handler(xen_hvm_change_state_handler, state);
>> +    vmstate_register(NULL, 0, &vmstate_xen_hvm, state);
> 
> Is the new use of vmstate_register() really necessary?
> Usually the state you're saving corresponds to some QOM
> device whose vmsd field you can use instead.

In this case, it seems like a job for a vmstate change handler.

Paolo

Re: [Qemu-devel] [PATCH v3 2/2] Xen: Use the ioreq-server API when available

[Paul Durrant]

> -----Original Message-----
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Sent: 16 October 2014 08:37
> To: Peter Maydell; Paul Durrant
> Cc: QEMU Developers; xen-devel@lists.xenproject.org; Stefano Stabellini;
> Michael Tokarev; Stefan Hajnoczi; Stefan Weil; Olaf Hering; Gerd Hoffmann;
> Alexey Kardashevskiy; Alexander Graf
> Subject: Re: [PATCH v3 2/2] Xen: Use the ioreq-server API when available
> 
> Il 15/10/2014 19:30, Peter Maydell ha scritto:
> > On 15 October 2014 11:16, Paul Durrant <paul.durrant@citrix.com> wrote:
> >> The ioreq-server API added to Xen 4.5 offers better security than
> >> the existing Xen/QEMU interface because the shared pages that are
> >> used to pass emulation request/results back and forth are removed
> >> from the guest's memory space before any requests are serviced.
> >> This prevents the guest from mapping these pages (they are in a
> >> well known location) and attempting to attack QEMU by synthesizing
> >> its own request structures. Hence, this patch modifies configure
> >> to detect whether the API is available, and adds the necessary
> >> code to use the API if it is.
> >
> > This commit message doesn't mention it, but presumably this is
> > all x86-specific given it's in a file which is only used for
> > x86 Xen?
> >
> >> +static void xen_hvm_pre_save(void *opaque)
> >> +{
> >> +    XenIOState *state = opaque;
> >> +
> >> +    /* Stop servicing emulation requests */
> >> +    xen_set_ioreq_server_state(xen_xc, xen_domid, state->ioservid, 0);
> >> +    xen_destroy_ioreq_server(xen_xc, xen_domid, state->ioservid);
> >> +}
> >> +
> >> +static const VMStateDescription vmstate_xen_hvm = {
> >> +    .name = "xen-hvm",
> >> +    .version_id = 4,
> >> +    .minimum_version_id = 4,
> >
> > This is new in upstream so why's it starting at version 4?
> >
> >> +    .pre_save = xen_hvm_pre_save,
> >> +    .fields = (VMStateField[]) {
> >> +        VMSTATE_END_OF_LIST()
> >> +    },
> >
> > A vmstate which doesn't actually save any state? This seems
> > rather suspicious...
> >
> >> @@ -1060,12 +1185,19 @@ int xen_hvm_init(ram_addr_t
> *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
> >>      xen_ram_init(below_4g_mem_size, above_4g_mem_size, ram_size,
> ram_memory);
> >>
> >>
> qemu_add_vm_change_state_handler(xen_hvm_change_state_handler,
> state);
> >> +    vmstate_register(NULL, 0, &vmstate_xen_hvm, state);
> >
> > Is the new use of vmstate_register() really necessary?
> > Usually the state you're saving corresponds to some QOM
> > device whose vmsd field you can use instead.
> 
> In this case, it seems like a job for a vmstate change handler.
> 

I looked at that but it did not seem to give me the right semantic, whereas the pre-save callback gave me exactly the right semantic.

  Paul

> Paolo

Re: [Qemu-devel] [PATCH v3 2/2] Xen: Use the ioreq-server API when available

[Paolo Bonzini]

Il 16/10/2014 10:25, Paul Durrant ha scritto:
>>> +static void xen_hvm_pre_save(void *opaque)
>>> +{
>>> +    XenIOState *state = opaque;
>>> +
>>> +    /* Stop servicing emulation requests */
>>> +    xen_set_ioreq_server_state(xen_xc, xen_domid, state->ioservid, 0);
>>> +    xen_destroy_ioreq_server(xen_xc, xen_domid, state->ioservid);
>>> +}
>>
>> Is the new use of vmstate_register() really necessary?
>> Usually the state you're saving corresponds to some QOM
>> device whose vmsd field you can use instead.
> 
> In this case, it seems like a job for a vmstate change handler.
> 
> I looked at that but it did not seem to give me the right semantic,
> whereas the pre-save callback gave me exactly the right semantic.

What exactly is the right semantics?  Note that save _can_ fail, so you
need the ability to roll back to the source machine.  I think this is
missing from your patch, and there is no post_save hook that you can use.

Paolo

Re: [Qemu-devel] [PATCH v3 2/2] Xen: Use the ioreq-server API when available

[Paul Durrant]

> -----Original Message-----
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Sent: 16 October 2014 11:10
> To: Paul Durrant; Peter Maydell
> Cc: QEMU Developers; xen-devel@lists.xenproject.org; Stefano Stabellini;
> Michael Tokarev; Stefan Hajnoczi; Stefan Weil; Olaf Hering; Gerd Hoffmann;
> Alexey Kardashevskiy; Alexander Graf
> Subject: Re: [PATCH v3 2/2] Xen: Use the ioreq-server API when available
> 
> Il 16/10/2014 10:25, Paul Durrant ha scritto:
> >>> +static void xen_hvm_pre_save(void *opaque)
> >>> +{
> >>> +    XenIOState *state = opaque;
> >>> +
> >>> +    /* Stop servicing emulation requests */
> >>> +    xen_set_ioreq_server_state(xen_xc, xen_domid, state->ioservid,
> 0);
> >>> +    xen_destroy_ioreq_server(xen_xc, xen_domid, state->ioservid);
> >>> +}
> >>
> >> Is the new use of vmstate_register() really necessary?
> >> Usually the state you're saving corresponds to some QOM
> >> device whose vmsd field you can use instead.
> >
> > In this case, it seems like a job for a vmstate change handler.
> >
> > I looked at that but it did not seem to give me the right semantic,
> > whereas the pre-save callback gave me exactly the right semantic.
> 
> What exactly is the right semantics?  Note that save _can_ fail, so you
> need the ability to roll back to the source machine.  I think this is
> missing from your patch, and there is no post_save hook that you can use.
> 

I need something that will be called prior to the VM memory image being saved, but if save can fail I will also need something to be called if that occurs too.

  Paul

> Paolo

Re: [Qemu-devel] [PATCH v3 2/2] Xen: Use the ioreq-server API when available

[Paolo Bonzini]

Il 16/10/2014 12:16, Paul Durrant ha scritto:
>> What exactly is the right semantics?  Note that save _can_ fail,
>> so you need the ability to roll back to the source machine.  I
>> think this is missing from your patch, and there is no post_save
>> hook that you can use.
> 
> I need something that will be called prior to the VM memory image
> being saved, but if save can fail I will also need something to be
> called if that occurs too.

Can you check the runstate in the vmstate change callback?  The runstate
to use is RUN_STATE_FINISH_MIGRATE (and then you revert if you get from
there to anything but RUN_STATE_POSTMIGRATE).

...

oh wait, those are the runstate for migration, xen's save-devices-state
command uses something else.  Luckily, the command is synchronous, so
management cannot "poll" the state as is the case for regular migration.
So a patch like this could provide the right runstates:

diff --git a/savevm.c b/savevm.c
index 2d8eb96..f9a8e27 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1144,7 +1144,7 @@ void qmp_xen_save_devices_state(const char *filename, Error **errp)
     int ret;
 
     saved_vm_running = runstate_is_running();
-    vm_stop(RUN_STATE_SAVE_VM);
+    vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
 
     f = qemu_fopen(filename, "wb");
     if (!f) {
@@ -1155,8 +1155,12 @@ void qmp_xen_save_devices_state(const char *filename, Error **errp)
     qemu_fclose(f);
     if (ret < 0) {
         error_set(errp, QERR_IO_ERROR);
+        goto the_end;
     }
 
+    runstate_set(RUN_STATE_POSTMIGRATE);
+    return;
+
  the_end:
     if (saved_vm_running) {
         vm_start();

(feel free to include it in your patches with Signed-off-by: Paolo
Bonzini <pbonzini@redhat.com>).

Alternatively, can you stop/restart emulation always (even on stop/cont
monitor commands) rather than just on migration?  That would make things
even simpler and not need anything like the above savevm.c change.

Paolo

Re: [Qemu-devel] [PATCH v3 2/2] Xen: Use the ioreq-server API when available

[Paul Durrant]

> -----Original Message-----
> From: Paul Durrant
> Sent: 16 October 2014 11:17
> To: 'Paolo Bonzini'; Peter Maydell
> Cc: QEMU Developers; xen-devel@lists.xenproject.org; Stefano Stabellini;
> Michael Tokarev; Stefan Hajnoczi; Stefan Weil; Olaf Hering; Gerd Hoffmann;
> Alexey Kardashevskiy; Alexander Graf
> Subject: RE: [PATCH v3 2/2] Xen: Use the ioreq-server API when available
> 
> > -----Original Message-----
> > From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> > Sent: 16 October 2014 11:10
> > To: Paul Durrant; Peter Maydell
> > Cc: QEMU Developers; xen-devel@lists.xenproject.org; Stefano Stabellini;
> > Michael Tokarev; Stefan Hajnoczi; Stefan Weil; Olaf Hering; Gerd
> Hoffmann;
> > Alexey Kardashevskiy; Alexander Graf
> > Subject: Re: [PATCH v3 2/2] Xen: Use the ioreq-server API when available
> >
> > Il 16/10/2014 10:25, Paul Durrant ha scritto:
> > >>> +static void xen_hvm_pre_save(void *opaque)
> > >>> +{
> > >>> +    XenIOState *state = opaque;
> > >>> +
> > >>> +    /* Stop servicing emulation requests */
> > >>> +    xen_set_ioreq_server_state(xen_xc, xen_domid, state->ioservid,
> > 0);
> > >>> +    xen_destroy_ioreq_server(xen_xc, xen_domid, state->ioservid);
> > >>> +}
> > >>
> > >> Is the new use of vmstate_register() really necessary?
> > >> Usually the state you're saving corresponds to some QOM
> > >> device whose vmsd field you can use instead.
> > >
> > > In this case, it seems like a job for a vmstate change handler.
> > >
> > > I looked at that but it did not seem to give me the right semantic,
> > > whereas the pre-save callback gave me exactly the right semantic.
> >
> > What exactly is the right semantics?  Note that save _can_ fail, so you
> > need the ability to roll back to the source machine.  I think this is
> > missing from your patch, and there is no post_save hook that you can use.
> >
> 
> I need something that will be called prior to the VM memory image being
> saved, but if save can fail I will also need something to be called if that occurs
> too.

Tracing this back through the many layers it looks like a state change notifier may work after all as qmp_xen_save_devices_state() does calls vm_stop() with RUN_STATE_SAVE_VM before calling qemu_save_device_state(). I'll check again.

  Paul

> 
>   Paul
> 
> > Paolo

Re: [Qemu-devel] [PATCH v3 2/2] Xen: Use the ioreq-server API when available

[Paul Durrant]

> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: 15 October 2014 18:30
> To: Paul Durrant
> Cc: QEMU Developers; xen-devel@lists.xenproject.org; Stefano Stabellini;
> Paolo Bonzini; Michael Tokarev; Stefan Hajnoczi; Stefan Weil; Olaf Hering;
> Gerd Hoffmann; Alexey Kardashevskiy; Alexander Graf
> Subject: Re: [PATCH v3 2/2] Xen: Use the ioreq-server API when available
> 
> On 15 October 2014 11:16, Paul Durrant <paul.durrant@citrix.com> wrote:
> > The ioreq-server API added to Xen 4.5 offers better security than
> > the existing Xen/QEMU interface because the shared pages that are
> > used to pass emulation request/results back and forth are removed
> > from the guest's memory space before any requests are serviced.
> > This prevents the guest from mapping these pages (they are in a
> > well known location) and attempting to attack QEMU by synthesizing
> > its own request structures. Hence, this patch modifies configure
> > to detect whether the API is available, and adds the necessary
> > code to use the API if it is.
> 
> This commit message doesn't mention it, but presumably this is
> all x86-specific given it's in a file which is only used for
> x86 Xen?
> 
> > +static void xen_hvm_pre_save(void *opaque)
> > +{
> > +    XenIOState *state = opaque;
> > +
> > +    /* Stop servicing emulation requests */
> > +    xen_set_ioreq_server_state(xen_xc, xen_domid, state->ioservid, 0);
> > +    xen_destroy_ioreq_server(xen_xc, xen_domid, state->ioservid);
> > +}
> > +
> > +static const VMStateDescription vmstate_xen_hvm = {
> > +    .name = "xen-hvm",
> > +    .version_id = 4,
> > +    .minimum_version_id = 4,
> 
> This is new in upstream so why's it starting at version 4?
> 

Good point. I was just using the Xen major, but that doesn't make much sense.

> > +    .pre_save = xen_hvm_pre_save,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_END_OF_LIST()
> > +    },
> 
> A vmstate which doesn't actually save any state? This seems
> rather suspicious...
> 

Not really. The state is actually in Xen and so is saved by the Xen toolstack. I need the pre-save hook here because the pages shared between QEMU and Xen need re-inserting into the guest before the Xen toolstack saves the memory image.

> > @@ -1060,12 +1185,19 @@ int xen_hvm_init(ram_addr_t
> *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
> >      xen_ram_init(below_4g_mem_size, above_4g_mem_size, ram_size,
> ram_memory);
> >
> >
> qemu_add_vm_change_state_handler(xen_hvm_change_state_handler,
> state);
> > +    vmstate_register(NULL, 0, &vmstate_xen_hvm, state);
> 
> Is the new use of vmstate_register() really necessary?
> Usually the state you're saving corresponds to some QOM
> device whose vmsd field you can use instead.
> 

I don't think so. As I said, there is no state to save but there is need for a callback before state is saved. Is there another way to achieve that? I could not find any 'clean' way to do it.

  Paul

> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v3 2/2] Xen: Use the ioreq-server API when available

[Stefano Stabellini]

On Wed, 15 Oct 2014, Peter Maydell wrote:
> On 15 October 2014 11:16, Paul Durrant <paul.durrant@citrix.com> wrote:
> > The ioreq-server API added to Xen 4.5 offers better security than
> > the existing Xen/QEMU interface because the shared pages that are
> > used to pass emulation request/results back and forth are removed
> > from the guest's memory space before any requests are serviced.
> > This prevents the guest from mapping these pages (they are in a
> > well known location) and attempting to attack QEMU by synthesizing
> > its own request structures. Hence, this patch modifies configure
> > to detect whether the API is available, and adds the necessary
> > code to use the API if it is.
> 
> This commit message doesn't mention it, but presumably this is
> all x86-specific given it's in a file which is only used for
> x86 Xen?

Unfortunately even though it is pretty x86 specific, it is still
compiled on ARM, even though it is never actually used (it is used in
i386 emulation with Xen acceleration support, while on ARM we only use
the PV machine).

Re: [Qemu-devel] [PATCH v3 2/2] Xen: Use the ioreq-server API when available

[Peter Maydell]

On 16 October 2014 13:29, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> Unfortunately even though it is pretty x86 specific, it is still
> compiled on ARM, even though it is never actually used (it is used in
> i386 emulation with Xen acceleration support, while on ARM we only use
> the PV machine).

Really? CONFIG_XEN_I386 is only set in the i386 and x86_64 defconfigs...

-- PMM

Re: [Qemu-devel] [PATCH v3 2/2] Xen: Use the ioreq-server API when available

[Stefano Stabellini]

On Thu, 16 Oct 2014, Peter Maydell wrote:
> On 16 October 2014 13:29, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > Unfortunately even though it is pretty x86 specific, it is still
> > compiled on ARM, even though it is never actually used (it is used in
> > i386 emulation with Xen acceleration support, while on ARM we only use
> > the PV machine).
> 
> Really? CONFIG_XEN_I386 is only set in the i386 and x86_64 defconfigs...

We are still using --target-list=i386-softmmu on ARM :-(
Of course we don't need i386-softmmu emulation on ARM, but we didn't
manage to disentangle the PV machine from i386 architecture emulation,
see:

http://marc.info/?l=qemu-devel&m=139082410318316&w=2
http://marc.info/?l=qemu-devel&m=140491268731773&w=2