[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 4/4] xen: introduce XENMEM_exchange_and_pin and XENMEM_unpin
On Thu, 15 Aug 2013, Jan Beulich wrote: > >>> On 15.08.13 at 13:18, Stefano Stabellini > >>> <stefano.stabellini@xxxxxxxxxxxxx> wrote: > > @@ -549,6 +549,15 @@ static long > > memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg) > > { > > for ( k = 0; k < (1UL << exch.out.extent_order); k++ ) > > set_gpfn_from_mfn(mfn + k, gpfn + k); > > + } > > + if ( op == XENMEM_exchange_and_pin ) > > + { > > + rc = guest_physmap_pin_range(d, gpfn, > > exch.out.extent_order); > > + if ( rc ) > > + continue; > > I guess you decided to use "continue" here because the other > paths do so too. Here, however, I don't think that's correct: The > pinning is an essential part of the operation, and the failure may > get masked by a later successful pin (because that could flush > rc back to zero) - as much as any other earlier failure that resulted > in rc being -EFAULT. > > So I'm afraid you have to go the unpleasant route here and do > the proper rollback if you encounter a failure here. Or see whether > the pinning can be moved even earlier. I'll add a check for the validity of the range at the beginning of the function. It looks like the safest and simplest thing to do. > > +static long unpin(XEN_GUEST_HANDLE_PARAM(xen_unpin_t) arg) > > +{ > > + int rc; > > + xen_ulong_t i; > > Wasn't it that xen_ulong_t is 64 bits wide on ARM32? If so, this > isn't suitable as an iterator used to index arrays (or really array > like constructs), and compilers might even warn about this. > > > + struct xen_unpin unpin; > > + xen_pfn_t gpfn; > > + struct domain *d; > > + > > + if ( copy_from_guest(&unpin, arg, 1) ) > > + return -EFAULT; > > + > > + /* Various sanity checks. */ > > + if ( /* Extent orders are sensible? */ > > + (unpin.in.extent_order > MAX_ORDER) || > > + /* Sizes of input list do not overflow a long? */ > > + ((~0UL >> unpin.in.extent_order) < unpin.in.nr_extents) ) > > Since you already bound check nr_extents against this unsigned > long value, making i unsigned long would be just fine. OK > > +struct xen_unpin { > > + /* > > + * [IN] Details of memory extents to be unpinned (GMFN bases). > > + * Note that @in.address_bits is ignored and unused. > > + */ > > + struct xen_memory_reservation in; > > +}; > > While the [IN] above is in line with your implementation - how does > a guest learn about partial success of the operation? I'll add a nr_unpinned to the struct _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |