[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall



On Tue, 25 Mar 2014, Arianna Avanzini wrote:
> This commit introduces a first attempt of implementation of the
> XEN_DOMCTL_memory_mapping hypercall for ARM. As the implementation
> would have been almost identical to the one for x86, this code has
> been made common to both the architectures. The only difference
> between the two procedures lies in the arch-specific implementation
> of the map_mmio_regions() and unmap_mmio_regions() functions.
> 
> 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: Jan Beulich <JBeulich@xxxxxxxx>
> Cc: Keir Fraser <keir@xxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Jan Beulich <JBeulich@xxxxxxxx>
> Cc: Keir Fraser <keir@xxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> Cc: Eric Trudeau <etrudeau@xxxxxxxxxxxx>
> Cc: Viktor Kleinik <viktor.kleinik@xxxxxxxxxxxxxxx>
> 
> ---
> 
>     v4:
>         - Use a define for paddr_bits instead of a new variable.
>         - Define prototypes for common functions map_mmio_regions() and
>           unmap_mmio_regions() only once in a common header.
>         - Fix type and signedness of local variables used as indexes in
>           map_mmio_regions() and unmap_mmio_regions() for x86.
>         - Clear p2m entries in map_mmio_regions() for x86 only if 
>           set_mmio_p2m_entry() returned with and error.
>         - Make ranges inclusive of the end address in map_mmio_regions()
>           and unmap_mmio_regions() for x86.
>         - Turn hard tabs into spaces.
> 
>     v3:
>         - Move code to xen/common/domctl.c; abstract out differences between
>           the x86 and ARM code:
>           . add map_mmio_regions() and unmap_mmio_regions() functions for x86;
>           . add a paddr_bits variable for ARM.
>         - Use pfn as parameters to the unmap_mmio_regions() function.
>         - Compute gfn + nr_mfns and mfn + nr_mfns only once.
> 
>     v2:
>         - Move code to xen/arm/domctl.c.
>         - Use the already-defined PADDR_BITS constant in the new DOMCTL.
>         - Use paddr_t as arguments to the map_mmio_regions() function.
>         - Page-align addresses given as arguments to map_mmio_regions() and
>           unmap_mmio_regions(). 
> 
> ---
>  xen/arch/arm/p2m.c        | 12 ++++++++
>  xen/arch/x86/domctl.c     | 70 
> -----------------------------------------------
>  xen/arch/x86/mm/p2m.c     | 40 +++++++++++++++++++++++++++
>  xen/common/domctl.c       | 65 +++++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/p2m.h |  9 ++----
>  xen/include/asm-x86/p2m.h |  1 +
>  xen/include/xen/p2m.h     | 16 +++++++++++
>  7 files changed, 137 insertions(+), 76 deletions(-)
>  create mode 100644 xen/include/xen/p2m.h
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index c8e77b9..961213d 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -480,6 +480,18 @@ int map_mmio_regions(struct domain *d,
>                               MATTR_DEV, p2m_mmio_direct);
>  }
>  
> +int unmap_mmio_regions(struct domain *d,
> +                       unsigned long start_gfn,
> +                       unsigned long end_gfn,
> +                       unsigned long mfn)
> +{
> +    return apply_p2m_changes(d, REMOVE,
> +                             pfn_to_paddr(start_gfn),
> +                             pfn_to_paddr(end_gfn),
> +                             pfn_to_paddr(mfn),
> +                             MATTR_DEV, p2m_mmio_direct);
> +}
> +
>  int guest_physmap_add_entry(struct domain *d,
>                              unsigned long gpfn,
>                              unsigned long mfn,
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 26635ff..69a7fbf 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -639,76 +639,6 @@ long arch_do_domctl(
>      }
>      break;
>  
> -    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;
> -        unsigned long i;
> -
> -        ret = -EINVAL;
> -        if ( (mfn + nr_mfns - 1) < mfn || /* wrap? */
> -             ((mfn | (mfn + nr_mfns - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
> -             (gfn + nr_mfns - 1) < gfn ) /* wrap? */
> -            break;
> -
> -        ret = -EPERM;
> -        if ( !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 
> 1) )
> -            break;
> -
> -        ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, add);
> -        if ( ret )
> -            break;
> -
> -        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 && paging_mode_translate(d) )
> -            {
> -                for ( i = 0; !ret && i < nr_mfns; i++ )
> -                    if ( !set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i)) )
> -                        ret = -EIO;
> -                if ( ret )
> -                {
> -                    printk(XENLOG_G_WARNING
> -                           "memory_map:fail: dom%d gfn=%lx mfn=%lx\n",
> -                           d->domain_id, gfn + i, mfn + i);
> -                    while ( i-- )
> -                        clear_mmio_p2m_entry(d, gfn + i);
> -                    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);
> -
> -            if ( paging_mode_translate(d) )
> -                for ( i = 0; i < nr_mfns; i++ )
> -                    add |= !clear_mmio_p2m_entry(d, gfn + i);
> -            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);
> -        }
> -    }
> -    break;
> -
>      case XEN_DOMCTL_ioport_mapping:
>      {
>  #define MAX_IOPORTS    0x10000
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 8f380ed..a44d7a3 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1624,6 +1624,46 @@ unsigned long paging_gva_to_gfn(struct vcpu *v,
>      return hostmode->gva_to_gfn(v, hostp2m, va, pfec);
>  }
>  
> +int map_mmio_regions(struct domain *d,
> +                     unsigned long start_gfn,
> +                     unsigned long end_gfn,
> +                     unsigned long mfn)
> +{
> +    int ret = 0;
> +    unsigned long i;
> +    unsigned long nr_mfns = end_gfn - start_gfn + 1;
> +
> +    if ( !paging_mode_translate(d) )
> +        return 0;
> +
> +    for ( i = 0; !ret && i < nr_mfns; i++ )
> +        if ( !set_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn + i)) )
> +            ret = -EIO;
> +    if ( ret )
> +        while ( i-- )
> +            clear_mmio_p2m_entry(d, start_gfn + i);
> +
> +    return ret;
> +}
> +
> +int unmap_mmio_regions(struct domain *d,
> +                       unsigned long start_gfn,
> +                       unsigned long end_gfn,
> +                       unsigned long mfn)
> +{
> +    int ret = 0;
> +    unsigned long nr_mfns = end_gfn - start_gfn + 1;
> +    unsigned long i;
> +
> +    if ( !paging_mode_translate(d) )
> +        return 0;
> +
> +    for ( i = 0; i < nr_mfns; i++ )
> +        ret |= !clear_mmio_p2m_entry(d, start_gfn + i);
> +    return ret;
> +}
> +
>  /*** Audit ***/
>  
>  #if P2M_AUDIT
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 7cf610a..ae120d9 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -818,6 +818,71 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>      }
>      break;
>  
> +    case XEN_DOMCTL_memory_mapping:
> +    {
> +        unsigned long gfn = op->u.memory_mapping.first_gfn;
> +        unsigned long mfn = op->u.memory_mapping.first_mfn;
> +        unsigned long nr_mfns = op->u.memory_mapping.nr_mfns;
> +        unsigned long mfn_end = mfn + nr_mfns;
> +        unsigned long gfn_end = gfn + nr_mfns;
> +        int add = op->u.memory_mapping.add_mapping;
> +
> +        ret = -EINVAL;
> +        if ( (mfn_end - 1) < mfn || /* wrap? */
> +             ((mfn | (mfn_end - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
> +             (gfn_end - 1) < gfn ) /* wrap? */
> +            return ret;
> +
> +        ret = -EPERM;
> +        if ( !iomem_access_permitted(current->domain, mfn, mfn_end - 1) )
> +            return ret;
> +
> +        ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end - 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_end - 1);
> +            if ( !ret )
> +            {
> +                ret = map_mmio_regions(d, gfn, gfn_end - 1, 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_end - 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_end - 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, gfn, gfn_end - 1, mfn);
> +            ret = iomem_deny_access(d, mfn, mfn_end - 1);
> +            if ( ret && add )
> +                ret = -EIO;

This is a functional change. The removed code did:

-            ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
-            if ( !ret && add )
-                ret = -EIO;

The change looks correct to me, but I would appreciate if you could
mention it explicitly in the commit message.


> +            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_end - 1);
> +        }
> +    }
> +    break;
> +
>      case XEN_DOMCTL_settimeoffset:
>      {
>          domain_set_time_offset(d, op->u.settimeoffset.time_offset_seconds);
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index d2d2ce3..2792d0d 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -2,6 +2,9 @@
>  #define _XEN_P2M_H
>  
>  #include <xen/mm.h>
> +#include <xen/p2m.h>
> +
> +#define paddr_bits PADDR_BITS
>  
>  struct domain;
>  
> @@ -83,12 +86,6 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, 
> xen_pfn_t end_mfn);
>  
>  /* Setup p2m RAM mapping for domain d from start-end. */
>  int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end);
> -/* Map MMIO regions in the p2m: start_gfn and end_gfn is the range in the 
> guest
> - * physical address space to map, starting from the machine frame number 
> mfn. */
> -int map_mmio_regions(struct domain *d,
> -                     unsigned long start_gfn,
> -                     unsigned long end_gfn,
> -                     unsigned long mfn);
>  
>  int guest_physmap_add_entry(struct domain *d,
>                              unsigned long gfn,
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index f4e7253..e7bf06d 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -29,6 +29,7 @@
>  
>  #include <xen/config.h>
>  #include <xen/paging.h>
> +#include <xen/p2m.h>
>  #include <asm/mem_sharing.h>
>  #include <asm/page.h>    /* for pagetable_t */
>  
> diff --git a/xen/include/xen/p2m.h b/xen/include/xen/p2m.h
> new file mode 100644
> index 0000000..2be9a86
> --- /dev/null
> +++ b/xen/include/xen/p2m.h
> @@ -0,0 +1,16 @@
> +#ifndef _XEN_P2M_COMMON_H
> +#define _XEN_P2M_COMMON_H
> +
> +/* Map MMIO regions in the p2m: start_gfn and end_gfn is the range
> + * in the guest physical address space to map, starting from the
> + * machine frame number mfn. */
> +int map_mmio_regions(struct domain *d,
> +                     unsigned long start_gfn,
> +                     unsigned long end_gfn,
> +                     unsigned long mfn);
> +int unmap_mmio_regions(struct domain *d,
> +                       unsigned long start_gfn,
> +                       unsigned long end_gfn,
> +                       unsigned long mfn);
> +
> +#endif /* _XEN_P2M_COMMON_H */
> -- 
> 1.9.0
> 

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.