Re: [Xen-devel] [PATCH v4 4/4] xen: introduce XENMEM_exchange_and_pin and XENMEM_unpin

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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