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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@xxxxxxxxxx]
> Sent: 07 September 2017 13:16
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx; Ian
> Jackson <Ian.Jackson@xxxxxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Konrad
> Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>
> 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@xxxxxxxxxx]
> > > Sent: 07 September 2017 13:00
> > > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> > > Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Ian Jackson
> <Ian.Jackson@xxxxxxxxxx>;
> > > Wei Liu <wei.liu2@xxxxxxxxxx>; Andrew Cooper
> > > <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> > > <George.Dunlap@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Konrad
> > > Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini
> > > <sstabellini@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>
> > > 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@xxxxxxxxxx>
> > > > Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > > > ---
> > > > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > > > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > > > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > > > Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> > > > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > > > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > > Cc: Tim Deegan <tim@xxxxxxx>
> > > >
> > > > 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?

  Paul

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