| Message ID | 20200922182444.12350-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Multiple fixes to XENMEM_acquire_resource | expand |
> -----Original Message----- > From: Andrew Cooper <andrew.cooper3@citrix.com> > Sent: 22 September 2020 19:25 > To: Xen-devel <xen-devel@lists.xenproject.org> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <George.Dunlap@eu.citrix.com>; Ian > Jackson <iwj@xenproject.org>; Jan Beulich <JBeulich@suse.com>; Stefano Stabellini > <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Julien Grall <julien@xen.org>; Paul Durrant > <paul@xen.org>; Michał Leszczyński <michal.leszczynski@cert.pl>; Hubert Jasudowicz > <hubert.jasudowicz@cert.pl>; Tamas K Lengyel <tamas@tklengyel.com> > 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@citrix.com> > --- > CC: George Dunlap <George.Dunlap@eu.citrix.com> > CC: Ian Jackson <iwj@xenproject.org> > CC: Jan Beulich <JBeulich@suse.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Wei Liu <wl@xen.org> > CC: Julien Grall <julien@xen.org> > CC: Paul Durrant <paul@xen.org> > CC: Michał Leszczyński <michal.leszczynski@cert.pl> > CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl> > CC: Tamas K Lengyel <tamas@tklengyel.com> > > 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
On 22.09.2020 20:24, Andrew Cooper wrote: > --- 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. */ > + if ( !nr_frames ) > + return -EINVAL; I continue to object to this becoming an error. It wasn't before, and it wouldn't be in line with various other operations, not the least XENMEM_resource_ioreq_server handling, but also various of the other mem-op functions (just to give concrete examples) or basically all of the grant-table ones. And if it really was to be an error, it could be had by folding into ... > + /* Overflow checks */ > + if ( frame + nr_frames < frame ) > + return -EINVAL; this: 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: > + rc = -EINVAL; > + break; > + } > + rc = gnttab_get_status_frame_mfn(d, tot_frames - 1, &tmp); > + break; > + } > + > + /* Any errors from growing the table? */ > + if ( rc ) > + goto out; > + > + 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; > + } Why the 2nd switch()? As long as you don't de-reference vaddrs prior to checking rc, I don't see anything that could go wrong. And once both are folded, maybe vaddr's initializer could also be dropped again? Jan
On 24/09/2020 10:51, Paul Durrant wrote: >> 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. The whole point of relaxing the style was because feedback over minutia such as this was deemed detrimental to the community. If I ever see feedback like this, I will commit commit the patch there and then. This is the only way upstream Xen is going to turn into a less toxic environment for contributors. >> + 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. And you've correctly identified why I didn't. If we had a relax-write-to-read lock operation, that would also be fine, but we don't. Fundamentally, this is an operation made once during VM construction, to map one single frame. It is not a hotpath in need of microptimising its locking pattern, and absolutely not something worth introducing a safety hazard for. ~Andrew
On 25/09/2020 14:17, Jan Beulich wrote: > On 22.09.2020 20:24, Andrew Cooper wrote: >> --- 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. */ >> + if ( !nr_frames ) >> + return -EINVAL; > I continue to object to this becoming an error. It's not a path any legitimate caller will ever exercise. POSIX defines any mmap() of zero length to be an error, and I completely agree. The problem isn't, per say, with accepting bogus arguments. It is the quantity of additional complexity in the hypervisor required to support accepting the bogus input cleanly. There are exactly 2 cases where 0 might be found here. Either the caller passed it in directly (and bypassed the POSIX check which would reject the attempt), or some part of multi-layer continuation handling went wrong on the previous iteration. For this hypercall (by the end of the series), _acquire_resource() returning 0 is specifically treated as an error so we don't livelock in 32-chunking loop until some other preemption kicks in. In this case, the check isn't actually necessary because it is (will be) guarded higher up the call chain in a more general way, but I'm not interested in adding unnecessary extra complexity (to area I've had to rewrite from scratch to remove the bugs) simply to support a non-existent usecase. ~Andrew
On 11.01.2021 22:22, Andrew Cooper wrote: > On 25/09/2020 14:17, Jan Beulich wrote: >> On 22.09.2020 20:24, Andrew Cooper wrote: >>> --- 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. */ >>> + if ( !nr_frames ) >>> + return -EINVAL; >> I continue to object to this becoming an error. > > It's not a path any legitimate caller will ever exercise. POSIX defines > any mmap() of zero length to be an error, and I completely agree. > > The problem isn't, per say, with accepting bogus arguments. It is the > quantity of additional complexity in the hypervisor required to support > accepting the bogus input cleanly. Is there any, besides s/-EINVAL/0/ for the line above? > There are exactly 2 cases where 0 might be found here. Either the > caller passed it in directly (and bypassed the POSIX check which would > reject the attempt), or some part of multi-layer continuation handling > went wrong on the previous iteration. > > For this hypercall (by the end of the series), _acquire_resource() > returning 0 is specifically treated as an error so we don't livelock in > 32-chunking loop until some other preemption kicks in. > > In this case, the check isn't actually necessary because it is (will be) > guarded higher up the call chain in a more general way, but I'm not > interested in adding unnecessary extra complexity (to area I've had to > rewrite from scratch to remove the bugs) simply to support a > non-existent usecase. In a couple of cases you've been calling out the principle of least surprise. This holds for the entirety of batched (in whatever form) hypercalls then as well. Either zero elements means "no-op" everywhere, or it gets treated as an error everywhere. Anything else is inconsistent and hence possibly surprising. To be clear - if others agree with your view, I'm not meaning to NAK the change in this form. But I'm also not going to knowingly ack introduction of inconsistencies. Jan
On 11.01.2021 22:22, Andrew Cooper wrote: > On 24/09/2020 10:51, Paul Durrant wrote: >>> 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. > > The whole point of relaxing the style was because feedback over minutia > such as this was deemed detrimental to the community. > > If I ever see feedback like this, I will commit commit the patch there > and then. This is the only way upstream Xen is going to turn into a > less toxic environment for contributors. Paul had clearly marked this as "nit". ./CODING_STYLE explicitly allows omission as well as presence of a full stop here. So while I agree with you that you'd be okay ignoring such a remark, I also agree with Paul giving the remark if he's found a nearby comment using the opposite variant. Besides relaxing requirements where sensible, at least local consistency also helps avoid such comments (now or for future contributors). Jan
> -----Original Message----- > From: Andrew Cooper <amc96@cam.ac.uk> > Sent: 11 January 2021 21:23 > To: paul@xen.org; 'Xen-devel' <xen-devel@lists.xenproject.org> > Cc: 'George Dunlap' <George.Dunlap@eu.citrix.com>; 'Ian Jackson' <iwj@xenproject.org>; 'Jan Beulich' > <JBeulich@suse.com>; 'Stefano Stabellini' <sstabellini@kernel.org>; 'Wei Liu' <wl@xen.org>; 'Julien > Grall' <julien@xen.org>; 'Michał Leszczyński' <michal.leszczynski@cert.pl>; 'Hubert Jasudowicz' > <hubert.jasudowicz@cert.pl>; 'Tamas K Lengyel' <tamas@tklengyel.com>; Andrew Cooper <amc96@cam.ac.uk> > Subject: Re: [PATCH v2 02/11] xen/gnttab: Rework resource acquisition > > On 24/09/2020 10:51, Paul Durrant wrote: > >> 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. > > The whole point of relaxing the style was because feedback over minutia > such as this was deemed detrimental to the community. > > If I ever see feedback like this, I will commit commit the patch there > and then. This is the only way upstream Xen is going to turn into a > less toxic environment for contributors. The tone of your response rather proves that Xen is already a toxic environment, I'm afraid. Paul
On 12/01/2021 08:15, Jan Beulich wrote: > On 11.01.2021 22:22, Andrew Cooper wrote: >> On 25/09/2020 14:17, Jan Beulich wrote: >>> On 22.09.2020 20:24, Andrew Cooper wrote: >>>> --- 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. */ >>>> + if ( !nr_frames ) >>>> + return -EINVAL; >>> I continue to object to this becoming an error. >> It's not a path any legitimate caller will ever exercise. POSIX defines >> any mmap() of zero length to be an error, and I completely agree. >> >> The problem isn't, per say, with accepting bogus arguments. It is the >> quantity of additional complexity in the hypervisor required to support >> accepting the bogus input cleanly. > Is there any, besides s/-EINVAL/0/ for the line above? It's really not that simple. I've dropped this particular hunk from v3 (as it is not necessary for safety at this point in the series), but retained the equivalent logical change in later patches where it does become necessary. > >> There are exactly 2 cases where 0 might be found here. Either the >> caller passed it in directly (and bypassed the POSIX check which would >> reject the attempt), or some part of multi-layer continuation handling >> went wrong on the previous iteration. >> >> For this hypercall (by the end of the series), _acquire_resource() >> returning 0 is specifically treated as an error so we don't livelock in >> 32-chunking loop until some other preemption kicks in. >> >> In this case, the check isn't actually necessary because it is (will be) >> guarded higher up the call chain in a more general way, but I'm not >> interested in adding unnecessary extra complexity (to area I've had to >> rewrite from scratch to remove the bugs) simply to support a >> non-existent usecase. > In a couple of cases you've been calling out the principle of least > surprise. This holds for the entirety of batched (in whatever form) > hypercalls then as well. Either zero elements means "no-op" > everywhere, or it gets treated as an error everywhere. Anything > else is inconsistent and hence possibly surprising. If you want to talk consistency, I'm fixing an inconsistency with this change, not introducing one. The existing logic will already yield EINVAL for addr==0, num !=0, rather than writing the size back. addr!=0, num==0 is the odd-combination-out. Its not a useful input, and won't be seen from legitimate callers, but can crop up from bugs in the continuation logic. ~Andrew
On 12/01/2021 08:23, Jan Beulich wrote: > On 11.01.2021 22:22, Andrew Cooper wrote: >> On 24/09/2020 10:51, Paul Durrant wrote: >>>> 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. >> The whole point of relaxing the style was because feedback over minutia >> such as this was deemed detrimental to the community. >> >> If I ever see feedback like this, I will commit commit the patch there >> and then. This is the only way upstream Xen is going to turn into a >> less toxic environment for contributors. > Paul had clearly marked this as "nit". ./CODING_STYLE explicitly > allows omission as well as presence of a full stop here. So while > I agree with you that you'd be okay ignoring such a remark, I > also agree with Paul giving the remark if he's found a nearby > comment using the opposite variant. A reply like that means "this needs changing". This is how it is read by people, even if it is not how it was intended. Contributors do not know CODING_STYLE off by heart, and even if they did, it is the responsibility of a reviewer not to mislead the contributor, not the contributors responsibility to challenge incorrect feedback. As it stands, the reply requires change of something which is explicitly permitted under CODING_STYLE. The change to CODING_STYLE was made specially to prevent feedback of this form to begin with. This is not an ok use of anyone's time, nor is repeating the argument which lead to CODING_STYLE being changed to begin with. ~Andrew
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. */ + 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: + rc = -EINVAL; + break; + } + rc = gnttab_get_status_frame_mfn(d, tot_frames - 1, &tmp); + break; + } + + /* Any errors from growing the table? */ + if ( rc ) + goto out; + + 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; }
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@citrix.com> --- CC: George Dunlap <George.Dunlap@eu.citrix.com> CC: Ian Jackson <iwj@xenproject.org> CC: Jan Beulich <JBeulich@suse.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Wei Liu <wl@xen.org> CC: Julien Grall <julien@xen.org> CC: Paul Durrant <paul@xen.org> CC: Michał Leszczyński <michal.leszczynski@cert.pl> CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl> CC: Tamas K Lengyel <tamas@tklengyel.com> 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(-)