Re: [Xen-devel] [PATCH v7 21/22] xen/arm: Add a hypercall for device mmio mapping

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Fri, Mar 25, 2016 at 09:48:54PM +0800, Shannon Zhao wrote:
> It needs to map platform or amba device mmio to Dom0 on ARM. But when
> booting with ACPI, it can't get the mmio region in Xen due to lack of
> AML interpreter to parse DSDT table. Therefore, let Dom0 call a
> hypercall to map mmio region when it adds the devices.
> 
> Here we add a new map space like the XEN_DOMCTL_memory_mapping to map
> mmio region for Dom0.
> 
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Keir Fraser <keir@xxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Signed-off-by: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
> ---
>  xen/arch/arm/mm.c           |  3 +++
>  xen/arch/arm/p2m.c          | 22 ++++++++++++++++++++++
>  xen/common/memory.c         | 16 ++++++++++++++++
>  xen/include/asm-arm/p2m.h   |  5 +++++
>  xen/include/public/memory.h |  1 +
>  5 files changed, 47 insertions(+)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 81f9e2e..0aae6c5 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1138,6 +1138,9 @@ int xenmem_add_to_physmap_one(
>          rcu_unlock_domain(od);
>          break;
>      }
> +    case XENMAPSPACE_dev_mmio:
> +        rc = map_dev_mmio_region(d, gpfn, 1, idx);
> +        return rc;
>  
>      default:
>          return -ENOSYS;
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 7e5f5d1..0011708 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -7,6 +7,7 @@
>  #include <xen/bitops.h>
>  #include <xen/vm_event.h>
>  #include <xen/mem_access.h>
> +#include <xen/iocap.h>
>  #include <public/vm_event.h>
>  #include <asm/flushtlb.h>
>  #include <asm/gic.h>
> @@ -1270,6 +1271,27 @@ int unmap_mmio_regions(struct domain *d,
>                               d->arch.p2m.default_access);
>  }
>  
> +int map_dev_mmio_region(struct domain *d,
> +                        unsigned long start_gfn,
> +                        unsigned long nr,
> +                        unsigned long mfn)
> +{
> +    int res;
> +
> +    if ( !(nr && iomem_access_permitted(d, start_gfn, start_gfn + nr - 1)) )
> +        return 0;
> +
> +    res = map_mmio_regions(d, start_gfn, nr, mfn);
> +    if ( res < 0 )
> +    {
> +        printk(XENLOG_ERR "Unable to map [%#lx - %#lx] in Dom%d\n",
> +               start_gfn, start_gfn + nr - 1, d->domain_id);
> +        return res;
> +    }
> +
> +    return 0;
> +}
> +
>  int guest_physmap_add_entry(struct domain *d,
>                              unsigned long gpfn,
>                              unsigned long mfn,
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index c7fca96..25ff86c 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -980,6 +980,14 @@ long do_memory_op(unsigned long cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( d == NULL )
>              return -ESRCH;
>  
> +        /*
> +         * XENMAPSPACE_dev_mmio mapping is only supported for hardware Domain

Any reason to make Domain be uppercase?

> +         * to map this kind of space to itself.
> +         */
> +        if ( (xatp.space == XENMAPSPACE_dev_mmio) &&
> +             ((d != current->domain) || !is_hardware_domain(d)) )
> +            return -EACCES;

Shouldnt' this be -EPERM? 
Also you probably want to do 'rcu_unlock_domain(d)' before you return?

> +
>          rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d);
>          if ( rc )
>          {
> @@ -1024,6 +1032,14 @@ long do_memory_op(unsigned long cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( d == NULL )
>              return -ESRCH;
>  
> +        /*
> +         * XENMAPSPACE_dev_mmio mapping is only supported for hardware Domain

Any reason to make Domain be uppercase?

> +         * to map this kind of space to itself.
> +         */
> +        if ( (xatpb.space == XENMAPSPACE_dev_mmio) &&
> +             ((d != current->domain) || !is_hardware_domain(d)) )
> +            return -EACCES;
> +
>          rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d);
>          if ( rc )
>          {
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 55626b4..d240d1e 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -154,6 +154,11 @@ int unmap_regions_rw_cache(struct domain *d,
>                             unsigned long nr_mfns,
>                             unsigned long mfn);
>  
> +int map_dev_mmio_region(struct domain *d,
> +                        unsigned long start_gfn,
> +                        unsigned long nr,
> +                        unsigned long mfn);
> +
>  int guest_physmap_add_entry(struct domain *d,
>                              unsigned long gfn,
>                              unsigned long mfn,
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index f69e92f..fe52ee1 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -220,6 +220,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_mapping_t);
>  #define XENMAPSPACE_gmfn_range   3 /* GMFN range, XENMEM_add_to_physmap 
> only. */
>  #define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another dom,
>                                      * XENMEM_add_to_physmap_batch only. */
> +#define XENMAPSPACE_dev_mmio     5 /* device mmio region */

You probably want to mention what the return values can be? Also why
not make 'device' be 'Device'.

>  /* ` } */
>  
>  /*
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

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