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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Sep 03, 2018 at 06:47:34PM +0300, Adrian Pop 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

You don't need to do this for tools and hypervisor. They are always
built with the latest interface -- see xen-compat.h, which you modified.

The only relevant changes are the ones in hvm_op.h AFAICT.

>  
[...]
> 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 */

#else here please, otherwise you're adding this new interface to a set
of old interfaces.

> +
> +struct xen_hvm_altp2m_mem_access {
> +    /* view */
> +    uint16_t view;
> +    /* Memory type */
> +    uint16_t hvmmem_access; /* xenmem_access_t */
> +    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 {

Why not provide xen_hvm_altp2m_mem_access_multi at the same time. That
would save you adding more compatibility cruft later.

Wei.

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