[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] RE: [PATCH v2 02/11] xen/gnttab: Rework resource acquisition
> -----Original Message----- > From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > Sent: 22 September 2020 19:25 > To: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George Dunlap > <George.Dunlap@xxxxxxxxxxxxx>; Ian > Jackson <iwj@xxxxxxxxxxxxxx>; Jan Beulich <JBeulich@xxxxxxxx>; Stefano > Stabellini > <sstabellini@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Julien Grall > <julien@xxxxxxx>; Paul Durrant > <paul@xxxxxxx>; Michał Leszczyński <michal.leszczynski@xxxxxxx>; Hubert > Jasudowicz > <hubert.jasudowicz@xxxxxxx>; Tamas K Lengyel <tamas@xxxxxxxxxxxxx> > Subject: [PATCH v2 02/11] xen/gnttab: Rework resource acquisition > > The existing logic doesn't function in the general case for mapping a guests > grant table, due to arbitrary 32 frame limit, and the default grant table > limit being 64. > > In order to start addressing this, rework the existing grant table logic by > implementing a single gnttab_acquire_resource(). This is far more efficient > than the previous acquire_grant_table() in memory.c because it doesn't take > the grant table write lock, and attempt to grow the table, for every single > frame. > > The new gnttab_acquire_resource() function subsumes the previous two > gnttab_get_{shared,status}_frame() helpers. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > --- > CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx> > CC: Ian Jackson <iwj@xxxxxxxxxxxxxx> > CC: Jan Beulich <JBeulich@xxxxxxxx> > CC: Stefano Stabellini <sstabellini@xxxxxxxxxx> > CC: Wei Liu <wl@xxxxxxx> > CC: Julien Grall <julien@xxxxxxx> > CC: Paul Durrant <paul@xxxxxxx> > CC: Michał Leszczyński <michal.leszczynski@xxxxxxx> > CC: Hubert Jasudowicz <hubert.jasudowicz@xxxxxxx> > CC: Tamas K Lengyel <tamas@xxxxxxxxxxxxx> > > v2: > * Fix CentOS 6 build by initialising vaddrs to NULL > * Add ASSERT_UNREACHABLE() and gt->status typecheck > --- > xen/common/grant_table.c | 102 > +++++++++++++++++++++++++++++++----------- > xen/common/memory.c | 42 +---------------- > xen/include/xen/grant_table.h | 19 +++----- > 3 files changed, 84 insertions(+), 79 deletions(-) > > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c > index a5d3ed8bda..912f07be47 100644 > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -4013,6 +4013,81 @@ static int gnttab_get_shared_frame_mfn(struct domain > *d, > return 0; > } > > +int gnttab_acquire_resource( > + struct domain *d, unsigned int id, unsigned long frame, > + unsigned int nr_frames, xen_pfn_t mfn_list[]) > +{ > + struct grant_table *gt = d->grant_table; > + unsigned int i = nr_frames, tot_frames; > + mfn_t tmp; > + void **vaddrs = NULL; > + int rc; > + > + /* Input sanity. */ Nit: inconsistency with full stops on single line comments. > + if ( !nr_frames ) > + return -EINVAL; > + > + /* Overflow checks */ > + if ( frame + nr_frames < frame ) > + return -EINVAL; > + > + tot_frames = frame + nr_frames; > + if ( tot_frames != frame + nr_frames ) > + return -EINVAL; > + > + /* Grow table if necessary. */ > + grant_write_lock(gt); > + switch ( id ) > + { > + case XENMEM_resource_grant_table_id_shared: > + rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp); > + break; > + > + case XENMEM_resource_grant_table_id_status: > + if ( gt->gt_version != 2 ) > + { > + default: Personally I dislike this kind of thing. IMO it would be neater to initialise rc to -EINVAL at the top, then this could just be a 'break' (so no braces) and you could have a simple 'default: break;' case instead. > + rc = -EINVAL; > + break; > + } > + rc = gnttab_get_status_frame_mfn(d, tot_frames - 1, &tmp); > + break; > + } > + I think you could drop the write lock here... > + /* Any errors from growing the table? */ > + if ( rc ) > + goto out; > + ...and acquire it read here, since we know the table cannot shrink. You'd need to re-check the gt_version for safety though. Paul > + switch ( id ) > + { > + case XENMEM_resource_grant_table_id_shared: > + vaddrs = gt->shared_raw; > + break; > + > + case XENMEM_resource_grant_table_id_status: > + /* Check that void ** is a suitable representation for gt->status. */ > + BUILD_BUG_ON(!__builtin_types_compatible_p( > + typeof(gt->status), grant_status_t **)); > + vaddrs = (void **)gt->status; > + break; > + } > + > + if ( !vaddrs ) > + { > + ASSERT_UNREACHABLE(); > + rc = -EINVAL; > + goto out; > + } > + > + for ( i = 0; i < nr_frames; ++i ) > + mfn_list[i] = virt_to_mfn(vaddrs[frame + i]); > + > + out: > + grant_write_unlock(gt); > + > + return rc; > +} > + > int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t > *mfn) > { > int rc = 0; > @@ -4047,33 +4122,6 @@ int gnttab_map_frame(struct domain *d, unsigned long > idx, gfn_t gfn, mfn_t > *mfn) > return rc; > } > > -int gnttab_get_shared_frame(struct domain *d, unsigned long idx, > - mfn_t *mfn) > -{ > - struct grant_table *gt = d->grant_table; > - int rc; > - > - grant_write_lock(gt); > - rc = gnttab_get_shared_frame_mfn(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; > - > - grant_write_lock(gt); > - rc = (gt->gt_version == 2) ? > - gnttab_get_status_frame_mfn(d, idx, mfn) : -EINVAL; > - 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 1bab0e80c2..177fc378d9 100644 > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -1007,44 +1007,6 @@ 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, > - xen_pfn_t mfn_list[]) > -{ > - unsigned int i = nr_frames; > - > - /* Iterate backwards in case table needs to grow */ > - while ( i-- != 0 ) > - { > - mfn_t mfn = INVALID_MFN; > - int rc; > - > - switch ( id ) > - { > - case XENMEM_resource_grant_table_id_shared: > - rc = gnttab_get_shared_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; > - > - ASSERT(!mfn_eq(mfn, INVALID_MFN)); > - mfn_list[i] = mfn_x(mfn); > - } > - > - return 0; > -} > - > static int acquire_resource( > XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg) > { > @@ -1099,8 +1061,8 @@ static int acquire_resource( > switch ( xmar.type ) > { > case XENMEM_resource_grant_table: > - rc = acquire_grant_table(d, xmar.id, xmar.frame, xmar.nr_frames, > - mfn_list); > + rc = gnttab_acquire_resource(d, xmar.id, xmar.frame, xmar.nr_frames, > + mfn_list); > break; > > default: > diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h > index 98603604b8..5a2c75b880 100644 > --- a/xen/include/xen/grant_table.h > +++ b/xen/include/xen/grant_table.h > @@ -56,10 +56,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_shared_frame(struct domain *d, unsigned long idx, > - mfn_t *mfn); > -int gnttab_get_status_frame(struct domain *d, unsigned long idx, > - mfn_t *mfn); > + > +int gnttab_acquire_resource( > + struct domain *d, unsigned int id, unsigned long frame, > + unsigned int nr_frames, xen_pfn_t mfn_list[]); > > #else > > @@ -93,14 +93,9 @@ static inline int gnttab_map_frame(struct domain *d, > unsigned long idx, > return -EINVAL; > } > > -static inline int gnttab_get_shared_frame(struct domain *d, unsigned long > idx, > - mfn_t *mfn) > -{ > - return -EINVAL; > -} > - > -static inline int gnttab_get_status_frame(struct domain *d, unsigned long > idx, > - mfn_t *mfn) > +static inline int gnttab_acquire_resource( > + struct domain *d, unsigned int id, unsigned long frame, > + unsigned int nr_frames, xen_pfn_t mfn_list[]) > { > return -EINVAL; > } > -- > 2.11.0
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |