Re: [RFC PATCH v2 2/3] arch, arm: add the XEN_DOMCTL_memory_mapping hypercall

[Julien Grall <julien.grall@linaro.org>]

Hello Arianna,

On 03/10/2014 08:25 AM, Arianna Avanzini wrote:
> This commit introduces a first attempt of implementation of the
> XEN_DOMCTL_memory_mapping hypercall for ARM. The range of I/O
> memory addresses is mapped all at once with map_mmio_regions().
> 
> Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
> Cc: Dario Faggioli <dario.faggioli@citrix.com>
> Cc: Paolo Valente <paolo.valente@unimore.it>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Julien Grall <julien.grall@citrix.com>
> Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
> ---
>  xen/arch/arm/domctl.c     | 69 +++++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/p2m.c        |  9 +++++++
>  xen/include/asm-arm/p2m.h |  2 ++
>  3 files changed, 80 insertions(+)
> 
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index 45974e7..078b165 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -10,6 +10,7 @@
>  #include <xen/errno.h>
>  #include <xen/sched.h>
>  #include <xen/hypercall.h>
> +#include <xen/iocap.h>
>  #include <public/domctl.h>
>  
>  long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
> @@ -30,7 +31,75 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>  
>          return p2m_cache_flush(d, s, e);
>      }
> +    case XEN_DOMCTL_memory_mapping:
> +    {
> +        unsigned long gfn = domctl->u.memory_mapping.first_gfn;
> +        unsigned long mfn = domctl->u.memory_mapping.first_mfn;
> +        unsigned long nr_mfns = domctl->u.memory_mapping.nr_mfns;
> +        int add = domctl->u.memory_mapping.add_mapping;
> +        long int ret;
> +
> +        ret = -EINVAL;
> +        if ( (mfn + nr_mfns - 1) < mfn || /* wrap? */
> +             ((mfn | (mfn + nr_mfns - 1)) >> (PADDR_BITS - PAGE_SHIFT)) ||
> +             (gfn + nr_mfns - 1) < gfn ) /* wrap? */
> +            return ret;
> +
> +        ret = -EPERM;
> +        if ( !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
> +            return ret;
>  
> +        ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, add);
> +        if ( ret )
> +            return ret;
> +
> +        if ( add )
> +        {
> +            printk(XENLOG_G_INFO
> +                   "memory_map: add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
> +                   d->domain_id, gfn, mfn, nr_mfns);
> +            ret = iomem_permit_access(d, mfn, mfn + nr_mfns - 1);
> +            if ( !ret )
> +            {
> +                ret = map_mmio_regions(d, PAGE_ALIGN(pfn_to_paddr(gfn)),
> +                                       PAGE_ALIGN(
> +                                           pfn_to_paddr(gfn + nr_mfns)) - 1,
> +                                       PAGE_ALIGN(pfn_to_paddr(mfn)));
> +                if ( ret )
> +                {
> +                    printk(XENLOG_G_WARNING
> +                           "memory_map: fail: dom%d gfn=%lx mfn=%lx\n",
> +                           d->domain_id, gfn, mfn);
> +                    if ( iomem_deny_access(d, mfn, mfn + nr_mfns - 1) &&
> +                         is_hardware_domain(current->domain) )
> +                        printk(XENLOG_ERR
> +                               "memory_map: failed to deny dom%d access "
> +                               "to [%lx,%lx]\n",
> +                               d->domain_id, mfn, mfn + nr_mfns - 1);
> +                }
> +            }
> +        }
> +	else
> +        {
> +            printk(XENLOG_G_INFO
> +                   "memory_map: remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
> +                   d->domain_id, gfn, mfn, nr_mfns);
> +
> +            add = unmap_mmio_regions(d, PAGE_ALIGN(pfn_to_paddr(gfn)),
> +                                     PAGE_ALIGN(
> +                                         pfn_to_paddr(gfn + nr_mfns)) - 1,
> +                                     PAGE_ALIGN(pfn_to_paddr(mfn)));
> +            ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
> +            if ( ret && add )
> +                ret = -EIO;
> +            if ( ret && is_hardware_domain(current->domain) )
> +                printk(XENLOG_ERR
> +                       "memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
> +                       ret, add ? "removing" : "denying", d->domain_id,
> +                       mfn, mfn + nr_mfns - 1);


The unmap part doesn't seem correct to me. With your solution, you are
allowed to remove any gfn as long as the current domain is permitted to
modify the mfn. No matter if gfn is effectively mapped to the mfn or not.

You should at least check that gfn is typed p2m_mmio_direct (This is
done by clean_mmio_p2m_entry on x86). You can also check that the gfn is
mapped to the mfn.

I would do that in the switch REMOVE in apply_p2m_changes.

> +	}
> +        return ret;
> +    }
>      default:
>          return subarch_do_domctl(domctl, d, u_domctl);
>      }
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index d00c882..710f74e 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -461,6 +461,15 @@ int map_mmio_regions(struct domain *d,
>                               maddr, MATTR_DEV, p2m_mmio_direct);
>  }
>  
> +int unmap_mmio_regions(struct domain *d,
> +                       paddr_t start_gaddr,
> +                       paddr_t end_gaddr,
> +                       paddr_t maddr)
> +{

Can you use pfn instead of physical address?

Regards,

-- 
Julien Grall