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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Mar 15, 2013 at 05:20:18PM -0700, Mukesh Rathor 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@oracle.com>
> ---
>  xen/arch/x86/mm.c |   82 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 79 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index d00d9a2..6603752 100644
> --- 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)
>  {
>      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);
>              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);
> +}
> +
> +static noinline int xenmem_add_to_physmap_range(struct domain *d,
> +                                       struct xen_add_to_physmap_range *xatpr)

noinline?

> +{
> +    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;
> +

Should you also set?
	xatp.err = 0;

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

And in case of error, like this, se xatp.err to rc?

> +    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) )
> +            rc = -EFAULT;

That is a bit odd. You are only copying in the values in case
of error? But what if the kernel gave you an 'xatpr' that has
garbage for the xatp.err field (which is marked as OUT). Should
we do the copy_to_guest irrespective of the rc?

> +
> +        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@lists.xen.org
> http://lists.xen.org/xen-devel
>