[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v4] x86/altp2m: Add a subop for obtaining the mem access of a page



On Mon, Sep 3, 2018 at 9:47 AM Adrian Pop <apop@xxxxxxxxxxxxxxx> wrote:
>
> Currently there is a subop for setting the memaccess of a page, but not
> for consulting it.  The new HVMOP_altp2m_get_mem_access adds this
> functionality.
>
> Both altp2m get/set mem access functions use the struct
> xen_hvm_altp2m_mem_access which has now dropped the `set' part and has
> been renamed from xen_hvm_altp2m_set_mem_access.
>
> Signed-off-by: Adrian Pop <apop@xxxxxxxxxxxxxxx>
> ---
> Changes in v4:
> - don't break the stable interface
>
> Changes in v3:
> - remove the unrelated helper function
> - simplify the locking in p2m_get_mem_access
>
> Changes in v2:
> - use the _p2m_get_mem_access helper from p2m_get_mem_access
> - minor Arm adjustments
> - move out the addition of a memaccess helper function to a separate
>   patch in the attempts of making the diff clearer
> ---
>  tools/libxc/include/xenctrl.h   |  3 +++
>  tools/libxc/xc_altp2m.c         | 33 +++++++++++++++++++++++++++++++++
>  xen/arch/arm/mem_access.c       |  8 ++++++--
>  xen/arch/x86/hvm/hvm.c          | 27 +++++++++++++++++++++++++++
>  xen/arch/x86/mm/mem_access.c    | 21 +++++++++++++++++++--
>  xen/common/mem_access.c         |  2 +-
>  xen/include/public/hvm/hvm_op.h | 19 +++++++++++++++++++
>  xen/include/public/xen-compat.h |  2 +-
>  xen/include/xen/mem_access.h    |  3 ++-
>  9 files changed, 111 insertions(+), 7 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index c626984aba..ae298899fc 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1958,6 +1958,9 @@ int xc_altp2m_set_mem_access(xc_interface *handle, 
> uint32_t domid,
>  int xc_altp2m_set_mem_access_multi(xc_interface *handle, uint32_t domid,
>                                     uint16_t view_id, uint8_t *access,
>                                     uint64_t *gfns, uint32_t nr);
> +int xc_altp2m_get_mem_access(xc_interface *handle, uint32_t domid,
> +                             uint16_t view_id, xen_pfn_t gfn,
> +                             xenmem_access_t *access);
>  int xc_altp2m_change_gfn(xc_interface *handle, uint32_t domid,
>                           uint16_t view_id, xen_pfn_t old_gfn,
>                           xen_pfn_t new_gfn);
> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
> index ce4a1e4d60..53754ff6d3 100644
> --- a/tools/libxc/xc_altp2m.c
> +++ b/tools/libxc/xc_altp2m.c
> @@ -177,9 +177,15 @@ int xc_altp2m_set_mem_access(xc_interface *handle, 
> uint32_t domid,
>      arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
>      arg->cmd = HVMOP_altp2m_set_mem_access;
>      arg->domain = domid;
> +#if __XEN_INTERFACE_VERSION__ < 0x00040a00
>      arg->u.set_mem_access.view = view_id;
>      arg->u.set_mem_access.hvmmem_access = access;
>      arg->u.set_mem_access.gfn = gfn;
> +#else /* __XEN_INTERFACE_VERSION__ >= 0x00040a00 */
> +    arg->u.mem_access.view = view_id;
> +    arg->u.mem_access.hvmmem_access = access;
> +    arg->u.mem_access.gfn = gfn;
> +#endif
>
>      rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
>                   HYPERCALL_BUFFER_AS_ARG(arg));
> @@ -254,3 +260,30 @@ int xc_altp2m_set_mem_access_multi(xc_interface *xch, 
> uint32_t domid,
>
>      return rc;
>  }
> +
> +int xc_altp2m_get_mem_access(xc_interface *handle, uint32_t domid,
> +                             uint16_t view_id, xen_pfn_t gfn,
> +                             xenmem_access_t *access)
> +{
> +    int rc;
> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
> +
> +    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
> +    if ( arg == NULL )
> +        return -1;
> +
> +    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
> +    arg->cmd = HVMOP_altp2m_get_mem_access;
> +    arg->domain = domid;
> +    arg->u.mem_access.view = view_id;
> +    arg->u.mem_access.gfn = gfn;
> +
> +    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
> +                 HYPERCALL_BUFFER_AS_ARG(arg));
> +
> +    if ( !rc )
> +        *access = arg->u.mem_access.hvmmem_access;
> +
> +    xc_hypercall_buffer_free(handle, arg);
> +    return rc;
> +}
> diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
> index ba4ec780fd..178bc1a6c1 100644
> --- a/xen/arch/arm/mem_access.c
> +++ b/xen/arch/arm/mem_access.c
> @@ -236,7 +236,7 @@ bool p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const 
> struct npfec npfec)
>      if ( !p2m->mem_access_enabled )
>          return true;
>
> -    rc = p2m_get_mem_access(v->domain, gaddr_to_gfn(gpa), &xma);
> +    rc = p2m_get_mem_access(v->domain, gaddr_to_gfn(gpa), &xma, 0);
>      if ( rc )
>          return true;
>
> @@ -441,11 +441,15 @@ long p2m_set_mem_access_multi(struct domain *d,
>  }
>
>  int p2m_get_mem_access(struct domain *d, gfn_t gfn,
> -                       xenmem_access_t *access)
> +                       xenmem_access_t *access, unsigned int altp2m_idx)
>  {
>      int ret;
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>
> +    /* altp2m is not yet implemented on Arm. The altp2m_idx should be 0. */

Just use ASSERT here.

> +    if ( altp2m_idx )
> +        return -EINVAL;
> +
>      p2m_read_lock(p2m);
>      ret = __p2m_get_mem_access(d, gfn, access);
>      p2m_read_unlock(p2m);
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 72c51faecb..460c9f7d4f 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4526,6 +4526,7 @@ static int do_altp2m_op(
>      case HVMOP_altp2m_switch_p2m:
>      case HVMOP_altp2m_set_mem_access:
>      case HVMOP_altp2m_set_mem_access_multi:
> +    case HVMOP_altp2m_get_mem_access:
>      case HVMOP_altp2m_change_gfn:
>          break;
>
> @@ -4642,12 +4643,21 @@ static int do_altp2m_op(
>          break;
>
>      case HVMOP_altp2m_set_mem_access:
> +#if __XEN_INTERFACE_VERSION__ < 0x00040a00
>          if ( a.u.set_mem_access.pad )
>              rc = -EINVAL;
>          else
>              rc = p2m_set_mem_access(d, _gfn(a.u.set_mem_access.gfn), 1, 0, 0,
>                                      a.u.set_mem_access.hvmmem_access,
>                                      a.u.set_mem_access.view);
> +#else /* __XEN_INTERFACE_VERSION__ >= 0x00040a00 */
> +        if ( a.u.mem_access.pad )
> +            rc = -EINVAL;
> +        else
> +            rc = p2m_set_mem_access(d, _gfn(a.u.mem_access.gfn), 1, 0, 0,
> +                                    a.u.mem_access.hvmmem_access,
> +                                    a.u.mem_access.view);
> +#endif
>          break;
>
>      case HVMOP_altp2m_set_mem_access_multi:
> @@ -4683,6 +4693,23 @@ static int do_altp2m_op(
>          }
>          break;
>
> +    case HVMOP_altp2m_get_mem_access:
> +        if ( a.u.mem_access.pad )
> +            rc = -EINVAL;
> +        else
> +        {
> +            xenmem_access_t access;
> +
> +            rc = p2m_get_mem_access(d, _gfn(a.u.mem_access.gfn), &access,
> +                                    a.u.mem_access.view);
> +            if ( !rc )
> +            {
> +                a.u.mem_access.hvmmem_access = access;
> +                rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> +            }
> +        }
> +        break;
> +
>      case HVMOP_altp2m_change_gfn:
>          if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
>              rc = -EINVAL;
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index 84d260ebd8..78abdaed36 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -469,9 +469,26 @@ long p2m_set_mem_access_multi(struct domain *d,
>      return rc;
>  }
>
> -int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
> +int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access,
> +                       unsigned int altp2m_idx)
>  {
> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *p2m;
> +
> +    if ( !altp2m_active(d) )
> +    {
> +        if ( altp2m_idx )
> +            return -EINVAL;
> +
> +        p2m = p2m_get_hostp2m(d);
> +    }
> +    else
> +    {
> +        if ( altp2m_idx >= MAX_ALTP2M ||
> +             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
> +            return -EINVAL;
> +
> +        p2m = d->arch.altp2m_p2m[altp2m_idx];
> +    }
>
>      return _p2m_get_mem_access(p2m, gfn, access);
>  }
> diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c
> index 1bf6824442..010e6f8dbf 100644
> --- a/xen/common/mem_access.c
> +++ b/xen/common/mem_access.c
> @@ -99,7 +99,7 @@ int mem_access_memop(unsigned long cmd,
>          if ( (mao.pfn > domain_get_maximum_gpfn(d)) && mao.pfn != ~0ull )
>              break;
>
> -        rc = p2m_get_mem_access(d, _gfn(mao.pfn), &access);
> +        rc = p2m_get_mem_access(d, _gfn(mao.pfn), &access, 0);
>          if ( rc != 0 )
>              break;
>
> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> index bbba99e5f5..bbb0aa984a 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -234,6 +234,7 @@ struct xen_hvm_altp2m_view {
>  typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_view_t);
>
> +#if __XEN_INTERFACE_VERSION__ < 0x00040a00
>  struct xen_hvm_altp2m_set_mem_access {
>      /* view */
>      uint16_t view;
> @@ -245,6 +246,19 @@ struct xen_hvm_altp2m_set_mem_access {
>  };
>  typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
> +#endif /* __XEN_INTERFACE_VERSION__ < 0x00040a00 */
> +
> +struct xen_hvm_altp2m_mem_access {
> +    /* view */
> +    uint16_t view;
> +    /* Memory type */
> +    uint16_t hvmmem_access; /* xenmem_access_t */

A structure name with "mem_access" having a variable name
"hvmmem_access" and a comment saying "xenmem_access". This is
confusing. I understand that you copy/pasted this from the existing op
but it doesn't look good. Perhaps fix both if we are touching it?
Also, in public/memory.h the width of access is uint8_t so I'm not
sure why the discrepancy.

> +    uint32_t pad;
> +    /* gfn */
> +    uint64_t gfn;
> +};
> +typedef struct xen_hvm_altp2m_mem_access xen_hvm_altp2m_mem_access_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_mem_access_t);
>
>  struct xen_hvm_altp2m_set_mem_access_multi {
>      /* view */
> @@ -296,6 +310,8 @@ struct xen_hvm_altp2m_op {
>  #define HVMOP_altp2m_change_gfn           8
>  /* Set access for an array of pages */
>  #define HVMOP_altp2m_set_mem_access_multi 9
> +/* Get the access of a page of memory from a certain view */
> +#define HVMOP_altp2m_get_mem_access       10
>      domid_t domain;
>      uint16_t pad1;
>      uint32_t pad2;
> @@ -303,7 +319,10 @@ struct xen_hvm_altp2m_op {
>          struct xen_hvm_altp2m_domain_state         domain_state;
>          struct xen_hvm_altp2m_vcpu_enable_notify   enable_notify;
>          struct xen_hvm_altp2m_view                 view;
> +#if __XEN_INTERFACE_VERSION__ < 0x00040a00
>          struct xen_hvm_altp2m_set_mem_access       set_mem_access;
> +#endif /* __XEN_INTERFACE_VERSION__ < 0x00040a00 */
> +        struct xen_hvm_altp2m_mem_access           mem_access;
>          struct xen_hvm_altp2m_change_gfn           change_gfn;
>          struct xen_hvm_altp2m_set_mem_access_multi set_mem_access_multi;
>          uint8_t pad[64];
> diff --git a/xen/include/public/xen-compat.h b/xen/include/public/xen-compat.h
> index b67365340b..fa6ffb72e8 100644
> --- a/xen/include/public/xen-compat.h
> +++ b/xen/include/public/xen-compat.h
> @@ -27,7 +27,7 @@
>  #ifndef __XEN_PUBLIC_XEN_COMPAT_H__
>  #define __XEN_PUBLIC_XEN_COMPAT_H__
>
> -#define __XEN_LATEST_INTERFACE_VERSION__ 0x00040900
> +#define __XEN_LATEST_INTERFACE_VERSION__ 0x00040a00
>
>  #if defined(__XEN__) || defined(__XEN_TOOLS__)
>  /* Xen is built with matching headers and implements the latest interface. */
> diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h
> index 7e95eab81c..7348f81233 100644
> --- a/xen/include/xen/mem_access.h
> +++ b/xen/include/xen/mem_access.h
> @@ -76,7 +76,8 @@ long p2m_set_mem_access_multi(struct domain *d,
>   * Get access type for a gfn.
>   * If gfn == INVALID_GFN, gets the default access type.
>   */
> -int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access);
> +int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access,
> +                       unsigned int altp2m_idx);
>
>  #ifdef CONFIG_MEM_ACCESS
>  int mem_access_memop(unsigned long cmd,
> --
> 2.18.0

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.