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

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

On Wed, Mar 30, 2016 at 06:08:13PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
> 
> 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. Also add a helper to combine the
> xsm_add_to_physmap and XENMAPSPACE_dev_mmio space check together.
> 
> 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>
> ---
> v8: add a helper to combine xsm_add_to_physmap and XENMAPSPACE_dev_mmio
> space check together

I was by accident reviewing the earlier ersion. So let me give comments
here as well.
.. snipp.
> +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",

Should this be printk ratelimited?
> +               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..644f81a 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -834,6 +834,19 @@ static int get_reserved_device_memory(xen_pfn_t start, 
> xen_ulong_t nr,
>  }
>  #endif
>  
> +static long xatp_permission_check(struct domain *d, unsigned int space)
> +{
> +    /*
> +     * XENMAPSPACE_dev_mmio mapping is only supported for hardware Domain

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

Why not -EPERM?

> +
> +    return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
> +}
> +
>  long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
>      struct domain *d;
> @@ -980,7 +993,7 @@ long do_memory_op(unsigned long cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( d == NULL )
>              return -ESRCH;
>  
> -        rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d);
> +        rc = xatp_permission_check(d, xatp.space);

Ah, which nicely takes care of rcu_unlock_domain.
>          if ( rc )
>          {
>              rcu_unlock_domain(d);
> @@ -1024,7 +1037,7 @@ long do_memory_op(unsigned long cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( d == NULL )
>              return -ESRCH;
>  
> -        rc = xsm_add_to_physmap(XSM_TARGET, current->domain, d);
> +        rc = xatp_permission_check(d, xatpb.space);
>          if ( rc )
>          {
>              rcu_unlock_domain(d);
> 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 */

s/device/Device/ And maybe an full stop at the end?

>  /* ` } */
>  
>  /*
> -- 
> 2.0.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