Re: [Xen-devel] [PATCH 1/3] x86/mem_access: Make the mem_access ops generic

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.04.14 at 05:49, <aravindp@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -21,31 +21,98 @@
>   */
>  
>  
> +#include <xen/sched.h>
> +#include <xen/guest_access.h>
> +#include <xen/hypercall.h>
>  #include <asm/p2m.h>
>  #include <asm/mem_event.h>
> +#include <xsm/xsm.h>
>  
>  
> -int mem_access_memop(struct domain *d, xen_mem_event_op_t *meo)
> +#define ACCESS_op_mask  0xff

I think this needs to be MEMOP_CMD_MASK (and then you wouldn't
need a new #define anyway). Or at least you need to put a
BUILD_BUG_ON() somewhere making sure this mask is at least as
wide as MEMOP_CMD_MASK.

> +
> +int mem_access_memop(XEN_GUEST_HANDLE_PARAM(void) arg, unsigned long 
> start_iter)
>  {
> -    int rc;
> +    long rc;
> +    xen_mem_access_op_t mao;
> +    struct domain *d;
> +
> +    if ( copy_from_guest(&mao, arg, 1) )
> +        return -EFAULT;
> +
> +    rc = rcu_lock_live_remote_domain_by_id(mao.domid, &d);
> +    if ( rc )
> +        return rc;
> +
> +    if ( !is_hvm_domain(d) )
> +        return -EINVAL;
> +
> +    rc = xsm_mem_event_op(XSM_TARGET, d, XENMEM_access_op);
> +    if ( rc )
> +        goto out;
>  
>      if ( unlikely(!d->mem_event->access.ring_page) )
>          return -ENODEV;
>  
> -    switch( meo->op )
> +    switch ( mao.op )
>      {
>      case XENMEM_access_op_resume:
>      {
>          p2m_mem_access_resume(d);
>          rc = 0;
> +        break;
> +    }
> +
> +    case XENMEM_access_op_set_access:
> +        rc = -EINVAL;
> +
> +        /*
> +         * Undo adjustment done in do_memory_op() so that it matches
> +         * p2m_set_mem_access()
> +         */
> +        start_iter <<= MEMOP_EXTENT_SHIFT;

This is why perhaps it would be better to pass "cmd" down instead of
"op" and "start_extent".

> +        if ( (mao.pfn != ~0ull) &&
> +             (mao.nr < start_iter ||
> +              ((mao.pfn + mao.nr - 1) < mao.pfn) ||
> +              ((mao.pfn + mao.nr - 1) > domain_get_maximum_gpfn(d))) )
> +            break;
> +
> +        rc = p2m_set_mem_access(d, mao.pfn, mao.nr, start_iter,
> +                                ACCESS_op_mask, mao.access);
> +        if ( rc > 0 )
> +        {
> +            ASSERT(!(rc & ACCESS_op_mask));
> +            rc = hypercall_create_continuation(__HYPERVISOR_memory_op, "lh",
> +                                               XENMEM_access_op | rc,
> +                                               arg);
> +        }
> +        break;
> +
> +    case XENMEM_access_op_get_access:
> +    {
> +        xenmem_access_t access;
> +
> +        rc = -EINVAL;
> +        if ( (mao.pfn > domain_get_maximum_gpfn(d)) && mao.pfn != ~0ull )
> +            break;
> +
> +        rc = p2m_get_mem_access(d, mao.pfn, &access);
> +        if ( rc != 0 )
> +            break;
> +
> +        mao.access = access;
> +        rc = __copy_to_guest(arg, &mao, 1) ? -EFAULT : 0;

If you passed a properly typed handle into this function (which is
possible because it wants only one kind of interface structure) you
would get away with the cheaper __copy_field_to_guest() here.

> @@ -1359,7 +1359,7 @@ long p2m_set_mem_access(struct domain *d, unsigned long 
> pfn, uint32_t nr,
>      long rc = 0;
>  
>      static const p2m_access_t memaccess[] = {
> -#define ACCESS(ac) [HVMMEM_access_##ac] = p2m_access_##ac
> +#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
>          ACCESS(n),
>          ACCESS(r),
>          ACCESS(w),
> @@ -1416,23 +1416,23 @@ long p2m_set_mem_access(struct domain *d, unsigned 
> long pfn, uint32_t nr,
>  /* Get access type for a pfn
>   * If pfn == -1ul, gets the default access type */
>  int p2m_get_mem_access(struct domain *d, unsigned long pfn, 
> -                       hvmmem_access_t *access)
> +                       xenmem_access_t *access)
>  {
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>      p2m_type_t t;
>      p2m_access_t a;
>      mfn_t mfn;
>  
> -    static const hvmmem_access_t memaccess[] = {
> -        HVMMEM_access_n,
> -        HVMMEM_access_r,
> -        HVMMEM_access_w,
> -        HVMMEM_access_rw,
> -        HVMMEM_access_x,
> -        HVMMEM_access_rx,
> -        HVMMEM_access_wx,
> -        HVMMEM_access_rwx,
> -        HVMMEM_access_rx2rw
> +    static const xenmem_access_t memaccess[] = {
> +        XENMEM_access_n,
> +        XENMEM_access_r,
> +        XENMEM_access_w,
> +        XENMEM_access_rw,
> +        XENMEM_access_x,
> +        XENMEM_access_rx,
> +        XENMEM_access_wx,
> +        XENMEM_access_rwx,
> +        XENMEM_access_rx2rw

Please use an ACCESS() macro similar to that added by commit
ca0f2184 (also seen in the quoted hunk above this one) if you
already need to touch this, to remove the ordering dependency.

> @@ -195,6 +196,10 @@ int compat_arch_memory_op(int op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>              return -EFAULT;
>          break;
>      }
> +    case XENMEM_access_op:
> +        rc = mem_access_memop(arg, start_extent);
> +        break;
> +

Blank line also above this block please. Also you'll need to add
verification that the compat and native structures' layouts match
(i.e. to prove that you're allowed to pass the argument here
without translation). Looks like this is also missing for the already
existing xen_mem_event_op_t and xen_mem_sharing_op_t.

> @@ -1017,6 +1018,10 @@ long subarch_memory_op(int op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>              return -EFAULT;
>          break;
>      }
> +    case XENMEM_access_op:
> +        rc = mem_access_memop(arg, start_extent);
> +        break;
> +

Again blank line above this block too please.

Jan

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