Re: [PATCH RESEND v9 09/14] xen/common: move the memory_mapping DOMCTL hypercall to common code

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

Hi Arianna,

On 07/14/2014 01:50 AM, Arianna Avanzini wrote:
> This commit moves to common code the implementation of the memory_mapping
> DOMCTL, currently available only for the x86 architecture. It also adds
> a definition for the PADDR_BITS constant for ARM, that is to be used in
> common code and currently not available for the ARM architecture.
> 
> Signed-off-by: Arianna Avanzini <avanzini.arianna@gmail.com>
> Acked-by: Jan Beulich <JBeulich@suse.com>

Acked-by: Julien Grall <julien.grall@linaro.org>
Tested-by: Julien Grall <julien.grall@linaro.org>

Regards,

> 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: Keir Fraser <keir@xen.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Eric Trudeau <etrudeau@broadcom.com>
> Cc: Viktor Kleinik <viktor.kleinik@globallogic.com>
> 
> ---
> 
>     v9:
>         - Don't expose the type mfn_t to common code; let the x86-specific map/
>           unmap functions perform the type cast and use unsigned long in common
>           code.
> 
>     v7:
>         - Change the name of tmp_rc to rc. Also, do not uselessly initialize it.
>         - ifdef out the invocation of memory_type_changed() to be called only
>           if the architecture is x86 instead of adding an useless empty stub
>           for ARM.
> 
>     v6:
>         - Add an empty definition of the memory_type_changed() function for ARM.
> 
>     v5:
>         - Rename new header to p2m-common.h.
> 
>     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.
> 
>     v3:
>         - Add a paddr_bits variable for ARM.
> 
>     v2:
>         - Move code to xen/arm/domctl.c.
> 
> ---
>  xen/arch/arm/p2m.c           |  8 +++---
>  xen/arch/x86/domctl.c        | 68 --------------------------------------------
>  xen/common/domctl.c          | 68 ++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/p2m.h    | 15 +++-------
>  xen/include/asm-x86/p2m.h    | 13 +--------
>  xen/include/xen/p2m-common.h | 16 +++++++++++
>  6 files changed, 93 insertions(+), 95 deletions(-)
>  create mode 100644 xen/include/xen/p2m-common.h
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index a85e58d..57e36b2 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -862,24 +862,24 @@ int p2m_populate_ram(struct domain *d,
>  
>  int map_mmio_regions(struct domain *d,
>                       unsigned long start_gfn,
> -                     unsigned long nr_mfns,
> +                     unsigned long nr,
>                       unsigned long mfn)
>  {
>      return apply_p2m_changes(d, INSERT,
>                               pfn_to_paddr(start_gfn),
> -                             pfn_to_paddr(start_gfn + nr_mfns),
> +                             pfn_to_paddr(start_gfn + nr),
>                               pfn_to_paddr(mfn),
>                               MATTR_DEV, p2m_mmio_direct);
>  }
>  
>  int unmap_mmio_regions(struct domain *d,
>                         unsigned long start_gfn,
> -                       unsigned long nr_mfns,
> +                       unsigned long nr,
>                         unsigned long mfn)
>  {
>      return apply_p2m_changes(d, REMOVE,
>                               pfn_to_paddr(start_gfn),
> -                             pfn_to_paddr(start_gfn + nr_mfns),
> +                             pfn_to_paddr(start_gfn + nr),
>                               pfn_to_paddr(mfn),
>                               MATTR_DEV, p2m_invalid);
>  }
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 15af792..c211818 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -641,74 +641,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;
> -        unsigned long mfn_end = mfn + nr_mfns - 1;
> -        int add = domctl->u.memory_mapping.add_mapping;
> -
> -        ret = -EINVAL;
> -        if ( mfn_end < mfn || /* wrap? */
> -             ((mfn | mfn_end) >> (paddr_bits - PAGE_SHIFT)) ||
> -             (gfn + nr_mfns - 1) < gfn ) /* wrap? */
> -            break;
> -
> -        ret = -EPERM;
> -        if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
> -            break;
> -
> -        ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, 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_end);
> -            if ( !ret )
> -            {
> -                ret = map_mmio_regions(d, gfn, nr_mfns, mfn);
> -                if ( ret )
> -                {
> -                    printk(XENLOG_G_WARNING
> -                           "memory_map:fail: dom%d gfn=%lx mfn=%lx nr=%lx ret:%ld\n",
> -                           d->domain_id, gfn, mfn, nr_mfns, ret);
> -                    if ( iomem_deny_access(d, mfn, mfn_end) &&
> -                         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);
> -                }
> -            }
> -        }
> -        else
> -        {
> -            int rc = 0;
> -
> -            printk(XENLOG_G_INFO
> -                   "memory_map:remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
> -                   d->domain_id, gfn, mfn, nr_mfns);
> -
> -            rc = unmap_mmio_regions(d, gfn, nr_mfns, mfn);
> -            ret = iomem_deny_access(d, mfn, mfn_end);
> -            if ( !ret )
> -                ret = rc;
> -            if ( ret && is_hardware_domain(current->domain) )
> -                printk(XENLOG_ERR
> -                       "memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
> -                       ret, rc ? "removing" : "denying", d->domain_id,
> -                       mfn, mfn_end);
> -        }
> -        /* Do this unconditionally to cover errors on above failure paths. */
> -        memory_type_changed(d);
> -    }
> -    break;
> -
>      case XEN_DOMCTL_ioport_mapping:
>      {
>          struct hvm_iommu *hd;
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 24102c0..80b7800 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -902,6 +902,74 @@ 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 - 1;
> +        int add = op->u.memory_mapping.add_mapping;
> +
> +        ret = -EINVAL;
> +        if ( mfn_end < mfn || /* wrap? */
> +             ((mfn | mfn_end) >> (paddr_bits - PAGE_SHIFT)) ||
> +             (gfn + nr_mfns - 1) < gfn ) /* wrap? */
> +            break;
> +
> +        ret = -EPERM;
> +        if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
> +            break;
> +
> +        ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, 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_end);
> +            if ( !ret )
> +            {
> +                ret = map_mmio_regions(d, gfn, nr_mfns, mfn);
> +                if ( ret )
> +                {
> +                    printk(XENLOG_G_WARNING
> +                           "memory_map:fail: dom%d gfn=%lx mfn=%lx nr=%lx ret:%ld\n",
> +                           d->domain_id, gfn, mfn, nr_mfns, ret);
> +                    if ( iomem_deny_access(d, mfn, mfn_end) &&
> +                         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);
> +                }
> +            }
> +        }
> +        else
> +        {
> +            int rc = 0;
> +
> +            printk(XENLOG_G_INFO
> +                   "memory_map:remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
> +                   d->domain_id, gfn, mfn, nr_mfns);
> +
> +            rc = unmap_mmio_regions(d, gfn, nr_mfns, mfn);
> +            ret = iomem_deny_access(d, mfn, mfn_end);
> +            if ( !ret )
> +                ret = rc;
> +            if ( ret && is_hardware_domain(current->domain) )
> +                printk(XENLOG_ERR
> +                       "memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
> +                       ret, rc ? "removing" : "denying", d->domain_id,
> +                       mfn, mfn_end);
> +        }
> +        /* Do this unconditionally to cover errors on above failure paths. */
> +        memory_type_changed(d);
> +    }
> +    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 648144f..08ce07b 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -3,6 +3,10 @@
>  
>  #include <xen/mm.h>
>  
> +#include <xen/p2m-common.h>
> +
> +#define paddr_bits PADDR_BITS
> +
>  struct domain;
>  
>  extern void memory_type_changed(struct domain *);
> @@ -101,17 +105,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 nr_mfns describe 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 nr_mfns,
> -                     unsigned long mfn);
> -int unmap_mmio_regions(struct domain *d,
> -                       unsigned long start_gfn,
> -                       unsigned long nr_mfns,
> -                       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 cea46e1..2a65848 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -29,21 +29,10 @@
>  
>  #include <xen/config.h>
>  #include <xen/paging.h>
> +#include <xen/p2m-common.h>
>  #include <asm/mem_sharing.h>
>  #include <asm/page.h>    /* for pagetable_t */
>  
> -/* Map MMIO regions in the p2m: start_gfn and nr describe 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 nr,
> -                     unsigned long mfn);
> -int unmap_mmio_regions(struct domain *d,
> -                       unsigned long start_gfn,
> -                       unsigned long nr,
> -                       unsigned long mfn);
> -
>  extern bool_t opt_hap_1gb, opt_hap_2mb;
>  
>  /*
> diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
> new file mode 100644
> index 0000000..9f1b771
> --- /dev/null
> +++ b/xen/include/xen/p2m-common.h
> @@ -0,0 +1,16 @@
> +#ifndef _XEN_P2M_COMMON_H
> +#define _XEN_P2M_COMMON_H
> +
> +/* Map MMIO regions in the p2m: start_gfn and nr describe 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 nr,
> +                     unsigned long mfn);
> +int unmap_mmio_regions(struct domain *d,
> +                       unsigned long start_gfn,
> +                       unsigned long nr,
> +                       unsigned long mfn);
> +
> +#endif /* _XEN_P2M_COMMON_H */
> 


-- 
Julien Grall