Message ID | 20210112194841.1537-2-andrew.cooper3@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Multiple fixes to XENMEM_acquire_resource | expand |
On 12.01.2021 20:48, Andrew Cooper wrote: > 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> Reviewed-by: Jan Beulich <jbeulich@suse.com> albeit with a remark and a question: > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -4013,6 +4013,59 @@ 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; It doesn't look like this initializer was needed. The only use of i that I can spot is the loop near the end, which starts from 0. > + mfn_t tmp; > + void **vaddrs; > + int rc; > + > + /* Overflow checks */ > + if ( frame + nr_frames < frame ) > + return -EINVAL; > + > + tot_frames = frame + nr_frames; > + if ( tot_frames != frame + nr_frames ) > + return -EINVAL; Can't these two be folded into unsigned int tot_frames = frame + nr_frames; if ( tot_frames < frame ) return -EINVAL; ? Both truncation and wrapping look to be taken care of this way. Jan
On 12.01.2021 20:48, Andrew Cooper wrote: > 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> I'm sorry, but I have to withdraw the R-b given a few minutes ago. > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -4013,6 +4013,59 @@ 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; > + int rc; > + > + /* Overflow checks */ > + if ( frame + nr_frames < frame ) > + return -EINVAL; > + > + tot_frames = frame + nr_frames; > + if ( tot_frames != frame + nr_frames ) > + return -EINVAL; While you did remove the explicit returning of an error when nr_frames is zero, ... > + /* Grow table if necessary. */ > + grant_write_lock(gt); > + rc = -EINVAL; > + switch ( id ) > + { > + case XENMEM_resource_grant_table_id_shared: > + vaddrs = gt->shared_raw; > + rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp); ... this will degenerate (and still cause an error) when frame is also zero, and will cause undue growing of the table when frame is non-zero yet not overly large. As indicated before, I'm of the clear opinion that here - like elsewhere - a number of zero frames requested means that no action be taken at all, and success be returned. > + break; > + > + case XENMEM_resource_grant_table_id_status: > + if ( gt->gt_version != 2 ) > + break; As per various other places in this file I think you want to wrap this in evaluate_nospec(). Jan
On 15/01/2021 11:43, Jan Beulich wrote: >> --- a/xen/common/grant_table.c >> +++ b/xen/common/grant_table.c >> @@ -4013,6 +4013,59 @@ 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; > It doesn't look like this initializer was needed. The only > use of i that I can spot is the loop near the end, which > starts from 0. Its possibly stale from v1. I'll adjust. >> + mfn_t tmp; >> + void **vaddrs; >> + int rc; >> + >> + /* Overflow checks */ >> + if ( frame + nr_frames < frame ) >> + return -EINVAL; >> + >> + tot_frames = frame + nr_frames; >> + if ( tot_frames != frame + nr_frames ) >> + return -EINVAL; > Can't these two be folded into > > unsigned int tot_frames = frame + nr_frames; > > if ( tot_frames < frame ) > return -EINVAL; > > ? Both truncation and wrapping look to be taken care of this > way. Not when frame is a multiple of 4G (or fractionally over, I think). This ABI with mismatched widths really is obnoxious to work with. ~Andrew
On 15.01.2021 17:03, Andrew Cooper wrote: > On 15/01/2021 11:43, Jan Beulich wrote: >>> + mfn_t tmp; >>> + void **vaddrs; >>> + int rc; >>> + >>> + /* Overflow checks */ >>> + if ( frame + nr_frames < frame ) >>> + return -EINVAL; >>> + >>> + tot_frames = frame + nr_frames; >>> + if ( tot_frames != frame + nr_frames ) >>> + return -EINVAL; >> Can't these two be folded into >> >> unsigned int tot_frames = frame + nr_frames; >> >> if ( tot_frames < frame ) >> return -EINVAL; >> >> ? Both truncation and wrapping look to be taken care of this >> way. > > Not when frame is a multiple of 4G (or fractionally over, I think). How that? In this case any unsigned int value will be less than frame. Jan
On 15.01.2021 17:26, Jan Beulich wrote: > On 15.01.2021 17:03, Andrew Cooper wrote: >> On 15/01/2021 11:43, Jan Beulich wrote: >>>> + mfn_t tmp; >>>> + void **vaddrs; >>>> + int rc; >>>> + >>>> + /* Overflow checks */ >>>> + if ( frame + nr_frames < frame ) >>>> + return -EINVAL; >>>> + >>>> + tot_frames = frame + nr_frames; >>>> + if ( tot_frames != frame + nr_frames ) >>>> + return -EINVAL; >>> Can't these two be folded into >>> >>> unsigned int tot_frames = frame + nr_frames; >>> >>> if ( tot_frames < frame ) >>> return -EINVAL; >>> >>> ? Both truncation and wrapping look to be taken care of this >>> way. >> >> Not when frame is a multiple of 4G (or fractionally over, I think). > > How that? In this case any unsigned int value will be less than > frame. And in the 32-bit case the above becomes a simple overflow check. Jan
On 15/01/2021 11:56, Jan Beulich wrote: >> + /* Grow table if necessary. */ >> + grant_write_lock(gt); >> + rc = -EINVAL; >> + switch ( id ) >> + { >> + case XENMEM_resource_grant_table_id_shared: >> + vaddrs = gt->shared_raw; >> + rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp); > ... this will degenerate (and still cause an error) when frame > is also zero, and will cause undue growing of the table when > frame is non-zero yet not overly large. Urgh, yes - that is why I had the check. In which case I retract my change between v2 and v3 here. > As indicated before, I'm of the clear opinion that here - like > elsewhere - a number of zero frames requested means that no > action be taken at all, and success be returned. The general world we work in (POSIX) agrees with my opinion over yours when it comes to this matter. I spent a lot of time and effort getting this logic correct in v2, and I do not have any further time to waste adding complexity to support a non-existent corner case, nor is it reasonable to further delay all the work which is depending on this series. This entire mess is already too damn complicated, without taking extra complexity. Entertaining the idea of supporting 0 length requests is really not as simple as you seem to think it is, and is a large part of why I'm stubbornly refusing to do so. I am going to commit this patch (with some of the other minor adjustments). If you wish to add the extra complexity yourself, you are welcome to all the unit tests I put together which exercise the complexity, but I will not ack the resulting change. ~Andrew
On 15.01.2021 17:57, Andrew Cooper wrote: > On 15/01/2021 11:56, Jan Beulich wrote: >>> + /* Grow table if necessary. */ >>> + grant_write_lock(gt); >>> + rc = -EINVAL; >>> + switch ( id ) >>> + { >>> + case XENMEM_resource_grant_table_id_shared: >>> + vaddrs = gt->shared_raw; >>> + rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp); >> ... this will degenerate (and still cause an error) when frame >> is also zero, and will cause undue growing of the table when >> frame is non-zero yet not overly large. > > Urgh, yes - that is why I had the check. > > In which case I retract my change between v2 and v3 here. > >> As indicated before, I'm of the clear opinion that here - like >> elsewhere - a number of zero frames requested means that no >> action be taken at all, and success be returned. > > The general world we work in (POSIX) agrees with my opinion over yours > when it comes to this matter. I assume you are referring to mmap()? I ask because I think there are numerous counter examples (some even in the C standard): malloc() & friends allow for either behavior. memcpy() / memmove() happily do nothing when passed a zero size. read() / write() are at least allowed to read/write nothing (and return success) when told so. Otoh I notice that a zero vector count passed to readv() / writev() is indeed an error, yet nothing is said at all about individual vector elements specifying zero size. Plus of course I don't think POSIX is the main reference point here, when the rest of the hypercalls allowing for some form of batching permit empty batches. > I spent a lot of time and effort getting this logic correct in v2, and I > do not have any further time to waste adding complexity to support a > non-existent corner case, nor is it reasonable to further delay all the > work which is depending on this series. This entire mess is already too > damn complicated, without taking extra complexity. > > Entertaining the idea of supporting 0 length requests is really not as > simple as you seem to think it is, and is a large part of why I'm > stubbornly refusing to do so. I'd be really happy to be educated of the complications; sadly so far you've only claimed ones would exist without actually going into sufficient detail. In particular I don't view placing if ( size == 0 ) return 0; suitably early coming anywhere near "complexity". Even more so that as per your reply you mean to undo removal of a respective check, just that in your version it'll return an error instead of success. > I am going to commit this patch (with some of the other minor adjustments). I'm not concerned enough of the introduced inconsistency to outright veto you doing so, but I still don't think this is an appropriate step to take under the present conditions. Jan
On 18/01/2021 08:23, Jan Beulich wrote: > On 15.01.2021 17:57, Andrew Cooper wrote: >> On 15/01/2021 11:56, Jan Beulich wrote: >>>> + /* Grow table if necessary. */ >>>> + grant_write_lock(gt); >>>> + rc = -EINVAL; >>>> + switch ( id ) >>>> + { >>>> + case XENMEM_resource_grant_table_id_shared: >>>> + vaddrs = gt->shared_raw; >>>> + rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp); >>> ... this will degenerate (and still cause an error) when frame >>> is also zero, and will cause undue growing of the table when >>> frame is non-zero yet not overly large. >> Urgh, yes - that is why I had the check. >> >> In which case I retract my change between v2 and v3 here. >> >>> As indicated before, I'm of the clear opinion that here - like >>> elsewhere - a number of zero frames requested means that no >>> action be taken at all, and success be returned. >> The general world we work in (POSIX) agrees with my opinion over yours >> when it comes to this matter. > I assume you are referring to mmap()? I ask because I think there > are numerous counter examples (some even in the C standard): > malloc() & friends allow for either behavior. memcpy() / memmove() > ... This entire infrastructure lives behind an mmap() (or equivalent) system call. Other specs are not relevant. Any request for a zero sized mapping is a bug. It is either a buggy caller, or buggy continuation logic. >> I spent a lot of time and effort getting this logic correct in v2, and I >> do not have any further time to waste adding complexity to support a >> non-existent corner case, nor is it reasonable to further delay all the >> work which is depending on this series. This entire mess is already too >> damn complicated, without taking extra complexity. >> >> Entertaining the idea of supporting 0 length requests is really not as >> simple as you seem to think it is, and is a large part of why I'm >> stubbornly refusing to do so. > I'd be really happy to be educated of the complications; sadly > so far you've only claimed ones would exist without actually > going into sufficient detail. In particular I don't view placing > > if ( size == 0 ) > return 0; > > suitably early coming anywhere near "complexity". Even more so > that as per your reply you mean to undo removal of a respective > check, just that in your version it'll return an error instead > of success. I am not being a belligerent arse for the sake of it. I've got far better things I ought to be doing with my time. I spent a lot of time getting this working, and with sufficient testing to demonstrate it working in practice, particularly in continuation cases. You've spent a lot of time and effort insisting that I reintroduce support a fundamentally broken corner case, despite my best attempts to demonstrate why it will livelock in the hypervisor because of the mess which is the double looping between the compat and native handlers. You've also spent a lot of time obfuscating the overflow checks and breaking them in the process. You are welcome, in your own time - and not mine, to use the testing infrastructure I've already provided to discover why the compat mess has a habit of livelocking in certain continuation cases. (It won't actually livelock if you switch to using return 0. You'll instead hit the ASSERT_UNREACHABLE() I put in a failsafe path specifically to avoid bugs in this are turning back into XSAs.) Combined with the fact that 0 length requests are buggy in all circumstances, I chose to implement logic which is robust even in the case of failure, because the compat logic is almost intractable and borderline untestable because noone runs 32bit kernels in anger these days. I can't commit my test infrastructure which spots the problems, because we obviously can't have a hypercall which panics when the input buffer doesn't match the test pattern. I am not inclined to adopt an approach which is fundamentally more likely to contain security vulnerabilities in a fragile and untestable area of code. You are going to have to come up with a far more compelling argument that "because I want to support 0 length mapping requests" to change my mind. ~Andrew
On 28.01.2021 23:56, Andrew Cooper wrote: > On 18/01/2021 08:23, Jan Beulich wrote: >> On 15.01.2021 17:57, Andrew Cooper wrote: >>> On 15/01/2021 11:56, Jan Beulich wrote: >>>>> + /* Grow table if necessary. */ >>>>> + grant_write_lock(gt); >>>>> + rc = -EINVAL; >>>>> + switch ( id ) >>>>> + { >>>>> + case XENMEM_resource_grant_table_id_shared: >>>>> + vaddrs = gt->shared_raw; >>>>> + rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp); >>>> ... this will degenerate (and still cause an error) when frame >>>> is also zero, and will cause undue growing of the table when >>>> frame is non-zero yet not overly large. >>> Urgh, yes - that is why I had the check. >>> >>> In which case I retract my change between v2 and v3 here. >>> >>>> As indicated before, I'm of the clear opinion that here - like >>>> elsewhere - a number of zero frames requested means that no >>>> action be taken at all, and success be returned. >>> The general world we work in (POSIX) agrees with my opinion over yours >>> when it comes to this matter. >> I assume you are referring to mmap()? I ask because I think there >> are numerous counter examples (some even in the C standard): >> malloc() & friends allow for either behavior. memcpy() / memmove() >> ... > > This entire infrastructure lives behind an mmap() (or equivalent) system > call. Other specs are not relevant. With this you're implying restrictions on what callers might use this for. I don't see why a ring-0 only environment would necessarily have anything like mmap(). Anyway, I'm not going to further comment on any of the below. I'm not going to either ack or nak this change, so if you have the agreement of others feel free to put in. Jan > Any request for a zero sized mapping is a bug. It is either a buggy > caller, or buggy continuation logic. > >>> I spent a lot of time and effort getting this logic correct in v2, and I >>> do not have any further time to waste adding complexity to support a >>> non-existent corner case, nor is it reasonable to further delay all the >>> work which is depending on this series. This entire mess is already too >>> damn complicated, without taking extra complexity. >>> >>> Entertaining the idea of supporting 0 length requests is really not as >>> simple as you seem to think it is, and is a large part of why I'm >>> stubbornly refusing to do so. >> I'd be really happy to be educated of the complications; sadly >> so far you've only claimed ones would exist without actually >> going into sufficient detail. In particular I don't view placing >> >> if ( size == 0 ) >> return 0; >> >> suitably early coming anywhere near "complexity". Even more so >> that as per your reply you mean to undo removal of a respective >> check, just that in your version it'll return an error instead >> of success. > > I am not being a belligerent arse for the sake of it. I've got far > better things I ought to be doing with my time. > > I spent a lot of time getting this working, and with sufficient testing > to demonstrate it working in practice, particularly in continuation cases. > > You've spent a lot of time and effort insisting that I reintroduce > support a fundamentally broken corner case, despite my best attempts to > demonstrate why it will livelock in the hypervisor because of the mess > which is the double looping between the compat and native handlers. > > You've also spent a lot of time obfuscating the overflow checks and > breaking them in the process. > > You are welcome, in your own time - and not mine, to use the testing > infrastructure I've already provided to discover why the compat mess has > a habit of livelocking in certain continuation cases. (It won't > actually livelock if you switch to using return 0. You'll instead hit > the ASSERT_UNREACHABLE() I put in a failsafe path specifically to avoid > bugs in this are turning back into XSAs.) > > Combined with the fact that 0 length requests are buggy in all > circumstances, I chose to implement logic which is robust even in the > case of failure, because the compat logic is almost intractable and > borderline untestable because noone runs 32bit kernels in anger these > days. I can't commit my test infrastructure which spots the problems, > because we obviously can't have a hypercall which panics when the input > buffer doesn't match the test pattern. > > I am not inclined to adopt an approach which is fundamentally more > likely to contain security vulnerabilities in a fragile and untestable > area of code. You are going to have to come up with a far more > compelling argument that "because I want to support 0 length mapping > requests" to change my mind. > > ~Andrew >
Jan Beulich writes ("Re: [PATCH v3 1/7] xen/gnttab: Rework resource acquisition"): > Anyway, I'm not going to further comment on any of the below. > I'm not going to either ack or nak this change, so if you > have the agreement of others feel free to put in. This patch, Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index a5d3ed8bda..f560c250d7 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -4013,6 +4013,59 @@ 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; + int rc; + + /* 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); + rc = -EINVAL; + switch ( id ) + { + case XENMEM_resource_grant_table_id_shared: + vaddrs = gt->shared_raw; + rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp); + break; + + case XENMEM_resource_grant_table_id_status: + if ( gt->gt_version != 2 ) + break; + + /* 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; + rc = gnttab_get_status_frame_mfn(d, tot_frames - 1, &tmp); + break; + } + + /* Any errors? Bad id, or from growing the table? */ + if ( rc ) + 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 +4100,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 b21b6c452d..82cf7b41ee 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -1052,44 +1052,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) { @@ -1144,8 +1106,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> v3: * Fold switch statements in gnttab_acquire_resource() --- xen/common/grant_table.c | 80 ++++++++++++++++++++++++++++--------------- xen/common/memory.c | 42 ++--------------------- xen/include/xen/grant_table.h | 19 ++++------ 3 files changed, 62 insertions(+), 79 deletions(-)