Re: [PATCH 2/2] altp2m: Implement p2m_get_mem_access for altp2m views

[Wei Liu <wei.liu2@citrix.com>]

On Wed, Jan 27, 2016 at 01:06:41PM -0700, Tamas K Lengyel wrote:
> Extend the existing get_mem_access memop to allow querying permissions in
> altp2m views as well.
> 
> Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  tools/libxc/include/xenctrl.h       |  3 ++-
>  tools/libxc/xc_mem_access.c         |  8 +++++---
>  tools/tests/xen-access/xen-access.c |  5 ++++-

The code looks sensible. Some nits and question below.

>  xen/arch/arm/p2m.c                  |  4 ++--
>  xen/arch/x86/mm/p2m.c               | 23 +++++++++++++++++++----
>  xen/common/mem_access.c             |  2 +-
>  xen/include/xen/p2m-common.h        |  3 ++-
>  7 files changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index b4e57d8..09d9f62 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2065,7 +2065,8 @@ int xc_set_mem_access(xc_interface *xch, domid_t domain_id,
>   * Gets the mem access for the given page (returned in access on success)
>   */
>  int xc_get_mem_access(xc_interface *xch, domid_t domain_id,
> -                      uint64_t pfn, xenmem_access_t *access);
> +                      uint64_t pfn, uint16_t altp2m_idx,
> +                      xenmem_access_t *access);
>  

The same questions regarding stability of these functions apply here as
well.

>  /*
>   * Instructions causing a mem_access violation can be emulated by Xen
> diff --git a/tools/libxc/xc_mem_access.c b/tools/libxc/xc_mem_access.c
> index d6fb409..a44865d 100644
> --- a/tools/libxc/xc_mem_access.c
> +++ b/tools/libxc/xc_mem_access.c
> @@ -46,14 +46,16 @@ int xc_set_mem_access(xc_interface *xch,
>  int xc_get_mem_access(xc_interface *xch,
>                        domid_t domain_id,
>                        uint64_t pfn,
> +                      uint16_t altp2m_idx,
>                        xenmem_access_t *access)
>  {
>      int rc;
>      xen_mem_access_op_t mao =
>      {
> -        .op    = XENMEM_access_op_get_access,
> -        .domid = domain_id,
> -        .pfn   = pfn
> +        .op         = XENMEM_access_op_get_access,
> +        .domid      = domain_id,
> +        .pfn        = pfn,
> +        .altp2m_idx = altp2m_idx
>      };
>  
>      rc = do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao));
> diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
> index 2300e9a..d9dda62 100644
> --- a/tools/tests/xen-access/xen-access.c
> +++ b/tools/tests/xen-access/xen-access.c
> @@ -571,7 +571,10 @@ int main(int argc, char *argv[])
>  
>              switch (req.reason) {
>              case VM_EVENT_REASON_MEM_ACCESS:
> -                rc = xc_get_mem_access(xch, domain_id, req.u.mem_access.gfn, &access);
> +                rc = xc_get_mem_access(xch, domain_id, req.u.mem_access.gfn,
> +                                       ((req.flags & VM_EVENT_FLAG_ALTERNATE_P2M) ? req.altp2m_idx : 0),

Ling too long.

> +                                       &access
> +                                       );

And fold this ")" to previous line?

>                  if (rc < 0)
>                  {
>                      ERROR("Error %d getting mem_access event\n", rc);
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 8e9b4be..932c6e2 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1666,7 +1666,7 @@ bool_t 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, _gfn(paddr_to_pfn(gpa)), &xma);
> +    rc = p2m_get_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), 0, &xma);
>      if ( rc )
>          return true;
>  
> @@ -1847,7 +1847,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>      return 0;
>  }
>  
> -int p2m_get_mem_access(struct domain *d, gfn_t gfn,
> +int p2m_get_mem_access(struct domain *d, gfn_t gfn, unsigned long altp2m_idx,
>                         xenmem_access_t *access)
>  {
>      int ret;
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 95bf7ce..18068e8 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1572,7 +1572,9 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
>          bool_t violation = 1;
>          const struct vm_event_mem_access *data = &rsp->u.mem_access;
>  
> -        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access) == 0 )
> +        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
> +                                altp2m_active(v->domain) ? vcpu_altp2m(v).p2midx : 0,

Line too long.

Wei.