Re: [PATCH v2 1/3] x86/mem_access: Make the mem_access ops generic

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

On 14/04/14 23:02, Aravindh Puthiyaparambil wrote:
> This patch does the following:
> 1. Deprecate the HVMOP_[sg]et_mem_access HVM ops.
> 2. Move the ops under XENMEM_access_opi.
> 3. Rename enums and structs to be more generic rather than HVM specific.
> 4. Remove the enums and structs associated with the HVM ops.
>
> Signed-off-by: Aravindh Puthiyaparambil <aravindp@cisco.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Tim Deegan <tim@xen.org>
>
> ---
> Changes from version 1 of the patch:
> 1. Use MEMOP_CMD_MASK instead of introducing a new mask.
> 2. Pass "cmd" down from do_memory_op() instead of "op" and "start_extent".
> 3. Pass typed handle to mem_access_memop() and use __copy_field_to_guest().
> 4. Use ACCESS() macro to remove ordering dependency.
> 5. Add compat verification for xen_mem_access_op_t.
> 6. Fix formatting.
>
> Changes from the RFC version of the patch:
> 1. Removed pointless braces.
> 2. Change preemption handling to use upper "cmd" bits from
> do_memory_op().
> 3. Delete old interface enum and structs.
> 4. Remove xenmem_ prefix.
> 5. Make access uint8_t and place above domid.
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 38c491e..eeaa72e 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4589,79 +4589,10 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>      }
>  
>      case HVMOP_set_mem_access:
> -    {
> -        struct xen_hvm_set_mem_access a;
> -        struct domain *d;
> -
> -        if ( copy_from_guest(&a, arg, 1) )
> -            return -EFAULT;
> -
> -        rc = rcu_lock_remote_domain_by_id(a.domid, &d);
> -        if ( rc != 0 )
> -            return rc;
> -
> -        rc = -EINVAL;
> -        if ( !is_hvm_domain(d) )
> -            goto param_fail5;
> -
> -        rc = xsm_hvm_param(XSM_TARGET, d, op);
> -        if ( rc )
> -            goto param_fail5;
> -
> -        rc = -EINVAL;
> -        if ( (a.first_pfn != ~0ull) &&
> -             (a.nr < start_iter ||
> -              ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
> -              ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) )
> -            goto param_fail5;
> -            
> -        rc = p2m_set_mem_access(d, a.first_pfn, a.nr, start_iter,
> -                                HVMOP_op_mask, a.hvmmem_access);
> -        if ( rc > 0 )
> -        {
> -            start_iter = rc;
> -            rc = -EAGAIN;
> -        }
> -
> -    param_fail5:
> -        rcu_unlock_domain(d);
> -        break;
> -    }
> -
>      case HVMOP_get_mem_access:
>      {
> -        struct xen_hvm_get_mem_access a;
> -        struct domain *d;
> -        hvmmem_access_t access;
> -
> -        if ( copy_from_guest(&a, arg, 1) )
> -            return -EFAULT;
> -
> -        rc = rcu_lock_remote_domain_by_id(a.domid, &d);
> -        if ( rc != 0 )
> -            return rc;
> -
> -        rc = -EINVAL;
> -        if ( !is_hvm_domain(d) )
> -            goto param_fail6;
> -
> -        rc = xsm_hvm_param(XSM_TARGET, d, op);
> -        if ( rc )
> -            goto param_fail6;
> -
> -        rc = -EINVAL;
> -        if ( (a.pfn > domain_get_maximum_gpfn(d)) && a.pfn != ~0ull )
> -            goto param_fail6;
> -
> -        rc = p2m_get_mem_access(d, a.pfn, &access);
> -        if ( rc != 0 )
> -            goto param_fail6;
> -
> -        a.hvmmem_access = access;
> -        rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> -
> -    param_fail6:
> -        rcu_unlock_domain(d);
> +        gdprintk(XENLOG_DEBUG, "Deprecated HVM op %ld.\n", op);

Is this really sensible?  I suppose it should hopefully be a rare
hypercall, but it is hardly a useful message to print.

>  
>  /* 
>   * Internal functions, only called by other p2m code
> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> index 3204ec4..f00f6d2 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h

Changes here need to be in sync with libxc as well, also for build reasons.

~Andrew

> @@ -162,49 +162,11 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_xentrace_t);
>  /* Following tools-only interfaces may change in future. */
>  #if defined(__XEN__) || defined(__XEN_TOOLS__)
>  
> +/* Deprecated by XENMEM_access_op_set_access */
>  #define HVMOP_set_mem_access        12
> -typedef enum {
> -    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,       /* Page starts off as r-x, but automatically
> -                                * change to r-w on a write */
> -    HVMMEM_access_n2rwx,       /* Log access: starts off as n, automatically 
> -                                * goes to rwx, generating an event without
> -                                * pausing the vcpu */
> -    HVMMEM_access_default      /* Take the domain default */
> -} hvmmem_access_t;
> -/* Notify that a region of memory is to have specific access types */
> -struct xen_hvm_set_mem_access {
> -    /* Domain to be updated. */
> -    domid_t domid;
> -    /* Memory type */
> -    uint16_t hvmmem_access; /* hvm_access_t */
> -    /* Number of pages, ignored on setting default access */
> -    uint32_t nr;
> -    /* First pfn, or ~0ull to set the default access for new pages */
> -    uint64_aligned_t first_pfn;
> -};
> -typedef struct xen_hvm_set_mem_access xen_hvm_set_mem_access_t;
> -DEFINE_XEN_GUEST_HANDLE(xen_hvm_set_mem_access_t);
>  
> +/* Deprecated by XENMEM_access_op_get_access */
>  #define HVMOP_get_mem_access        13
> -/* Get the specific access type for that region of memory */
> -struct xen_hvm_get_mem_access {
> -    /* Domain to be queried. */
> -    domid_t domid;
> -    /* Memory type: OUT */
> -    uint16_t hvmmem_access; /* hvm_access_t */
> -    /* pfn, or ~0ull for default access for new pages.  IN */
> -    uint64_aligned_t pfn;
> -};
> -typedef struct xen_hvm_get_mem_access xen_hvm_get_mem_access_t;
> -DEFINE_XEN_GUEST_HANDLE(xen_hvm_get_mem_access_t);
>  
>  #define HVMOP_inject_trap            14
>  /* Inject a trap into a VCPU, which will get taken up on the next
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index f19ac14..5bcd475 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -363,9 +363,6 @@ typedef struct xen_pod_target xen_pod_target_t;
>  #define XENMEM_paging_op_evict              1
>  #define XENMEM_paging_op_prep               2
>  
> -#define XENMEM_access_op                    21
> -#define XENMEM_access_op_resume             0
> -
>  struct xen_mem_event_op {
>      uint8_t     op;         /* XENMEM_*_op_* */
>      domid_t     domain;
> @@ -379,6 +376,56 @@ struct xen_mem_event_op {
>  typedef struct xen_mem_event_op xen_mem_event_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_mem_event_op_t);
>  
> +#define XENMEM_access_op                    21
> +#define XENMEM_access_op_resume             0
> +#define XENMEM_access_op_set_access         1
> +#define XENMEM_access_op_get_access         2
> +
> +typedef enum {
> +    XENMEM_access_n,
> +    XENMEM_access_r,
> +    XENMEM_access_w,
> +    XENMEM_access_rw,
> +    XENMEM_access_x,
> +    XENMEM_access_rx,
> +    XENMEM_access_wx,
> +    XENMEM_access_rwx,
> +    /*
> +     * Page starts off as r-x, but automatically
> +     * change to r-w on a write
> +     */
> +    XENMEM_access_rx2rw,
> +    /*
> +     * Log access: starts off as n, automatically
> +     * goes to rwx, generating an event without
> +     * pausing the vcpu
> +     */
> +    XENMEM_access_n2rwx,
> +    /* Take the domain default */
> +    XENMEM_access_default
> +} xenmem_access_t;
> +
> +struct xen_mem_access_op {
> +    /* XENMEM_access_op_* */
> +    uint8_t op;
> +    /* xenmem_access_t */
> +    uint8_t access;
> +    domid_t domid;
> +    /*
> +     * Number of pages for set op
> +     * Ignored on setting default access and other ops
> +     */
> +    uint32_t nr;
> +    /*
> +     * First pfn for set op
> +     * pfn for get op
> +     * ~0ull is used to set and get the default access for pages
> +     */
> +    uint64_aligned_t pfn;
> +};
> +typedef struct xen_mem_access_op xen_mem_access_op_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_mem_access_op_t);
> +
>  #define XENMEM_sharing_op                   22
>  #define XENMEM_sharing_op_nominate_gfn      0
>  #define XENMEM_sharing_op_nominate_gref     1
> diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
> index 5d354d8..9a35dd7 100644
> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -60,6 +60,7 @@
>  !	memory_exchange			memory.h
>  !	memory_map			memory.h
>  !	memory_reservation		memory.h
> +?	mem_access_op		memory.h
>  !	pod_target			memory.h
>  !	remove_from_physmap		memory.h
>  ?	physdev_eoi			physdev.h