Re: [Xen-devel] [PATCH 03/17] PVH xen: create domctl_memory_mapping() function

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.04.13 at 23:25, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -46,6 +46,72 @@ 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)

bool_t.

> +{
> +    unsigned long i;
> +    long ret;
> +
> +    if ( !IS_PRIV(current->domain)  &&
> +         !iomem_access_permitted(current->domain, mfn, mfn + nr_mfns - 1) )
> +        return -EPERM;

This construct is stale as of 76401237 ("x86: remove IS_PRIV access
check bypasses"). Oh, I just saw that you say this series is based on
an almost week old tree...

> +
> +    if ( (mfn + nr_mfns - 1) < mfn || /* wrap? */
> +         ((mfn | (mfn + nr_mfns - 1)) >> (paddr_bits - PAGE_SHIFT)) ||
> +         (gfn + nr_mfns - 1) < gfn ) /* wrap? */
> +        return -EINVAL;
> +
> +    ret = xsm_iomem_permission(XSM_HOOK, d, mfn, mfn + nr_mfns - 1, add_map);
> +    if ( ret )
> +        return ret;
> +
> +    if ( add_map )
> +    {
> +        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 {

How shall we trust this is pure code movement if even formatting
got broken?

> +        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_map |= !clear_mmio_p2m_entry(d, gfn + i);
> +        ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
> +        if ( !ret && add_map )
> +            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_map ? "removing" : "denying", d->domain_id,
> +                   mfn, mfn + nr_mfns - 1);
> +    }
> +    return ret;
> +}
> +
>  long arch_do_domctl(
>      struct xen_domctl *domctl, struct domain *d,
>      XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> @@ -628,68 +694,8 @@ 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;
> -
> -        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
> -        {

See the proper original code here.

Jan

> -            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;
>  


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