[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 7/7] xen/memory: Fix mapping grant tables with XENMEM_acquire_resource
On 12.01.2021 20:48, Andrew Cooper wrote: > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -4628,7 +4628,6 @@ int arch_acquire_resource(struct domain *d, unsigned > int type, > if ( id != (unsigned int)ioservid ) > break; > > - rc = 0; > for ( i = 0; i < nr_frames; i++ ) > { > mfn_t mfn; How "good" are our chances that older gcc won't recognize that without this initialization ... > @@ -4639,6 +4638,9 @@ int arch_acquire_resource(struct domain *d, unsigned > int type, > > mfn_list[i] = mfn_x(mfn); > } > + if ( i == nr_frames ) > + /* Success. Passed nr_frames back to the caller. */ > + rc = nr_frames; ... rc will nevertheless not remain uninitialized when nr_frames is zero? > @@ -432,6 +419,28 @@ int compat_memory_op(unsigned int cmd, > XEN_GUEST_HANDLE_PARAM(void) compat) > > if ( xen_frame_list && cmp.mar.nr_frames ) > { > + unsigned int xlat_max_frames = > + (COMPAT_ARG_XLAT_SIZE - sizeof(*nat.mar)) / > + sizeof(*xen_frame_list); > + > + if ( start_extent >= nat.mar->nr_frames ) Comparing with the enclosing if() I find it slightly confusing that different instances of nr_frames get used. Perhaps best to adjust the earlier patch to also use nat.mar->nr_frames? Or is this perhaps on purpose? > + return -EINVAL; > + > + /* > + * Adjust nat to account for work done on previous > + * continuations, leaving cmp pristine. Hide the > continaution Nit: continuation > + * from the native code to prevent double accounting. > + */ > + nat.mar->nr_frames -= start_extent; > + nat.mar->frame += start_extent; > + cmd &= MEMOP_CMD_MASK; > + > + /* > + * If there are two many frames to fit within the xlat > buffer, s/two/too/ > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -1072,17 +1072,31 @@ static unsigned int resource_max_frames(const struct > domain *d, > } > } > > +/* > + * Returns -errno on error, or positive in the range [1, nr_frames] on > + * success. Returning less than nr_frames contitutes a request for a Nit: constitutes > @@ -1127,26 +1138,47 @@ static int acquire_resource( > goto out; > } > > + /* > + * Limiting nr_frames at (UINT_MAX >> MEMOP_EXTENT_SHIFT) isn't ideal. > If > + * it ever becomes a practical problem, we can switch to mutating > + * xmar.{frame,nr_frames,frame_list} in guest memory. > + */ > + rc = -EINVAL; > + if ( start_extent >= xmar.nr_frames || At the very least start_extend == 0 and xmar.nr_frames == 0 ought to be "success" (with nothing done). Elsewhere I think we generalize this and only reject start_extent > nr, which to me suggests we also should do so here (and in the compat handling) for consistency. Of course this then means ... > + xmar.nr_frames > (UINT_MAX >> MEMOP_EXTENT_SHIFT) ) > + goto out; > + > + /* Adjust for work done on previous continuations. */ > + xmar.nr_frames -= start_extent; > + xmar.frame += start_extent; > + guest_handle_add_offset(xmar.frame_list, start_extent); > + > do { > - switch ( xmar.type ) > - { > - case XENMEM_resource_grant_table: > - rc = gnttab_acquire_resource(d, xmar.id, xmar.frame, > xmar.nr_frames, > - mfn_list); > - break; > + /* > + * Arbitrary size. Not too much stack space, and a reasonable stride > + * for continuation checks. > + */ > + xen_pfn_t mfn_list[32]; > + unsigned int todo = MIN(ARRAY_SIZE(mfn_list), xmar.nr_frames), done; > > - default: > - rc = arch_acquire_resource(d, xmar.type, xmar.id, xmar.frame, > - xmar.nr_frames, mfn_list); > - break; > - } > + rc = _acquire_resource(d, xmar.type, xmar.id, xmar.frame, > + todo, mfn_list); > + if ( rc < 0 ) > + goto out; > > - if ( rc ) > + done = rc; > + rc = 0; > + if ( done == 0 || done > todo ) ... to only retain the right side of the || here and ... > @@ -1166,7 +1198,32 @@ static int acquire_resource( > rc = -EIO; > } > } > - } while ( 0 ); > + > + if ( rc ) > + goto out; > + > + xmar.nr_frames -= done; > + xmar.frame += done; > + guest_handle_add_offset(xmar.frame_list, done); > + start_extent += done; > + > + /* > + * Explicit continuation request from _acquire_resource(), or we've > + * still got more work to do. > + */ > + if ( done < todo || > + (xmar.nr_frames && hypercall_preempt_check()) ) > + { > + rc = hypercall_create_continuation( > + __HYPERVISOR_memory_op, "lh", > + XENMEM_acquire_resource | (start_extent << > MEMOP_EXTENT_SHIFT), > + arg); > + goto out; > + } > + > + } while ( xmar.nr_frames ); ... converting this loop from do-while to just while. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |