Re: [Xen-devel] [V3 PATCH 5/9] PVH dom0: implement XENMEM_add_to_physmap_range for x86

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.11.13 at 03:27, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> This preparatory patch adds support for XENMEM_add_to_physmap_range
> on x86 so it can be used to create a guest on PVH dom0. To this end, we
> add a new function xenmem_add_to_physmap_range(), and change
> xenmem_add_to_physmap_once parameters so it can be called from
> xenmem_add_to_physmap_range.
> 
> Please note, compat will continue to return -ENOSYS.

And as noted a number of times before - I don't think that's
appropriate. There's nothing keeping non-PVH guests from using
this interface, and hence it should either be uniformly available to
all of them, or uniformly unavailable. I'm not intending to apply
such a half baked thing.

> +static int xenmem_add_to_physmap_range(struct domain *d,
> +                                       struct xen_add_to_physmap_range 
> *xatpr)
> +{
> +    int rc;

Why is this being left here ...

> +
> +    /* Process entries in reverse order to allow continuations */
> +    while ( xatpr->size > 0 )
> +    {
> +        xen_ulong_t idx;
> +        xen_pfn_t gpfn;
> +        struct xen_add_to_physmap xatp;

... when all the other ones are properly scope restricted?

> +
> +        if ( copy_from_guest_offset(&idx, xatpr->idxs, xatpr->size-1, 1)  ||
> +             copy_from_guest_offset(&gpfn, xatpr->gpfns, xatpr->size-1, 1) )
> +        {
> +            return -EFAULT;
> +        }

Pointless braces (and inconsistent with code further down).

> @@ -4689,6 +4725,10 @@ long arch_memory_op(int op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( copy_from_guest(&xatp, arg, 1) )
>              return -EFAULT;
>  
> +        /* This one is only supported for add_to_physmap_range */
> +        if ( xatp.space == XENMAPSPACE_gmfn_foreign )
> +            return -EINVAL;

Kind of an odd restriction - as this isn't by design, adding "for now"
would seem desirable.

> +        if ( (rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d)) )

By inverting the condition here ...

> +        {
> +            rcu_unlock_domain(d);
> +            return rc;
> +        }
> +
> +        rc = xenmem_add_to_physmap_range(d, &xatpr);

... and putting this into the body of the if(), you could avoid having
two distinct exit paths (under the premise that a XSM call wouldn't
return -EAGAIN).

Jan


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