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

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

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: 07 September 2017 15:23
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>; xen-devel@lists.xenproject.org; Ian
> Jackson <Ian.Jackson@citrix.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>; Konrad
> Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [PATCH v4 11/12] x86/hvm/ioreq: defer mapping gfns until they
> are actually requsted
> 
> On Thu, Sep 07, 2017 at 01:29:12PM +0100, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Wei Liu [mailto:wei.liu2@citrix.com]
> > > Sent: 07 September 2017 13:16
> > > To: Paul Durrant <Paul.Durrant@citrix.com>
> > > Cc: Wei Liu <wei.liu2@citrix.com>; xen-devel@lists.xenproject.org; Ian
> > > Jackson <Ian.Jackson@citrix.com>; Andrew Cooper
> > > <Andrew.Cooper3@citrix.com>; George Dunlap
> > > <George.Dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>; Konrad
> > > Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini
> > > <sstabellini@kernel.org>; Tim (Xen.org) <tim@xen.org>
> > > Subject: Re: [PATCH v4 11/12] x86/hvm/ioreq: defer mapping gfns until
> they
> > > are actually requsted
> > >
> > > On Thu, Sep 07, 2017 at 01:03:46PM +0100, Paul Durrant wrote:
> > > > > -----Original Message-----
> > > > > From: Wei Liu [mailto:wei.liu2@citrix.com]
> > > > > Sent: 07 September 2017 13:00
> > > > > To: Paul Durrant <Paul.Durrant@citrix.com>
> > > > > Cc: xen-devel@lists.xenproject.org; Ian Jackson
> > > <Ian.Jackson@citrix.com>;
> > > > > Wei Liu <wei.liu2@citrix.com>; Andrew Cooper
> > > > > <Andrew.Cooper3@citrix.com>; George Dunlap
> > > > > <George.Dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>;
> Konrad
> > > > > Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini
> > > > > <sstabellini@kernel.org>; Tim (Xen.org) <tim@xen.org>
> > > > > Subject: Re: [PATCH v4 11/12] x86/hvm/ioreq: defer mapping gfns
> until
> > > they
> > > > > are actually requsted
> > > > >
> > > > > On Tue, Sep 05, 2017 at 12:37:15PM +0100, 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>
> > > > > > ---
> > > > > > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > > > > > Cc: Wei Liu <wei.liu2@citrix.com>
> > > > > > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > > > > > Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> > > > > > Cc: Jan Beulich <jbeulich@suse.com>
> > > > > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > > > > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > > > > > Cc: Tim Deegan <tim@xen.org>
> > > > > >
> > > > > > 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                        | 41 +++++++++++++---------
> ---
> > > > > >  xen/include/asm-x86/hvm/domain.h                |  2 +-
> > > > > >  xen/include/public/hvm/dm_op.h                  | 32 +++++++++++------
> --
> > > > > >  6 files changed, 59 insertions(+), 39 deletions(-)
> > > > > >
> > > > > > diff --git a/tools/libs/devicemodel/core.c
> > > b/tools/libs/devicemodel/core.c
> > > > > > index fcb260d29b..28958934bf 100644
> > > > > > --- a/tools/libs/devicemodel/core.c
> > > > > > +++ b/tools/libs/devicemodel/core.c
> > > > > > @@ -188,6 +188,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;
> > > > > > +
> > > > >
> > > > > Sorry for not having noticed this earlier.
> > > > >
> > > > > This is a slight change to a stable API. The new functionality is an
> > > > > extension of the old. I would suggest you bump the minor number of
> this
> > > > > library as well.
> > > > >
> > > >
> > > > I don't believe there is an API change here. The code always coped with
> > > NULL being passed, it just wasn't documented. Or is there something else
> I'm
> > > missing?
> > > >
> > >
> > > There is.  The original code copes with NULL as in "I doesn't care,
> > > hypervisor will deal with it"; the new code actually gives NULL another
> > > meaning.
> > >
> > > Suppose an application that is compiled for this version,
> > > which discovered that passing NULL has behaviour A and then, when it
> > > runs on a previous version of this library (it would happily do so
> > > because MAJOR.MINOR has not changed) and gets behaviour B.
> > >
> > > Does that make sense?
> >
> > I don't understand what the discernible change in behaviour is as far as the
> application goes. What is the semantic difference? Sure the underlying
> hypercall has changed its semantic slightly but how is this visible in any way to
> the application?
> 
> I'm a bit dense today.
> 
> I think, after reading this series and the existing code again, the "may
> cause them to be mapped" is not a description of prior behaviour. It
> applies to the code in this patch.
> 
> So this patch doesn't change the behaviour of the API, hence no bumping
> version number is needed.

Ok. Thanks for confirming I wasn't missing anything subtle :-)

  Paul


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