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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> +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.

> +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?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel