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

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

Hello Arianna,

On 02/03/14 08:49, Arianna Avanzini wrote:
> This commit introduces a first attempt of implementation of the
> XEN_DOMCTL_memory_mapping hypercall for arm32. The following
> warnings must be taken into consideration:
> . currently, the hypercall simply performs an 1:1 mapping of I/O
>    memory ranges (mfn == gfn);
> . the range of I/O memory addresses is mapped all at once with
>    map_mmio_regions();
> . the hypercall takes for granted that any I/O memory range can
>    be mapped to a domU if the current domain is dom0.
>
> 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: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
> ---
>   xen/arch/arm/arm32/domctl.c | 74 +++++++++++++++++++++++++++++++++++++++++++++

The implementation of XEN_DOMCTL_memory_mapping is not ARM32 specific. 
It can be implemented in arch/arm/domctl.c.

>   xen/arch/arm/p2m.c          |  9 ++++++
>   xen/include/asm-arm/p2m.h   |  2 ++
>   3 files changed, 85 insertions(+)
>
> diff --git a/xen/arch/arm/arm32/domctl.c b/xen/arch/arm/arm32/domctl.c
> index c2ca4d3..67cf734 100644
> --- a/xen/arch/arm/arm32/domctl.c
> +++ b/xen/arch/arm/arm32/domctl.c
> @@ -10,8 +10,11 @@
>   #include <xen/errno.h>
>   #include <xen/sched.h>
>   #include <xen/hypercall.h>
> +#include <xen/iocap.h>
>   #include <public/domctl.h>
>
> +#include "cpu.h"
> +
>   long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>   {
> @@ -19,6 +22,77 @@ long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>       {
>       case XEN_DOMCTL_set_address_size:
>           return domctl->u.address_size.size == 32 ? 0 : -EINVAL;
> +    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;
> diff --git a/xen/arch/arm/arm32/domctl.c b/xen/arch/arm/
> +
> +        ret = -EPERM;
> +	/*
> +	 * NOTE: dom0 seems to have empty iomem_caps but to be however able to
> +	 *       access I/O memory ranges. The following check takes for granted
> +	 *       that any iomem range can be mapped to a domU if the current
> +	 *       domain is dom0.
> +	 */
> +        if ( current->domain->domain_id != 0 &&
> +             !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
> +            return ret;

This check can be removed by adding in construct_dom0 
(arch/arm/domain_build.c) something like that:
   /* DOM0 is permitted full I/O capabilities */
   rc = iomem_permit_access(d, 0UL, ~OUL);

I'm wondering if we can even restrict dom0 I/0 access by only permit 
access on devices passthrough to it. Because dom0 should not be allowed 
to map I/O ranges which belong to device used by Xen e.g : GIC, RAM,...

I think we should at least restrict dom0 to use the hypercall for 
mapping device memory. Otherwise dom0 may be allowed to map Xen address 
range, do wrong thing with foreign mapping...

> +
> +        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 )
> +            {
> +                /* 1:1 iomem mapping (gfn == mfn) */
> +                ret = map_mmio_regions(d, gfn, gfn + nr_mfns - 1, mfn);

The comment "1:1 iomem mapping (gfn == mfn)" seems wrong here. The 
implementation you wrote allow gfn != mfn.

Regards,

-- 
Julien Grall