Re: [Xen-devel] [PATCH 1/4] move XENMEM_add_to_physmap handling framework to common code

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-12-18 at 14:34 +0000, Jan Beulich wrote:
> There's really nothing really architecture specific here; the
> architecture specific handling is limited to
> xenmem_add_to_physmap_one().
> 
> Note that ARM's restriction of XENMAPSPACE_gmfn_foreign only being
> supported through XENMEM_add_to_physmap_range is being dropped as being
> pointless.

It's not pointless, dropping this restriction turns a comprehensible
EINVAL result into an incorrect ESRCH result. Which is incorrect because
no domain was specified so how can it not exist.

I'd also rather avoid introducing unnecessary/legacy hypercalls to ARM
if possible.

> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -968,7 +968,7 @@ void share_xen_page_with_privileged_gues
>      share_xen_page_with_guest(page, dom_xen, readonly);
>  }
>  
> -static int xenmem_add_to_physmap_one(
> +int xenmem_add_to_physmap_one(
>      struct domain *d,
>      uint16_t space,
>      domid_t foreign_domid,
> @@ -1118,37 +1118,6 @@ long arch_memory_op(int op, XEN_GUEST_HA
>  
>      switch ( op )
>      {
> -    case XENMEM_add_to_physmap:
> -    {
> -        struct xen_add_to_physmap xatp;
> -        struct domain *d;
> -
> -        if ( copy_from_guest(&xatp, arg, 1) )
> -            return -EFAULT;
> -
> -        /* Foreign mapping is only supported by 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;
> -
> -        rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d);
> -        if ( rc )
> -        {
> -            rcu_unlock_domain(d);
> -            return rc;
> -        }
> -
> -        rc = xenmem_add_to_physmap_one(d, xatp.space, DOMID_INVALID,
> -                                       xatp.idx, xatp.gpfn);
> -
> -        rcu_unlock_domain(d);
> -
> -        return rc;
> -    }
> -
>      case XENMEM_add_to_physmap_range:
>      {
>          struct xen_add_to_physmap_range xatpr;
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -542,6 +542,53 @@ static long memory_exchange(XEN_GUEST_HA
>      return rc;
>  }
>  
> +static int xenmem_add_to_physmap(struct domain *d,
> +                                 struct xen_add_to_physmap *xatp)
> +{
> +    struct xen_add_to_physmap start_xatp;
> +    int rc = 0;
> +
> +    if ( xatp->space != XENMAPSPACE_gmfn_range )
> +        return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID,
> +                                         xatp->idx, xatp->gpfn);
> +
> +#ifdef HAS_PASSTHROUGH
> +    if ( need_iommu(d) )
> +        this_cpu(iommu_dont_flush_iotlb) = 1;
> +#endif
> +
> +    start_xatp = *xatp;
> +    while ( xatp->size > 0 )
> +    {
> +        rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID,
> +                                       xatp->idx, xatp->gpfn);

ARM's xenmem_add_to_physmap_one does not support XENMAPSPACE_gmfn_range,
so it will return ENOSYS and fail out. I suppose that is ok. But it
makes the bulk of this function pointless. 

How about pulling the != XENMAPSPACE_gmfn_range check into do_memory_op
either calling xenmem_add_to_phymap_one or a per arch gmfn_range
function?

> +        if ( rc < 0 )
> +            break;
> +
> +        xatp->idx++;
> +        xatp->gpfn++;
> +        xatp->size--;
> +
> +        /* Check for continuation if it's not the last interation. */

That's the second or third time this "interation" typo has got
copied ;-)

> +        if ( xatp->size > 0 && hypercall_preempt_check() )
> +        {
> +            rc = -EAGAIN;
> +            break;
> +        }
> +    }
> +
> +#ifdef HAS_PASSTHROUGH
> +    if ( need_iommu(d) )
> +    {
> +        this_cpu(iommu_dont_flush_iotlb) = 0;
> +        iommu_iotlb_flush(d, start_xatp.idx, start_xatp.size - xatp->size);
> +        iommu_iotlb_flush(d, start_xatp.gpfn, start_xatp.size - xatp->size);
> +    }
> +#endif
> +
> +    return rc;
> +}
> +
>  long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
>      struct domain *d;
> @@ -673,6 +720,40 @@ long do_memory_op(unsigned long cmd, XEN
>  
>          break;
>  
> +    case XENMEM_add_to_physmap:
> +    {
> +        struct xen_add_to_physmap xatp;
> +
> +        if ( copy_from_guest(&xatp, arg, 1) )
> +            return -EFAULT;
> +
> +        d = rcu_lock_domain_by_any_id(xatp.domid);
> +        if ( d == NULL )
> +            return -ESRCH;
> +
> +        rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d);
> +        if ( rc )
> +        {
> +            rcu_unlock_domain(d);
> +            return rc;
> +        }
> +
> +        rc = xenmem_add_to_physmap(d, &xatp);
> +
> +        rcu_unlock_domain(d);
> +
> +        if ( xatp.space == XENMAPSPACE_gmfn_range && rc == -EAGAIN )
> +        {
> +            if ( !__copy_to_guest(arg, &xatp, 1) )
> +                rc = hypercall_create_continuation(
> +                        __HYPERVISOR_memory_op, "ih", op, arg);
> +            else
> +                rc = -EFAULT;
> +        }
> +
> +        return rc;
> +    }
> +
>      case XENMEM_remove_from_physmap:
>      {
>          struct xen_remove_from_physmap xrfp;



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