Re: [PATCH] mem_access: Use monitor_traps instead of mem_access_send_req

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 07/28/2016 10:35 PM, Tamas K Lengyel wrote:
> The two functions monitor_traps and mem_access_send_req duplicate
> some of the same functionality. The mem_access_send_req however leaves a
> lot of the standard vm_event fields to be filled by other functions.
> 
> Since mem_access events go on the monitor ring in this patch we consolidate
> all paths to use monitor_traps to place events on the ring and to fill in
> the common parts of the requests.
> 
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> ---
>  xen/arch/arm/p2m.c                | 69 +++++++++++++++++++--------------------
>  xen/arch/x86/hvm/hvm.c            | 16 ++++++---
>  xen/arch/x86/hvm/monitor.c        |  6 ++++
>  xen/arch/x86/mm/p2m.c             | 24 ++------------
>  xen/common/mem_access.c           | 11 -------
>  xen/include/asm-x86/hvm/monitor.h |  2 ++
>  xen/include/asm-x86/p2m.h         | 13 +++++---
>  xen/include/xen/mem_access.h      |  7 ----
>  8 files changed, 63 insertions(+), 85 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index d82349c..df898a3 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -5,7 +5,7 @@
>  #include <xen/domain_page.h>
>  #include <xen/bitops.h>
>  #include <xen/vm_event.h>
> -#include <xen/mem_access.h>
> +#include <xen/monitor.h>
>  #include <xen/iocap.h>
>  #include <public/vm_event.h>
>  #include <asm/flushtlb.h>
> @@ -1642,12 +1642,41 @@ void __init setup_virt_paging(void)
>      smp_call_function(setup_virt_paging_one, (void *)val, 1);
>  }
>  
> +static int
> +__p2m_mem_access_send_req(paddr_t gpa, vaddr_t gla, const struct npfec npfec,
> +                          xenmem_access_t xma)
> +{
> +    struct vcpu *v = current;
> +    vm_event_request_t req = {};
> +    bool_t sync = (xma == XENMEM_access_n2rwx) ? 0 : 1;
> +
> +    req.reason = VM_EVENT_REASON_MEM_ACCESS;
> +
> +    /* Send request to mem access subscriber */
> +    req.u.mem_access.gfn = gpa >> PAGE_SHIFT;
> +    req.u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1);
> +    if ( npfec.gla_valid )
> +    {
> +        req.u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
> +        req.u.mem_access.gla = gla;
> +
> +        if ( npfec.kind == npfec_kind_with_gla )
> +            req.u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
> +        else if ( npfec.kind == npfec_kind_in_gpt )
> +            req.u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
> +    }
> +    req.u.mem_access.flags |= npfec.read_access    ? MEM_ACCESS_R : 0;
> +    req.u.mem_access.flags |= npfec.write_access   ? MEM_ACCESS_W : 0;
> +    req.u.mem_access.flags |= npfec.insn_fetch     ? MEM_ACCESS_X : 0;
> +
> +    return monitor_traps(v, sync, &req);
> +}
> +
>  bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
>  {
>      int rc;
>      bool_t violation;
>      xenmem_access_t xma;
> -    vm_event_request_t *req;
>      struct vcpu *v = current;
>      struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
>  
> @@ -1734,40 +1763,8 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
>          return false;
>      }
>  
> -    req = xzalloc(vm_event_request_t);
> -    if ( req )
> -    {
> -        req->reason = VM_EVENT_REASON_MEM_ACCESS;
> -
> -        /* Pause the current VCPU */
> -        if ( xma != XENMEM_access_n2rwx )
> -            req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
> -
> -        /* Send request to mem access subscriber */
> -        req->u.mem_access.gfn = gpa >> PAGE_SHIFT;
> -        req->u.mem_access.offset =  gpa & ((1 << PAGE_SHIFT) - 1);
> -        if ( npfec.gla_valid )
> -        {
> -            req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
> -            req->u.mem_access.gla = gla;
> -
> -            if ( npfec.kind == npfec_kind_with_gla )
> -                req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
> -            else if ( npfec.kind == npfec_kind_in_gpt )
> -                req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
> -        }
> -        req->u.mem_access.flags |= npfec.read_access    ? MEM_ACCESS_R : 0;
> -        req->u.mem_access.flags |= npfec.write_access   ? MEM_ACCESS_W : 0;
> -        req->u.mem_access.flags |= npfec.insn_fetch     ? MEM_ACCESS_X : 0;
> -        req->vcpu_id = v->vcpu_id;

The line setting req->vcpu_id has been removed here ...

> -
> -        mem_access_send_req(v->domain, req);
> -        xfree(req);
> -    }
> -
> -    /* Pause the current VCPU */
> -    if ( xma != XENMEM_access_n2rwx )
> -        vm_event_vcpu_pause(v);
> +    if ( __p2m_mem_access_send_req(gpa, gla, npfec, xma) < 0 )
> +        domain_crash(v->domain);
>  
>      return false;
>  }
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index daaee1d..688370d 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1707,7 +1707,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
>      int rc, fall_through = 0, paged = 0;
>      int sharing_enomem = 0;
>      vm_event_request_t *req_ptr = NULL;
> -    bool_t ap2m_active;
> +    bool_t ap2m_active, sync = 0;
>  
>      /* On Nested Virtualization, walk the guest page table.
>       * If this succeeds, all is fine.
> @@ -1846,11 +1846,12 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
>                  }
>              }
>  
> -            if ( p2m_mem_access_check(gpa, gla, npfec, &req_ptr) )
> -            {
> +            sync = p2m_mem_access_check(gpa, gla, npfec, &req_ptr);
> +
> +            if ( !sync ) {
>                  fall_through = 1;
>              } else {
> -                /* Rights not promoted, vcpu paused, work here is done */
> +                /* Rights not promoted (aka. sync event), work here is done */
>                  rc = 1;
>                  goto out_put_gfn;
>              }
> @@ -1956,7 +1957,12 @@ out:
>      }
>      if ( req_ptr )
>      {
> -        mem_access_send_req(currd, req_ptr);
> +        if ( hvm_monitor_mem_access(curr, sync, req_ptr) < 0 )
> +        {
> +            /* Crash the domain */
> +            rc = 0;
> +        }
> +
>          xfree(req_ptr);
>      }
>      return rc;
> diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
> index 7277c12..c7285c6 100644
> --- a/xen/arch/x86/hvm/monitor.c
> +++ b/xen/arch/x86/hvm/monitor.c
> @@ -152,6 +152,12 @@ int hvm_monitor_cpuid(unsigned long insn_length)
>      return monitor_traps(curr, 1, &req);
>  }
>  
> +int hvm_monitor_mem_access(struct vcpu* v, bool_t sync,
> +                           vm_event_request_t *req)
> +{
> +    return monitor_traps(v, sync, req);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 812dbf6..27f9d26 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1728,13 +1728,8 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>      if ( req )
>      {
>          *req_ptr = req;
> -        req->reason = VM_EVENT_REASON_MEM_ACCESS;
> -
> -        /* Pause the current VCPU */
> -        if ( p2ma != p2m_access_n2rwx )
> -            req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
>  
> -        /* Send request to mem event */
> +        req->reason = VM_EVENT_REASON_MEM_ACCESS;
>          req->u.mem_access.gfn = gfn;
>          req->u.mem_access.offset = gpa & ((1 << PAGE_SHIFT) - 1);
>          if ( npfec.gla_valid )
> @@ -1750,23 +1745,10 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>          req->u.mem_access.flags |= npfec.read_access    ? MEM_ACCESS_R : 0;
>          req->u.mem_access.flags |= npfec.write_access   ? MEM_ACCESS_W : 0;
>          req->u.mem_access.flags |= npfec.insn_fetch     ? MEM_ACCESS_X : 0;
> -        req->vcpu_id = v->vcpu_id;

... and here, and as such it doesn't seem to get set anywhere else now.
Am I missing an code path outside of this patch where req->vcpu_id is
being correctly set so this has become unnecessary?


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel