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: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?

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