[Xen-devel] [PATCH v17 04/11] x86/hvm/ioreq: defer mapping gfns until they are actually requested

[Paul Durrant <paul.durrant@xxxxxxxxxx>]

A subsequent patch will introduce a new scheme to allow an emulator to
map ioreq server pages directly from Xen rather than the guest P2M.

This patch lays the groundwork for that change by deferring mapping of
gfns until their values are requested by an emulator. To that end, the
pad field of the xen_dm_op_get_ioreq_server_info structure is re-purposed
to a flags field and new flag, XEN_DMOP_no_gfns, defined which modifies the
behaviour of XEN_DMOP_get_ioreq_server_info to allow the caller to avoid
requesting the gfn values.

Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
---
Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Cc: Tim Deegan <tim@xxxxxxx>
Cc: Chao Gao <chao.gao@xxxxxxxxx>

v17:
 - Fix typo in commit comment

v16:
 - Leave call to map pages in hvm_ioreq_server_init() for default ioreq
   server instance, as pointed out by Chao (cc-ed). This is small and
   obvious change which reduces the size of the patch, so I have left
   existent R-bs and A-bs in place.

v8:
 - For safety make all of the pointers passed to
   hvm_get_ioreq_server_info() optional.
 - Shrink bufioreq_handling down to a uint8_t.

v3:
 - Updated in response to review comments from Wei and Roger.
 - Added a HANDLE_BUFIOREQ macro to make the code neater.
 - This patch no longer introduces a security vulnerability since there
   is now an explicit limit on the number of ioreq servers that may be
   created for any one domain.
---
 tools/libs/devicemodel/core.c                   |  8 ++++
 tools/libs/devicemodel/include/xendevicemodel.h |  6 +--
 xen/arch/x86/hvm/dm.c                           |  9 +++--
 xen/arch/x86/hvm/ioreq.c                        | 49 ++++++++++++++++---------
 xen/include/asm-x86/hvm/domain.h                |  2 +-
 xen/include/public/hvm/dm_op.h                  | 32 +++++++++-------
 6 files changed, 69 insertions(+), 37 deletions(-)

diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
index 355b7dec18..df2a8a0fe7 100644
--- a/tools/libs/devicemodel/core.c
+++ b/tools/libs/devicemodel/core.c
@@ -204,6 +204,14 @@ int xendevicemodel_get_ioreq_server_info(
 
     data->id = id;
 
+    /*
+     * If the caller is not requesting gfn values then instruct the
+     * hypercall not to retrieve them as this may cause them to be
+     * mapped.
+     */
+    if (!ioreq_gfn && !bufioreq_gfn)
+        data->flags |= XEN_DMOP_no_gfns;
+
     rc = xendevicemodel_op(dmod, domid, 1, &op, sizeof(op));
     if (rc)
         return rc;
diff --git a/tools/libs/devicemodel/include/xendevicemodel.h 
b/tools/libs/devicemodel/include/xendevicemodel.h
index dda0bc7695..fffee3a4a0 100644
--- a/tools/libs/devicemodel/include/xendevicemodel.h
+++ b/tools/libs/devicemodel/include/xendevicemodel.h
@@ -61,11 +61,11 @@ int xendevicemodel_create_ioreq_server(
  * @parm domid the domain id to be serviced
  * @parm id the IOREQ Server id.
  * @parm ioreq_gfn pointer to a xen_pfn_t to receive the synchronous ioreq
- *                  gfn
+ *                  gfn. (May be NULL if not required)
  * @parm bufioreq_gfn pointer to a xen_pfn_t to receive the buffered ioreq
- *                    gfn
+ *                    gfn. (May be NULL if not required)
  * @parm bufioreq_port pointer to a evtchn_port_t to receive the buffered
- *                     ioreq event channel
+ *                     ioreq event channel. (May be NULL if not required)
  * @return 0 on success, -1 on failure.
  */
 int xendevicemodel_get_ioreq_server_info(
diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index a787f43737..3c617bd754 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -416,16 +416,19 @@ static int dm_op(const struct dmop_args *op_args)
     {
         struct xen_dm_op_get_ioreq_server_info *data =
             &op.u.get_ioreq_server_info;
+        const uint16_t valid_flags = XEN_DMOP_no_gfns;
 
         const_op = false;
 
         rc = -EINVAL;
-        if ( data->pad )
+        if ( data->flags & ~valid_flags )
             break;
 
         rc = hvm_get_ioreq_server_info(d, data->id,
-                                       &data->ioreq_gfn,
-                                       &data->bufioreq_gfn,
+                                       (data->flags & XEN_DMOP_no_gfns) ?
+                                       NULL : &data->ioreq_gfn,
+                                       (data->flags & XEN_DMOP_no_gfns) ?
+                                       NULL : &data->bufioreq_gfn,
                                        &data->bufioreq_port);
         break;
     }
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index b2ef81f003..1e6f0f41e4 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -350,6 +350,9 @@ static void hvm_update_ioreq_evtchn(struct hvm_ioreq_server 
*s,
     }
 }
 
+#define HANDLE_BUFIOREQ(s) \
+    ((s)->bufioreq_handling != HVM_IOREQSRV_BUFIOREQ_OFF)
+
 static int hvm_ioreq_server_add_vcpu(struct hvm_ioreq_server *s,
                                      struct vcpu *v)
 {
@@ -371,7 +374,7 @@ static int hvm_ioreq_server_add_vcpu(struct 
hvm_ioreq_server *s,
 
     sv->ioreq_evtchn = rc;
 
-    if ( v->vcpu_id == 0 && s->bufioreq.va != NULL )
+    if ( v->vcpu_id == 0 && HANDLE_BUFIOREQ(s) )
     {
         struct domain *d = s->domain;
 
@@ -422,7 +425,7 @@ static void hvm_ioreq_server_remove_vcpu(struct 
hvm_ioreq_server *s,
 
         list_del(&sv->list_entry);
 
-        if ( v->vcpu_id == 0 && s->bufioreq.va != NULL )
+        if ( v->vcpu_id == 0 && HANDLE_BUFIOREQ(s) )
             free_xen_event_channel(v->domain, s->bufioreq_evtchn);
 
         free_xen_event_channel(v->domain, sv->ioreq_evtchn);
@@ -449,7 +452,7 @@ static void hvm_ioreq_server_remove_all_vcpus(struct 
hvm_ioreq_server *s)
 
         list_del(&sv->list_entry);
 
-        if ( v->vcpu_id == 0 && s->bufioreq.va != NULL )
+        if ( v->vcpu_id == 0 && HANDLE_BUFIOREQ(s) )
             free_xen_event_channel(v->domain, s->bufioreq_evtchn);
 
         free_xen_event_channel(v->domain, sv->ioreq_evtchn);
@@ -460,14 +463,13 @@ static void hvm_ioreq_server_remove_all_vcpus(struct 
hvm_ioreq_server *s)
     spin_unlock(&s->lock);
 }
 
-static int hvm_ioreq_server_map_pages(struct hvm_ioreq_server *s,
-                                      bool handle_bufioreq)
+static int hvm_ioreq_server_map_pages(struct hvm_ioreq_server *s)
 {
     int rc;
 
     rc = hvm_map_ioreq_gfn(s, false);
 
-    if ( !rc && handle_bufioreq )
+    if ( !rc && HANDLE_BUFIOREQ(s) )
         rc = hvm_map_ioreq_gfn(s, true);
 
     if ( rc )
@@ -597,13 +599,14 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server 
*s,
     if ( rc )
         return rc;
 
-    if ( bufioreq_handling == HVM_IOREQSRV_BUFIOREQ_ATOMIC )
-        s->bufioreq_atomic = true;
+    s->bufioreq_handling = bufioreq_handling;
 
-    rc = hvm_ioreq_server_map_pages(
-             s, bufioreq_handling != HVM_IOREQSRV_BUFIOREQ_OFF);
-    if ( rc )
-        goto fail_map;
+    if ( id == DEFAULT_IOSERVID )
+    {
+        rc = hvm_ioreq_server_map_pages(s);
+        if ( rc )
+            goto fail_map;
+    }
 
     for_each_vcpu ( d, v )
     {
@@ -757,12 +760,23 @@ int hvm_get_ioreq_server_info(struct domain *d, 
ioservid_t id,
 
     ASSERT(!IS_DEFAULT(s));
 
-    *ioreq_gfn = gfn_x(s->ioreq.gfn);
+    if ( ioreq_gfn || bufioreq_gfn )
+    {
+        rc = hvm_ioreq_server_map_pages(s);
+        if ( rc )
+            goto out;
+    }
 
-    if ( s->bufioreq.va != NULL )
+    if ( ioreq_gfn )
+        *ioreq_gfn = gfn_x(s->ioreq.gfn);
+
+    if ( HANDLE_BUFIOREQ(s) )
     {
-        *bufioreq_gfn = gfn_x(s->bufioreq.gfn);
-        *bufioreq_port = s->bufioreq_evtchn;
+        if ( bufioreq_gfn )
+            *bufioreq_gfn = gfn_x(s->bufioreq.gfn);
+
+        if ( bufioreq_port )
+            *bufioreq_port = s->bufioreq_evtchn;
     }
 
     rc = 0;
@@ -1268,7 +1282,8 @@ static int hvm_send_buffered_ioreq(struct 
hvm_ioreq_server *s, ioreq_t *p)
     pg->ptrs.write_pointer += qw ? 2 : 1;
 
     /* Canonicalize read/write pointers to prevent their overflow. */
-    while ( s->bufioreq_atomic && qw++ < IOREQ_BUFFER_SLOT_NUM &&
+    while ( (s->bufioreq_handling == HVM_IOREQSRV_BUFIOREQ_ATOMIC) &&
+            qw++ < IOREQ_BUFFER_SLOT_NUM &&
             pg->ptrs.read_pointer >= IOREQ_BUFFER_SLOT_NUM )
     {
         union bufioreq_pointers old = pg->ptrs, new;
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 3bd9c5d7c0..8b798ee4e9 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -69,7 +69,7 @@ struct hvm_ioreq_server {
     evtchn_port_t          bufioreq_evtchn;
     struct rangeset        *range[NR_IO_RANGE_TYPES];
     bool                   enabled;
-    bool                   bufioreq_atomic;
+    uint8_t                bufioreq_handling;
 };
 
 /*
diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
index e17308575a..13b3737c2f 100644
--- a/xen/include/public/hvm/dm_op.h
+++ b/xen/include/public/hvm/dm_op.h
@@ -79,28 +79,34 @@ struct xen_dm_op_create_ioreq_server {
  * XEN_DMOP_get_ioreq_server_info: Get all the information necessary to
  *                                 access IOREQ Server <id>.
  *
- * The emulator needs to map the synchronous ioreq structures and buffered
- * ioreq ring (if it exists) that Xen uses to request emulation. These are
- * hosted in the target domain's gmfns <ioreq_gfn> and <bufioreq_gfn>
- * respectively. In addition, if the IOREQ Server is handling buffered
- * emulation requests, the emulator needs to bind to event channel
- * <bufioreq_port> to listen for them. (The event channels used for
- * synchronous emulation requests are specified in the per-CPU ioreq
- * structures in <ioreq_gfn>).
- * If the IOREQ Server is not handling buffered emulation requests then the
- * values handed back in <bufioreq_gfn> and <bufioreq_port> will both be 0.
+ * If the IOREQ Server is handling buffered emulation requests, the
+ * emulator needs to bind to event channel <bufioreq_port> to listen for
+ * them. (The event channels used for synchronous emulation requests are
+ * specified in the per-CPU ioreq structures).
+ * In addition, if the XENMEM_acquire_resource memory op cannot be used,
+ * the emulator will need to map the synchronous ioreq structures and
+ * buffered ioreq ring (if it exists) from guest memory. If <flags> does
+ * not contain XEN_DMOP_no_gfns then these pages will be made available and
+ * the frame numbers passed back in gfns <ioreq_gfn> and <bufioreq_gfn>
+ * respectively. (If the IOREQ Server is not handling buffered emulation
+ * only <ioreq_gfn> will be valid).
  */
 #define XEN_DMOP_get_ioreq_server_info 2
 
 struct xen_dm_op_get_ioreq_server_info {
     /* IN - server id */
     ioservid_t id;
-    uint16_t pad;
+    /* IN - flags */
+    uint16_t flags;
+
+#define _XEN_DMOP_no_gfns 0
+#define XEN_DMOP_no_gfns (1u << _XEN_DMOP_no_gfns)
+
     /* OUT - buffered ioreq port */
     evtchn_port_t bufioreq_port;
-    /* OUT - sync ioreq gfn */
+    /* OUT - sync ioreq gfn (see block comment above) */
     uint64_aligned_t ioreq_gfn;
-    /* OUT - buffered ioreq gfn */
+    /* OUT - buffered ioreq gfn (see block comment above)*/
     uint64_aligned_t bufioreq_gfn;
 };
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel