[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 1/7] xen/gnttab: Rework resource acquisition
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@xxxxxxxxxx> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |