Re: [PATCH v14 04/11] x86/hvm/ioreq: defer mapping gfns until they are actually requsted

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Chao Gao [mailto:chao.gao@intel.com]
> Sent: 06 December 2017 21:50
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini
> <sstabellini@kernel.org>; George Dunlap <George.Dunlap@citrix.com>;
> Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [Xen-devel] [PATCH v14 04/11] x86/hvm/ioreq: defer mapping
> gfns until they are actually requsted
> 
> On Tue, Nov 28, 2017 at 03:08:46PM +0000, Paul Durrant wrote:
> >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@citrix.com>
> >Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> >Acked-by: Wei Liu <wei.liu2@citrix.com>
> >Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >---
> >Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> >Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> >Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >Cc: Stefano Stabellini <sstabellini@kernel.org>
> >Cc: Tim Deegan <tim@xen.org>
> >
> >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                        | 47 ++++++++++++++-----------
> > xen/include/asm-x86/hvm/domain.h                |  2 +-
> > xen/include/public/hvm/dm_op.h                  | 32 ++++++++++-------
> > 6 files changed, 63 insertions(+), 41 deletions(-)
> >
> >diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
> >index b66d4f9294..e684e657b6 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 eec4e4771e..39de659ddf 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,7 @@ 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;
> >-
> >-    rc = hvm_ioreq_server_map_pages(
> >-             s, bufioreq_handling != HVM_IOREQSRV_BUFIOREQ_OFF);
> 
> It seems that for default IO server, mapping gfns here is required. Old
> qemu won't call hvm_get_ioreq_server_info() (and cannot because of the
> assertion 'ASSERT(!IS_DEFAULT(s))') to set up the mapping.

Yes. Old qemu has no knowledge of the ioreq server API so the 'default' ioreq server is there to accommodate it. Again this is all legacy code so no need to extend it.

  Paul

> 
> Thanks
> Chao

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