Re: [Xen-devel] [PATCH 2/18 V2]: PVH xen: add XENMEM_add_to_physmap_range

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.03.13 at 01:20, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> In this patch 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. There is no PVH specific change here.
> 
> Changes in V2:
>    - Do not break parameter so xenmem_add_to_physmap_once() but pass in
>      struct xen_add_to_physmap.
> 
> Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> ---
>  xen/arch/x86/mm.c |   82 
> +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 79 insertions(+), 3 deletions(-)

This continued to lack compat mode support (i.e. modification to
xen/arch/x86/x86_64/compat/mm.c:compat_arch_memory_op()).

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4268,7 +4268,8 @@ static int handle_iomem_range(unsigned long s, unsigned 
> long e, void *p)
>  
>  static int xenmem_add_to_physmap_once(
>      struct domain *d,
> -    const struct xen_add_to_physmap *xatp)
> +    const struct xen_add_to_physmap *xatp,
> +    domid_t foreign_domid)

So you add this new parameter but don't use it?

>  {
>      struct page_info *page = NULL;
>      unsigned long gfn = 0; /* gcc ... */
> @@ -4395,7 +4396,7 @@ static int xenmem_add_to_physmap(struct domain *d,
>          start_xatp = *xatp;
>          while ( xatp->size > 0 )
>          {
> -            rc = xenmem_add_to_physmap_once(d, xatp);
> +            rc = xenmem_add_to_physmap_once(d, xatp, -1);

And if it indeed is being used, please use a proper DOMID_* value
here.

>              if ( rc < 0 )
>                  return rc;
>  
> @@ -4421,7 +4422,52 @@ static int xenmem_add_to_physmap(struct domain *d,
>          return rc;
>      }
>  
> -    return xenmem_add_to_physmap_once(d, xatp);
> +    return xenmem_add_to_physmap_once(d, xatp, -1);

And here.

> +}
> +
> +static noinline int xenmem_add_to_physmap_range(struct domain *d,
> +                                       struct xen_add_to_physmap_range 
> *xatpr)
> +{
> +    int rc;
> +
> +    /* 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;
> +
> +        rc = copy_from_guest_offset(&idx, xatpr->idxs, xatpr->size-1, 1);
> +        if ( rc < 0 )
> +            goto out;
> +
> +        rc = copy_from_guest_offset(&gpfn, xatpr->gpfns, xatpr->size-1, 1);
> +        if ( rc < 0 )
> +            goto out;
> +
> +        xatp.space = xatpr->space;
> +        xatp.idx = idx;
> +        xatp.gpfn = gpfn;
> +        rc = xenmem_add_to_physmap_once(d, &xatp, xatpr->foreign_domid);
> +
> +        if (rc)
> +            goto out;

That doesn't seem right, together with you apparently not using
the "errs" array altogether.

> +
> +        xatpr->size--;
> +
> +        /* Check for continuation if it's not the last interation */
> +        if ( xatpr->size > 0 && hypercall_preempt_check() )
> +        {
> +            rc = -EAGAIN;
> +            goto out;
> +        }
> +    }
> +
> +    rc = 0;
> +
> +out:
> +    return rc;
> +
>  }
>  
>  long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
> @@ -4438,6 +4484,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;
> +
>          d = rcu_lock_domain_by_any_id(xatp.domid);
>          if ( d == NULL )
>              return -ESRCH;
> @@ -4465,6 +4515,32 @@ long arch_memory_op(int op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          return rc;
>      }
>  
> +    case XENMEM_add_to_physmap_range:
> +    {
> +        struct xen_add_to_physmap_range xatpr;
> +        struct domain *d;
> +
> +        if ( copy_from_guest(&xatpr, arg, 1) )
> +            return -EFAULT;
> +
> +        rc = rcu_lock_target_domain_by_id(xatpr.domid, &d);
> +        if ( rc != 0 )
> +            return rc;
> +
> +        rc = xenmem_add_to_physmap_range(d, &xatpr);
> +
> +        rcu_unlock_domain(d);
> +
> +        if ( rc && copy_to_guest(arg, &xatpr, 1) )

For one, shouldn't this be "!rc"?

And then you update ->size, but that one is specified to be only
and IN field. And considering that "errs" is the only OUT one, yet
that isn't even formally correct (because the field itself is an IN,
its what it points to where the output goes), I don't see why you
would need to copy back any part of the structure.

Jan

> +            rc = -EFAULT;
> +
> +        if ( rc == -EAGAIN )
> +            rc = hypercall_create_continuation(
> +                __HYPERVISOR_memory_op, "ih", op, arg);
> +
> +        return rc;
> +    }
> +
>      case XENMEM_set_memory_map:
>      {
>          struct xen_foreign_memory_map fmap;
> -- 
> 1.7.2.3
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx 
> http://lists.xen.org/xen-devel 



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