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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Sep 07, 2017 at 01:29:12PM +0100, Paul Durrant wrote:
> > -----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?

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.

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