Re: [Xen-devel] [PATCH] mem_access: Use monitor_traps instead of mem_access_send_req

[Julien Grall <julien.grall@xxxxxxx>]

Hello Tamas,

On 28/07/2016 20:35, 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@xxxxxxxxxxxx>
> ---
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> ---
>  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;
> -
> -        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);
This patch is doing more than it is claimed in the commit message.

In general, moving the code and introducing changes within the same 
patch should really be avoided. So please split it in 2 patches.
Regards,

--
Julien Grall

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