Re: [PATCH V5 10/12] xen/vm_event: Relocate memop checks

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 13/02/15 16:33, Tamas K Lengyel wrote:
> The memop handler function for paging/sharing responsible for calling XSM
> doesn't really have anything to do with vm_event, thus in this patch we
> relocate it into mem_paging_memop and mem_sharing_memop. This has already
> been the approach in mem_access_memop, so in this patch we just make it
> consistent.
>
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
> ---
>  xen/arch/x86/mm/mem_paging.c      | 36 ++++++++++++++-----
>  xen/arch/x86/mm/mem_sharing.c     | 76 ++++++++++++++++++++++++++-------------
>  xen/arch/x86/x86_64/compat/mm.c   | 28 +++------------
>  xen/arch/x86/x86_64/mm.c          | 26 +++-----------
>  xen/common/vm_event.c             | 43 ----------------------
>  xen/include/asm-x86/mem_paging.h  |  7 +++-
>  xen/include/asm-x86/mem_sharing.h |  4 +--
>  xen/include/xen/vm_event.h        |  1 -
>  8 files changed, 97 insertions(+), 124 deletions(-)
>
> diff --git a/xen/arch/x86/mm/mem_paging.c b/xen/arch/x86/mm/mem_paging.c
> index e63d8c1..4aee6b7 100644
> --- a/xen/arch/x86/mm/mem_paging.c
> +++ b/xen/arch/x86/mm/mem_paging.c
> @@ -21,28 +21,45 @@
>   */
>  
>  
> +#include <xen/guest_access.h>
>  #include <asm/p2m.h>
> -#include <xen/vm_event.h>
> +#include <xsm/xsm.h>

Order of includes.

>  
> -
> -int mem_paging_memop(struct domain *d, xen_mem_paging_op_t *mpo)
> +int mem_paging_memop(unsigned long cmd,

I don't believe cmd is a useful parameter to pass.  You know that its
value is XENMEM_paging_op by virtue of being in this function.

> +                     XEN_GUEST_HANDLE_PARAM(xen_mem_paging_op_t) arg)
>  {
> -    int rc = -ENODEV;
> +    int rc;
> +    xen_mem_paging_op_t mpo;
> +    struct domain *d;
> +
> +    rc = -EFAULT;
> +    if ( copy_from_guest(&mpo, arg, 1) )
> +        return rc;
> +
> +    rc = rcu_lock_live_remote_domain_by_id(mpo.domain, &d);
> +    if ( rc )
> +        return rc;
> +
> +    rc = xsm_vm_event_op(XSM_DM_PRIV, d, XENMEM_paging_op);
> +    if ( rc )
> +        return rc;
> +
> +    rc = -ENODEV;
>      if ( unlikely(!d->vm_event->paging.ring_page) )
>          return rc;
>  
> -    switch( mpo->op )
> +    switch( mpo.op )
>      {
>      case XENMEM_paging_op_nominate:
> -        rc = p2m_mem_paging_nominate(d, mpo->gfn);
> +        rc = p2m_mem_paging_nominate(d, mpo.gfn);
>          break;
>  
>      case XENMEM_paging_op_evict:
> -        rc = p2m_mem_paging_evict(d, mpo->gfn);
> +        rc = p2m_mem_paging_evict(d, mpo.gfn);
>          break;
>  
>      case XENMEM_paging_op_prep:
> -        rc = p2m_mem_paging_prep(d, mpo->gfn, mpo->buffer);
> +        rc = p2m_mem_paging_prep(d, mpo.gfn, mpo.buffer);
>          break;
>  
>      default:
> @@ -50,6 +67,9 @@ int mem_paging_memop(struct domain *d, xen_mem_paging_op_t *mpo)
>          break;
>      }
>  
> +    if ( !rc && __copy_to_guest(arg, &mpo, 1) )
> +        rc = -EFAULT;

Do any of the paging ops need to be copied back?  Nothing appears to
write into mpo in this function.  (The original code looks to be overly
pessimistic).

> +
>      return rc;
>  }
>  
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index 4959407..612ed89 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -28,6 +28,7 @@
>  #include <xen/grant_table.h>
>  #include <xen/sched.h>
>  #include <xen/rcupdate.h>
> +#include <xen/guest_access.h>
>  #include <xen/vm_event.h>
>  #include <asm/page.h>
>  #include <asm/string.h>
> @@ -1293,30 +1294,52 @@ int relinquish_shared_pages(struct domain *d)
>      return rc;
>  }
>  
> -int mem_sharing_memop(struct domain *d, xen_mem_sharing_op_t *mec)
> +int mem_sharing_memop(unsigned long cmd,
> +                      XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
>  {
> -    int rc = 0;
> +    int rc;
> +    xen_mem_sharing_op_t mso;
> +    struct domain *d;
> +
> +    rc = -EFAULT;
> +    if ( copy_from_guest(&mso, arg, 1) )
> +        return rc;
> +
> +    if ( mso.op == XENMEM_sharing_op_audit )
> +        return mem_sharing_audit();
> +
> +    rc = rcu_lock_live_remote_domain_by_id(mso.domain, &d);
> +    if ( rc )
> +        return rc;
>  
>      /* Only HAP is supported */
>      if ( !hap_enabled(d) || !d->arch.hvm_domain.mem_sharing_enabled )
>           return -ENODEV;
>  
> -    switch(mec->op)
> +    rc = xsm_vm_event_op(XSM_DM_PRIV, d, XENMEM_sharing_op);
> +    if ( rc )
> +        return rc;
> +
> +    rc = -ENODEV;
> +    if ( unlikely(!d->vm_event->share.ring_page) )
> +        return rc;
> +
> +    switch(mso.op)

Style ( spaces )

> @@ -1465,6 +1488,9 @@ int mem_sharing_memop(struct domain *d, xen_mem_sharing_op_t *mec)
>              break;
>      }
>  
> +    if ( !rc && __copy_to_guest(arg, &mso, 1) )
> +        return -EFAULT;

Only certain subops need to copy information back.  It is common to have
a function-level bool_t copyback which relevant subops sets.

~Andrew

> +
>      return rc;
>  }