> -----Original Message----- > From: Wei Liu [mailto:wei.l...@citrix.com] > Sent: 07 September 2017 15:23 > To: Paul Durrant <paul.durr...@citrix.com> > Cc: Wei Liu <wei.l...@citrix.com>; xen-de...@lists.xenproject.org; Ian > Jackson <ian.jack...@citrix.com>; Andrew Cooper > <andrew.coop...@citrix.com>; George Dunlap > <george.dun...@citrix.com>; Jan Beulich <jbeul...@suse.com>; Konrad > Rzeszutek Wilk <konrad.w...@oracle.com>; Stefano Stabellini > <sstabell...@kernel.org>; Tim (Xen.org) <t...@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.l...@citrix.com] > > > Sent: 07 September 2017 13:16 > > > To: Paul Durrant <paul.durr...@citrix.com> > > > Cc: Wei Liu <wei.l...@citrix.com>; xen-de...@lists.xenproject.org; Ian > > > Jackson <ian.jack...@citrix.com>; Andrew Cooper > > > <andrew.coop...@citrix.com>; George Dunlap > > > <george.dun...@citrix.com>; Jan Beulich <jbeul...@suse.com>; Konrad > > > Rzeszutek Wilk <konrad.w...@oracle.com>; Stefano Stabellini > > > <sstabell...@kernel.org>; Tim (Xen.org) <t...@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.l...@citrix.com] > > > > > Sent: 07 September 2017 13:00 > > > > > To: Paul Durrant <paul.durr...@citrix.com> > > > > > Cc: xen-de...@lists.xenproject.org; Ian Jackson > > > <ian.jack...@citrix.com>; > > > > > Wei Liu <wei.l...@citrix.com>; Andrew Cooper > > > > > <andrew.coop...@citrix.com>; George Dunlap > > > > > <george.dun...@citrix.com>; Jan Beulich <jbeul...@suse.com>; > Konrad > > > > > Rzeszutek Wilk <konrad.w...@oracle.com>; Stefano Stabellini > > > > > <sstabell...@kernel.org>; Tim (Xen.org) <t...@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.durr...@citrix.com> > > > > > > Reviewed-by: Roger Pau Monné <roger....@citrix.com> > > > > > > --- > > > > > > Cc: Ian Jackson <ian.jack...@eu.citrix.com> > > > > > > Cc: Wei Liu <wei.l...@citrix.com> > > > > > > Cc: Andrew Cooper <andrew.coop...@citrix.com> > > > > > > Cc: George Dunlap <george.dun...@eu.citrix.com> > > > > > > Cc: Jan Beulich <jbeul...@suse.com> > > > > > > Cc: Konrad Rzeszutek Wilk <konrad.w...@oracle.com> > > > > > > Cc: Stefano Stabellini <sstabell...@kernel.org> > > > > > > Cc: Tim Deegan <t...@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
