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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

(x86 guys -- scope for making code common discussed below)

On Mon, 2014-03-10 at 09:25 +0100, 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@xxxxxxxxx>
> Cc: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> Cc: Paolo Valente <paolo.valente@xxxxxxxxxx>
> Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxxxxx>
> Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>
> Cc: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> Cc: Eric Trudeau <etrudeau@xxxxxxxxxxxx>
> Cc: Viktor Kleinik <viktor.kleinik@xxxxxxxxxxxxxxx>
> ---
>  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? */

"foo + foo_mfns" appears quite a lot here. I think you could calculate
those once into foo_start and foo_end and use throughout which would
make the remainder much easier to follow.

Oh, I see this came from the x86 function of which this is an almost
complete copy -- can it not be made common?

The difference seems to be map_mmio_regions vs. a loop over
set_mmio_p2m_entry which seems easy enough to abstract out.

> +             ((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);
> +     }
> +        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)
> +{
> +    return apply_p2m_changes(d, REMOVE, start_gaddr, end_gaddr,
> +                             maddr, MATTR_DEV, p2m_mmio_direct);
> +}
> +
>  int guest_physmap_add_entry(struct domain *d,
>                              unsigned long gpfn,
>                              unsigned long mfn,
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 3b39c45..907ce4a 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -88,6 +88,8 @@ int p2m_populate_ram(struct domain *d, paddr_t start, 
> paddr_t end);
>   * address maddr. */
>  int map_mmio_regions(struct domain *d, paddr_t start_gaddr,
>                       paddr_t end_gaddr, paddr_t maddr);
> +int unmap_mmio_regions(struct domain *d, paddr_t start_gaddr,
> +                       paddr_t end_gaddr, paddr_t maddr);
>  
>  int guest_physmap_add_entry(struct domain *d,
>                              unsigned long gfn,



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