Re: [Xen-devel] [PATCH 3/18 V2]: PVH xen: create domctl_memory_mapping() function

[Tim Deegan <tim@xxxxxxx>]

At 17:26 -0700 on 15 Mar (1363368383), Mukesh Rathor wrote:
> In this patch, XEN_DOMCTL_memory_mapping code is put into a function so
> it can be shared later for PVH. There is no change in it's
> functionality.
> 
> Changes in V2:
>   - Remove PHYSDEVOP_map_iomem sub hypercall, and the code supporting it
>     as the IO region is mapped transparently now.
> 
> Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> ---
>  xen/arch/x86/domctl.c    |  119 ++++++++++++++++++++++++---------------------
>  xen/include/xen/domain.h |    2 +
>  2 files changed, 65 insertions(+), 56 deletions(-)
> 
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 31937e0..ecc8240 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -46,6 +46,68 @@ static int gdbsx_guest_mem_io(
>      return (iop->remain ? -EFAULT : 0);
>  }
>  
> +long domctl_memory_mapping(struct domain *d, unsigned long gfn,
> +                           unsigned long mfn, unsigned long nr_mfns,
> +                           int add_map)
> +{
> +    int i;

The loop variable at the source was 'unsigned long', and the loop limit
is an unsigned long, so 'int' doesn't seem right. 

> @@ -628,68 +690,13 @@ long arch_do_domctl(
>          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 ( !IS_PRIV(current->domain) &&
>               !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 
> 1) )
>              break;

Why is this privilege check not being moved along with the rest of it?
Execially given that the two other IS_PRIV()s below are taken?

Tim.

> -        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_PRIV(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_PRIV(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);
> -        }
> +        ret = domctl_memory_mapping(d, gfn, mfn, nr_mfns, add);
>      }
>      break;
>  
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index d4ac50f..a7b4c34 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -86,4 +86,6 @@ extern unsigned int xen_processor_pmbits;
>  
>  extern bool_t opt_dom0_vcpus_pin;
>  
> +extern long domctl_memory_mapping(struct domain *d, unsigned long gfn,
> +                    unsigned long mfn, unsigned long nr_mfns, int add_map);
>  #endif /* __XEN_DOMAIN_H__ */
> -- 
> 1.7.2.3
> 
> 
> _______________________________________________
> 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