| Message ID | 20171012162603.3016-11-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> Paul Durrant <paul.durrant@citrix.com> 10/12/17 6:28 PM >>> >@@ -1608,7 +1608,8 @@ fault: >} > >static int >-gnttab_populate_status_frames(struct domain *d, struct grant_table *gt, >+gnttab_populate_status_frames(struct domain *d, >+ struct grant_table *gt, >unsigned int req_nr_frames) What is this change about? >+int gnttab_get_grant_frame(struct domain *d, unsigned long idx, >+ mfn_t *mfn) >+{ >+ struct grant_table *gt = d->grant_table; >+ int rc; >+ >+ /* write lock required as version may change and/or table may grow */ >+ grant_write_lock(gt); >+ >+ rc = (gt->gt_version == 2 && >+ idx > XENMAPIDX_grant_table_status) ? I don't understand this check - why does XENMAPIDX_grant_table_status matter here at all? Same in gnttab_get_status_frame() then. Jan
> -----Original Message----- > From: Jan Beulich [mailto:jbeulich@suse.com] > Sent: 17 October 2017 07:43 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu > <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian > Jackson <Ian.Jackson@citrix.com>; sstabellini@kernel.org; xen- > devel@lists.xenproject.org; konrad.wilk@oracle.com; Tim (Xen.org) > <tim@xen.org> > Subject: Re: [PATCH v11 10/11] common: add a new mappable resource > type: XENMEM_resource_grant_table > > >>> Paul Durrant <paul.durrant@citrix.com> 10/12/17 6:28 PM >>> > >@@ -1608,7 +1608,8 @@ fault: > >} > > > >static int > >-gnttab_populate_status_frames(struct domain *d, struct grant_table *gt, > >+gnttab_populate_status_frames(struct domain *d, > >+ struct grant_table *gt, > >unsigned int req_nr_frames) > > What is this change about? > It must have crept in accidentally. I'll get rid of it. > >+int gnttab_get_grant_frame(struct domain *d, unsigned long idx, > >+ mfn_t *mfn) > >+{ > >+ struct grant_table *gt = d->grant_table; > >+ int rc; > >+ > >+ /* write lock required as version may change and/or table may grow */ > >+ grant_write_lock(gt); > >+ > >+ rc = (gt->gt_version == 2 && > >+ idx > XENMAPIDX_grant_table_status) ? > > I don't understand this check - why does XENMAPIDX_grant_table_status > matter here at all? Same in gnttab_get_status_frame() then. > Well, the current legal range of grant table frames for v2 is 0 - (1 << XENMAPIDX_grant_table_status) whereas it appears that for v1 there is no limit. As for status frames, they are a v2-only concept but I agree that the range check there is wrong. Paul > Jan
>>> On 17.10.17 at 10:30, <Paul.Durrant@citrix.com> wrote: >> From: Jan Beulich [mailto:jbeulich@suse.com] >> Sent: 17 October 2017 07:43 >> >>> Paul Durrant <paul.durrant@citrix.com> 10/12/17 6:28 PM >>> >> >+int gnttab_get_grant_frame(struct domain *d, unsigned long idx, >> >+ mfn_t *mfn) >> >+{ >> >+ struct grant_table *gt = d->grant_table; >> >+ int rc; >> >+ >> >+ /* write lock required as version may change and/or table may grow */ >> >+ grant_write_lock(gt); >> >+ >> >+ rc = (gt->gt_version == 2 && >> >+ idx > XENMAPIDX_grant_table_status) ? >> >> I don't understand this check - why does XENMAPIDX_grant_table_status >> matter here at all? Same in gnttab_get_status_frame() then. >> > > Well, the current legal range of grant table frames for v2 is 0 - (1 << > XENMAPIDX_grant_table_status) whereas it appears that for v1 there is no > limit. As for status frames, they are a v2-only concept but I agree that the > range check there is wrong. I don't think the range limitation from the other interface should impose any restriction for this new one. Oh, one other thing I only notice now - could you please also attach a brief comment to the array that you grow to 32 entries making clear that this is a pretty arbitrary choice? Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 17 October 2017 10:06 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap > <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu > <wei.liu2@citrix.com>; sstabellini@kernel.org; xen- > devel@lists.xenproject.org; konrad.wilk@oracle.com; Tim (Xen.org) > <tim@xen.org> > Subject: RE: [PATCH v11 10/11] common: add a new mappable resource type: > XENMEM_resource_grant_table > > >>> On 17.10.17 at 10:30, <Paul.Durrant@citrix.com> wrote: > >> From: Jan Beulich [mailto:jbeulich@suse.com] > >> Sent: 17 October 2017 07:43 > >> >>> Paul Durrant <paul.durrant@citrix.com> 10/12/17 6:28 PM >>> > >> >+int gnttab_get_grant_frame(struct domain *d, unsigned long idx, > >> >+ mfn_t *mfn) > >> >+{ > >> >+ struct grant_table *gt = d->grant_table; > >> >+ int rc; > >> >+ > >> >+ /* write lock required as version may change and/or table may grow > */ > >> >+ grant_write_lock(gt); > >> >+ > >> >+ rc = (gt->gt_version == 2 && > >> >+ idx > XENMAPIDX_grant_table_status) ? > >> > >> I don't understand this check - why does XENMAPIDX_grant_table_status > >> matter here at all? Same in gnttab_get_status_frame() then. > >> > > > > Well, the current legal range of grant table frames for v2 is 0 - (1 << > > XENMAPIDX_grant_table_status) whereas it appears that for v1 there is no > > limit. As for status frames, they are a v2-only concept but I agree that the > > range check there is wrong. > > I don't think the range limitation from the other interface should > impose any restriction for this new one. Ok. I'll drop the check. > > Oh, one other thing I only notice now - could you please also > attach a brief comment to the array that you grow to 32 > entries making clear that this is a pretty arbitrary choice? > Sure. Paul > Jan
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index 6d20b17739..e42c1b6bf3 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -1608,7 +1608,8 @@ fault: } static int -gnttab_populate_status_frames(struct domain *d, struct grant_table *gt, +gnttab_populate_status_frames(struct domain *d, + struct grant_table *gt, unsigned int req_nr_frames) { unsigned i; @@ -3756,13 +3757,12 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref, } #endif -int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, - mfn_t *mfn) +/* Caller must hold write lock as version may change and table may grow */ +static int gnttab_get_frame(struct domain *d, unsigned long idx, + mfn_t *mfn) { - int rc = 0; struct grant_table *gt = d->grant_table; - - grant_write_lock(gt); + int rc = 0; if ( gt->gt_version == 0 ) gt->gt_version = 1; @@ -3787,6 +3787,19 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, rc = -EINVAL; } + return rc; +} + +int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, + mfn_t *mfn) +{ + struct grant_table *gt = d->grant_table; + int rc; + + grant_write_lock(gt); + + rc = gnttab_get_frame(d, idx, mfn); + if ( !rc ) gnttab_set_frame_gfn(gt, idx, gfn); @@ -3795,6 +3808,44 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, return rc; } +int gnttab_get_grant_frame(struct domain *d, unsigned long idx, + mfn_t *mfn) +{ + struct grant_table *gt = d->grant_table; + int rc; + + /* write lock required as version may change and/or table may grow */ + grant_write_lock(gt); + + rc = (gt->gt_version == 2 && + idx > XENMAPIDX_grant_table_status) ? + -EINVAL : + gnttab_get_frame(d, idx, mfn); + + grant_write_unlock(gt); + + return rc; +} + +int gnttab_get_status_frame(struct domain *d, unsigned long idx, + mfn_t *mfn) +{ + struct grant_table *gt = d->grant_table; + int rc; + + /* write lock required as version may change and/or table may grow */ + grant_write_lock(gt); + + rc = (gt->gt_version != 2 || + idx > XENMAPIDX_grant_table_status) ? + -EINVAL : + gnttab_get_frame(d, idx & XENMAPIDX_grant_table_status, mfn); + + grant_write_unlock(gt); + + return rc; +} + static void gnttab_usage_print(struct domain *rd) { int first = 1; diff --git a/xen/common/memory.c b/xen/common/memory.c index 1a9872b75c..a50d93d006 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -23,6 +23,7 @@ #include <xen/numa.h> #include <xen/mem_access.h> #include <xen/trace.h> +#include <xen/grant_table.h> #include <asm/current.h> #include <asm/hardirq.h> #include <asm/p2m.h> @@ -965,11 +966,47 @@ static long xatp_permission_check(struct domain *d, unsigned int space) return xsm_add_to_physmap(XSM_TARGET, current->domain, d); } +static int acquire_grant_table(struct domain *d, unsigned int id, + unsigned long frame, + unsigned int nr_frames, + unsigned long mfn_list[]) +{ + unsigned int i = nr_frames; + + while ( i-- != 0 ) + { + mfn_t mfn = INVALID_MFN; + int rc; + + switch ( id ) + { + case XENMEM_resource_grant_table_id_grant: + rc = gnttab_get_grant_frame(d, frame + i, &mfn); + break; + + case XENMEM_resource_grant_table_id_status: + rc = gnttab_get_status_frame(d, frame + i, &mfn); + break; + + default: + rc = -EINVAL; + break; + } + + if ( rc ) + return rc; + + mfn_list[i] = mfn_x(mfn); + } + + return 0; +} + static int acquire_resource(XEN_GUEST_HANDLE_PARAM(void) arg) { struct domain *d, *currd = current->domain; xen_mem_acquire_resource_t xmar; - unsigned long mfn_list[2]; + unsigned long mfn_list[32]; int rc; if ( copy_from_guest(&xmar, arg, 1) ) @@ -1005,6 +1042,11 @@ static int acquire_resource(XEN_GUEST_HANDLE_PARAM(void) arg) xmar.nr_frames, mfn_list); break; + case XENMEM_resource_grant_table: + rc = acquire_grant_table(d, xmar.id, xmar.frame, xmar.nr_frames, + mfn_list); + break; + default: rc = -EOPNOTSUPP; break; diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h index 53380287d4..79ccaecc27 100644 --- a/xen/include/public/memory.h +++ b/xen/include/public/memory.h @@ -611,14 +611,20 @@ struct xen_mem_acquire_resource { uint16_t type; #define XENMEM_resource_ioreq_server 0 +#define XENMEM_resource_grant_table 1 /* * IN - a type-specific resource identifier, which must be zero * unless stated otherwise. * * type == XENMEM_resource_ioreq_server -> id == ioreq server id + * type == XENMEM_resource_grant_table -> id defined below */ uint32_t id; + +#define XENMEM_resource_grant_table_id_grant 0 +#define XENMEM_resource_grant_table_id_status 1 + /* IN/OUT - As an IN parameter number of (4K) frames of the resource * to be mapped. However, if the specified value is 0 then * -EINVAL will be returned and this field will be set to the diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h index b3a95fda58..e9125e43e7 100644 --- a/xen/include/xen/grant_table.h +++ b/xen/include/xen/grant_table.h @@ -55,6 +55,10 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref, int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t *mfn); +int gnttab_get_grant_frame(struct domain *d, unsigned long idx, + mfn_t *mfn); +int gnttab_get_status_frame(struct domain *d, unsigned long idx, + mfn_t *mfn); unsigned int gnttab_dom0_frames(void);
This patch allows grant table frames to be mapped using the XENMEM_acquire_resource memory op. Signed-off-by: Paul Durrant <paul.durrant@citrix.com> --- Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@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> Cc: Wei Liu <wei.liu2@citrix.com> v10: - Addressed comments from Jan. v8: - The functionality was originally incorporated into the earlier patch "x86/mm: add HYPERVISOR_memory_op to acquire guest resources". --- xen/common/grant_table.c | 63 ++++++++++++++++++++++++++++++++++++++----- xen/common/memory.c | 44 +++++++++++++++++++++++++++++- xen/include/public/memory.h | 6 +++++ xen/include/xen/grant_table.h | 4 +++ 4 files changed, 110 insertions(+), 7 deletions(-)