[PATCH] mem_access: Use monitor_traps instead of mem_access_send_req

[PATCH] mem_access: Use monitor_traps instead of mem_access_send_req

[Tamas K Lengyel]

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;
-
-        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;
-
-        vm_event_fill_regs(req);
-
-        if ( altp2m_active(v->domain) )
-        {
-            req->flags |= VM_EVENT_FLAG_ALTERNATE_P2M;
-            req->altp2m_idx = vcpu_altp2m(v).p2midx;
-        }
     }
 
-    /* Pause the current VCPU */
-    if ( p2ma != p2m_access_n2rwx )
-        vm_event_vcpu_pause(v);
-
-    /* VCPU may be paused, return whether we promoted automatically */
-    return (p2ma == p2m_access_n2rwx);
+    /* Return whether vCPU pause is required (aka. sync event) */
+    return (p2ma != p2m_access_n2rwx);
 }
 
 static inline
diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c
index b4033f0..82f4bad 100644
--- a/xen/common/mem_access.c
+++ b/xen/common/mem_access.c
@@ -108,17 +108,6 @@ int mem_access_memop(unsigned long cmd,
     return rc;
 }
 
-int mem_access_send_req(struct domain *d, vm_event_request_t *req)
-{
-    int rc = vm_event_claim_slot(d, &d->vm_event->monitor);
-    if ( rc < 0 )
-        return rc;
-
-    vm_event_put_request(d, &d->vm_event->monitor, req);
-
-    return 0;
-}
-
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-x86/hvm/monitor.h b/xen/include/asm-x86/hvm/monitor.h
index a92f3fc..52c1f47 100644
--- a/xen/include/asm-x86/hvm/monitor.h
+++ b/xen/include/asm-x86/hvm/monitor.h
@@ -41,6 +41,8 @@ void hvm_monitor_msr(unsigned int msr, uint64_t value);
 int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type,
                       unsigned long trap_type, unsigned long insn_length);
 int hvm_monitor_cpuid(unsigned long insn_length);
+int hvm_monitor_mem_access(struct vcpu* v, bool_t sync,
+                           vm_event_request_t *req);
 
 #endif /* __ASM_X86_HVM_MONITOR_H__ */
 
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 194020e..f4a746f 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -660,11 +660,14 @@ int p2m_mem_paging_prep(struct domain *d, unsigned long gfn, uint64_t buffer);
 /* Resume normal operation (in case a domain was paused) */
 void p2m_mem_paging_resume(struct domain *d, vm_event_response_t *rsp);
 
-/* Send mem event based on the access (gla is -1ull if not available).  Handles
- * the rw2rx conversion. Boolean return value indicates if access rights have 
- * been promoted with no underlying vcpu pause. If the req_ptr has been populated, 
- * then the caller must put the event in the ring (once having released get_gfn*
- * locks -- caller must also xfree the request. */
+/*
+ * Setup vm_event request based on the access (gla is -1ull if not available).
+ * Handles the rw2rx conversion. Boolean return value indicates if event type
+ * is syncronous (aka. requires vCPU pause). If the req_ptr has been populated,
+ * then the caller should use monitor_traps to send the event on the MONITOR
+ * ring. Once having released get_gfn* locks caller must also xfree the
+ * request.
+ */
 bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
                             struct npfec npfec,
                             vm_event_request_t **req_ptr);
diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h
index 272f1e4..3d054e0 100644
--- a/xen/include/xen/mem_access.h
+++ b/xen/include/xen/mem_access.h
@@ -29,7 +29,6 @@
 
 int mem_access_memop(unsigned long cmd,
                      XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg);
-int mem_access_send_req(struct domain *d, vm_event_request_t *req);
 
 static inline
 void mem_access_resume(struct vcpu *v, vm_event_response_t *rsp)
@@ -47,12 +46,6 @@ int mem_access_memop(unsigned long cmd,
 }
 
 static inline
-int mem_access_send_req(struct domain *d, vm_event_request_t *req)
-{
-    return -ENOSYS;
-}
-
-static inline
 void mem_access_resume(struct vcpu *vcpu, vm_event_response_t *rsp)
 {
     /* Nothing to do. */
-- 
2.8.1


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

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

[Julien Grall]

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@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;
> -
> -        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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Andrew Cooper]

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@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>

Common and x86 bits Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>, but a few suggestions.

> 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
> @@ -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);

I see this is only code motion, but ~PAGE_MASK here instead of
open-coding it.

> 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
> @@ -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 ) {

Please keep this brace on the newline (inline with the style), even if
it doesn't match the style of the else clause.

>                  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;
> +        }

It is reasonable to omit the braces here.

> +
>          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,

vcpu *v.

~Andrew

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

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

[Tamas K Lengyel]

On Thu, Jul 28, 2016 at 2:54 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> 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@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>
>
> Common and x86 bits Reviewed-by: Andrew Cooper
> <andrew.cooper3@citrix.com>, but a few suggestions.
>
>> 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
>> @@ -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);
>
> I see this is only code motion, but ~PAGE_MASK here instead of
> open-coding it.

Sounds good.

>
>> 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
>> @@ -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 ) {
>
> Please keep this brace on the newline (inline with the style), even if
> it doesn't match the style of the else clause.

Sure.

>
>>                  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;
>> +        }
>
> It is reasonable to omit the braces here.

Yea but with the comment being in-between I just didn't like the look
of it without the braces..

>
>> +
>>          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,
>
> vcpu *v.
>
> ~Andrew

Thanks,
Tamas

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

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

[Tamas K Lengyel]

On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@arm.com> wrote:
> 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@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;
>> -
>> -        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.

Well, the changes are largely cosmetic so doing a whole separate patch
IMHO is an overkill. How about adjusting the commit message to
something like "sanitize code surrounding sending mem_access
vm_events" to better describe the adjustments made in this patch?

Tamas

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

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

[Razvan Cojocaru]

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

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

[Julien Grall]

On 28/07/16 23:54, Tamas K Lengyel wrote:
> On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@arm.com> wrote:
>> On 28/07/2016 20:35, Tamas K Lengyel wrote:
>> 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.
>
> Well, the changes are largely cosmetic so doing a whole separate patch
> IMHO is an overkill. How about adjusting the commit message to
> something like "sanitize code surrounding sending mem_access
> vm_events" to better describe the adjustments made in this patch?

I think the wiki page "Submitting Xen Project patches" [1] should answer 
to your question.

If not, trivial patches are easy to review, merging multiple trivial 
patches in a single patch is not. Moving code and at the same time as 
changing the behavior is fairly difficult to review because it hides the 
real modifications.

Regards,

[1] 
http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches

-- 
Julien Grall

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

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

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 1826 bytes --]

On Jul 29, 2016 02:50, "Julien Grall" <julien.grall@arm.com> wrote:
>
>
>
> On 28/07/16 23:54, Tamas K Lengyel wrote:
>>
>> On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@arm.com>
wrote:
>>>
>>> On 28/07/2016 20:35, Tamas K Lengyel wrote:
>>> 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.
>>
>>
>> Well, the changes are largely cosmetic so doing a whole separate patch
>> IMHO is an overkill. How about adjusting the commit message to
>> something like "sanitize code surrounding sending mem_access
>> vm_events" to better describe the adjustments made in this patch?
>
>
> I think the wiki page "Submitting Xen Project patches" [1] should answer
to your question.
>
> If not, trivial patches are easy to review, merging multiple trivial
patches in a single patch is not. Moving code and at the same time as
changing the behavior is fairly difficult to review because it hides the
real modifications.
>
> Regards,
>
> [1]
http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches
>

The behavior didn't change at all, this whole patch is code sanitization.
It's not worth doing a separate patch for each minor change. The few change
on the arm side is the vm_event request allocation going from xzalloc to
stack based and using monitor_traps now in a split-out function. It really
should be no problem reviewing it. Even Andrew requested minor adjustments
to be included in this patch. Anyway, I'm not looking to change this into a
series. If it's a no go from your side I'm just going to cut down the ARM
side sanitization to the bare minimum of using monitor_traps as the rest
just does not worth the effort.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 2428 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

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

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 8930 bytes --]

On Jul 29, 2016 01:27, "Razvan Cojocaru" <rcojocaru@bitdefender.com> wrote:
>
> 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?
>

Ah yes, I forgot to add that to monitor_traps. The idea is that common
parts if all requests should be set there, including the vcpu_id so we can
cut down on code duplication.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 12590 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

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

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 3268 bytes --]

On 29/07/16 15:21, Tamas K Lengyel wrote:
>
> On Jul 29, 2016 02:50, "Julien Grall" <julien.grall@arm.com
> <mailto:julien.grall@arm.com>> wrote:
> >
> >
> >
> > On 28/07/16 23:54, Tamas K Lengyel wrote:
> >>
> >> On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@arm.com
> <mailto:julien.grall@arm.com>> wrote:
> >>>
> >>> On 28/07/2016 20:35, Tamas K Lengyel wrote:
> >>> 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.
> >>
> >>
> >> Well, the changes are largely cosmetic so doing a whole separate patch
> >> IMHO is an overkill. How about adjusting the commit message to
> >> something like "sanitize code surrounding sending mem_access
> >> vm_events" to better describe the adjustments made in this patch?
> >
> >
> > I think the wiki page "Submitting Xen Project patches" [1] should
> answer to your question.
> >
> > If not, trivial patches are easy to review, merging multiple trivial
> patches in a single patch is not. Moving code and at the same time as
> changing the behavior is fairly difficult to review because it hides
> the real modifications.
> >
> > Regards,
> >
> > [1]
> http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches
> >
>
> The behavior didn't change at all, this whole patch is code
> sanitization. It's not worth doing a separate patch for each minor
> change. The few change on the arm side is the vm_event request
> allocation going from xzalloc to stack based and using monitor_traps
> now in a split-out function. It really should be no problem reviewing
> it. Even Andrew requested minor adjustments to be included in this
> patch. Anyway, I'm not looking to change this into a series. If it's a
> no go from your side I'm just going to cut down the ARM side
> sanitization to the bare minimum of using monitor_traps as the rest
> just does not worth the effort.
>

To offer an alternative opinion.

Looking at this patch, I personally would have a hard time justifying
breaking it down any further.  Each of the changes are closely related.

However, the commit message could be a lot clearer about which steps are
taken.  If I were writing the commit message, I would go with something
a bit more like this:

----8<----
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.

Remove mem_access_send_req() completely, making use of monitor_traps()
to put requests into the monitor ring.  This in turn causes some cleanup
around the old callsites of mem_access_send_req(), and on ARM, the
introduction of the __p2m_mem_access_send_req() helper to fill in common
mem_access information.

Finally, this change identifies that errors from mem_access_send_req()
were never checked.  As errors constitute a problem with the monitor
ring, crashing the domain is the most appropriate action to take.
----8<----

If in doubt, always spell out each of the changes, and their logical
relationships.  If nothing else, it helps people trying to review the patch.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 4894 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

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

[Stefano Stabellini]

On Fri, 29 Jul 2016, Tamas K Lengyel wrote:
> On Jul 29, 2016 02:50, "Julien Grall" <julien.grall@arm.com> wrote:
> >
> >
> >
> > On 28/07/16 23:54, Tamas K Lengyel wrote:
> >>
> >> On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@arm.com> wrote:
> >>>
> >>> On 28/07/2016 20:35, Tamas K Lengyel wrote:
> >>> 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.
> >>
> >>
> >> Well, the changes are largely cosmetic so doing a whole separate patch
> >> IMHO is an overkill. How about adjusting the commit message to
> >> something like "sanitize code surrounding sending mem_access
> >> vm_events" to better describe the adjustments made in this patch?
> >
> >
> > I think the wiki page "Submitting Xen Project patches" [1] should answer to your question.
> >
> > If not, trivial patches are easy to review, merging multiple trivial patches in a single patch is not. Moving
> code and at the same time as changing the behavior is fairly difficult to review because it hides the real
> modifications.
> >
> > Regards,
> >
> > [1] http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches
> >
> 
> The behavior didn't change at all, this whole patch is code sanitization. It's not worth doing a separate patch
> for each minor change. The few change on the arm side is the vm_event request allocation going from xzalloc to
> stack based and using monitor_traps now in a split-out function. It really should be no problem reviewing it. Even
> Andrew requested minor adjustments to be included in this patch. Anyway, I'm not looking to change this into a
> series. If it's a no go from your side I'm just going to cut down the ARM side sanitization to the bare minimum of
> using monitor_traps as the rest just does not worth the effort.

Hi Tamas,
let me premise that, like you wrote, the patch is just code
sanitization, certainly not worth turning it into an argument.

I think different maintainers have different styles. I for one always
dislike to have code movement, renaming or code style fixes together
with any other more meaningful changes. In fact when people suggest
"could you please also rename this variable" or "could you please also
move the function to common code", I usually reply: "I can, but it will
be in another patch".

So I agree with Julien that I would prefer to see two patches. In fact
if I were you, I would prefer to write two separate patches because it
would be less troubles for me as a developer. But clearly not everybody
agrees with this style as I get requests for cosmetic changes together
with other changes by many other seasoned maintainers. And that's OK, it
just means that different people think differently, which is a good
thing for the project as a whole.

You are a valued contributor and maintainer of this project -- if you
strongly feel this way, I am sure we can find a way to make this work
anyway.

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

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

[Tamas K Lengyel]

On Fri, Jul 29, 2016 at 10:27 AM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 29/07/16 15:21, Tamas K Lengyel wrote:
>
> On Jul 29, 2016 02:50, "Julien Grall" <julien.grall@arm.com> wrote:
>>
>>
>>
>> On 28/07/16 23:54, Tamas K Lengyel wrote:
>>>
>>> On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@arm.com>
>>> wrote:
>>>>
>>>> On 28/07/2016 20:35, Tamas K Lengyel wrote:
>>>> 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.
>>>
>>>
>>> Well, the changes are largely cosmetic so doing a whole separate patch
>>> IMHO is an overkill. How about adjusting the commit message to
>>> something like "sanitize code surrounding sending mem_access
>>> vm_events" to better describe the adjustments made in this patch?
>>
>>
>> I think the wiki page "Submitting Xen Project patches" [1] should answer
>> to your question.
>>
>> If not, trivial patches are easy to review, merging multiple trivial
>> patches in a single patch is not. Moving code and at the same time as
>> changing the behavior is fairly difficult to review because it hides the
>> real modifications.
>>
>> Regards,
>>
>> [1]
>> http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches
>>
>
> The behavior didn't change at all, this whole patch is code sanitization.
> It's not worth doing a separate patch for each minor change. The few change
> on the arm side is the vm_event request allocation going from xzalloc to
> stack based and using monitor_traps now in a split-out function. It really
> should be no problem reviewing it. Even Andrew requested minor adjustments
> to be included in this patch. Anyway, I'm not looking to change this into a
> series. If it's a no go from your side I'm just going to cut down the ARM
> side sanitization to the bare minimum of using monitor_traps as the rest
> just does not worth the effort.
>
>
> To offer an alternative opinion.
>
> Looking at this patch, I personally would have a hard time justifying
> breaking it down any further.  Each of the changes are closely related.
>
> However, the commit message could be a lot clearer about which steps are
> taken.  If I were writing the commit message, I would go with something a
> bit more like this:
>
> ----8<----
> 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.
>
> Remove mem_access_send_req() completely, making use of monitor_traps() to
> put requests into the monitor ring.  This in turn causes some cleanup around
> the old callsites of mem_access_send_req(), and on ARM, the introduction of
> the __p2m_mem_access_send_req() helper to fill in common mem_access
> information.
>
> Finally, this change identifies that errors from mem_access_send_req() were
> never checked.  As errors constitute a problem with the monitor ring,
> crashing the domain is the most appropriate action to take.
> ----8<----
>
> If in doubt, always spell out each of the changes, and their logical
> relationships.  If nothing else, it helps people trying to review the patch.
>

Thanks and I agree, adjusting the commit message is what I would think
would make sense as well here (and what I offered as an alternative to
breaking down the series into tiny patches).

Tamas

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

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

[Tamas K Lengyel]

On Fri, Jul 29, 2016 at 11:38 AM, Stefano Stabellini
<sstabellini@kernel.org> wrote:
> On Fri, 29 Jul 2016, Tamas K Lengyel wrote:
>> On Jul 29, 2016 02:50, "Julien Grall" <julien.grall@arm.com> wrote:
>> >
>> >
>> >
>> > On 28/07/16 23:54, Tamas K Lengyel wrote:
>> >>
>> >> On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@arm.com> wrote:
>> >>>
>> >>> On 28/07/2016 20:35, Tamas K Lengyel wrote:
>> >>> 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.
>> >>
>> >>
>> >> Well, the changes are largely cosmetic so doing a whole separate patch
>> >> IMHO is an overkill. How about adjusting the commit message to
>> >> something like "sanitize code surrounding sending mem_access
>> >> vm_events" to better describe the adjustments made in this patch?
>> >
>> >
>> > I think the wiki page "Submitting Xen Project patches" [1] should answer to your question.
>> >
>> > If not, trivial patches are easy to review, merging multiple trivial patches in a single patch is not. Moving
>> code and at the same time as changing the behavior is fairly difficult to review because it hides the real
>> modifications.
>> >
>> > Regards,
>> >
>> > [1] http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches
>> >
>>
>> The behavior didn't change at all, this whole patch is code sanitization. It's not worth doing a separate patch
>> for each minor change. The few change on the arm side is the vm_event request allocation going from xzalloc to
>> stack based and using monitor_traps now in a split-out function. It really should be no problem reviewing it. Even
>> Andrew requested minor adjustments to be included in this patch. Anyway, I'm not looking to change this into a
>> series. If it's a no go from your side I'm just going to cut down the ARM side sanitization to the bare minimum of
>> using monitor_traps as the rest just does not worth the effort.
>
> Hi Tamas,
> let me premise that, like you wrote, the patch is just code
> sanitization, certainly not worth turning it into an argument.
>
> I think different maintainers have different styles. I for one always
> dislike to have code movement, renaming or code style fixes together
> with any other more meaningful changes. In fact when people suggest
> "could you please also rename this variable" or "could you please also
> move the function to common code", I usually reply: "I can, but it will
> be in another patch".
>
> So I agree with Julien that I would prefer to see two patches. In fact
> if I were you, I would prefer to write two separate patches because it
> would be less troubles for me as a developer. But clearly not everybody
> agrees with this style as I get requests for cosmetic changes together
> with other changes by many other seasoned maintainers. And that's OK, it
> just means that different people think differently, which is a good
> thing for the project as a whole.
>
> You are a valued contributor and maintainer of this project -- if you
> strongly feel this way, I am sure we can find a way to make this work
> anyway.

I would highly appreciate that as I said it's just not worth the time
it takes to break this down into smaller patches. It really doubles
the effort it takes to test it. It's within your and Julien's right to
not accept such patches touching your code-base as Maintainer and
that's fine. If that's the case though we might want to look into
adjusting the layout of the code so that mem_access/monitor/vm_event
components are more isolated into separate files so that we can move
at a different phase and different style then the rest of the code
here without getting into arguments about that stuff.

Thanks,
Tamas

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

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

[Julien Grall]

On 29/07/2016 22:02, Tamas K Lengyel wrote:
> On Fri, Jul 29, 2016 at 11:38 AM, Stefano Stabellini
> <sstabellini@kernel.org> wrote:
>> On Fri, 29 Jul 2016, Tamas K Lengyel wrote:
>>> On Jul 29, 2016 02:50, "Julien Grall" <julien.grall@arm.com> wrote:
>>>>
>>>>
>>>>
>>>> On 28/07/16 23:54, Tamas K Lengyel wrote:
>>>>>
>>>>> On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@arm.com> wrote:
>>>>>>
>>>>>> On 28/07/2016 20:35, Tamas K Lengyel wrote:
>>>>>> 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.
>>>>>
>>>>>
>>>>> Well, the changes are largely cosmetic so doing a whole separate patch
>>>>> IMHO is an overkill. How about adjusting the commit message to
>>>>> something like "sanitize code surrounding sending mem_access
>>>>> vm_events" to better describe the adjustments made in this patch?
>>>>
>>>>
>>>> I think the wiki page "Submitting Xen Project patches" [1] should answer to your question.
>>>>
>>>> If not, trivial patches are easy to review, merging multiple trivial patches in a single patch is not. Moving
>>> code and at the same time as changing the behavior is fairly difficult to review because it hides the real
>>> modifications.
>>>>
>>>> Regards,
>>>>
>>>> [1] http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches
>>>>
>>>
>>> The behavior didn't change at all, this whole patch is code sanitization. It's not worth doing a separate patch
>>> for each minor change. The few change on the arm side is the vm_event request allocation going from xzalloc to
>>> stack based and using monitor_traps now in a split-out function. It really should be no problem reviewing it. Even
>>> Andrew requested minor adjustments to be included in this patch. Anyway, I'm not looking to change this into a
>>> series. If it's a no go from your side I'm just going to cut down the ARM side sanitization to the bare minimum of
>>> using monitor_traps as the rest just does not worth the effort.
>>
>> Hi Tamas,
>> let me premise that, like you wrote, the patch is just code
>> sanitization, certainly not worth turning it into an argument.
>>
>> I think different maintainers have different styles. I for one always
>> dislike to have code movement, renaming or code style fixes together
>> with any other more meaningful changes. In fact when people suggest
>> "could you please also rename this variable" or "could you please also
>> move the function to common code", I usually reply: "I can, but it will
>> be in another patch".
>>
>> So I agree with Julien that I would prefer to see two patches. In fact
>> if I were you, I would prefer to write two separate patches because it
>> would be less troubles for me as a developer. But clearly not everybody
>> agrees with this style as I get requests for cosmetic changes together
>> with other changes by many other seasoned maintainers. And that's OK, it
>> just means that different people think differently, which is a good
>> thing for the project as a whole.
>>
>> You are a valued contributor and maintainer of this project -- if you
>> strongly feel this way, I am sure we can find a way to make this work
>> anyway.
>
> I would highly appreciate that as I said it's just not worth the time
> it takes to break this down into smaller patches. It really doubles
> the effort it takes to test it.

I find this paragraph really offensive for reviewers. This requires more 
efforts for reviewers to review a such patch. My time is as valuable as 
yours. I hope you will reconsider what you've written.

Regards,
	
>
> Thanks,
> Tamas
>

-- 
Julien Grall

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

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

[Tamas K Lengyel]

On Fri, Jul 29, 2016 at 3:38 PM, Julien Grall <julien.grall@arm.com> wrote:
>
>
> On 29/07/2016 22:02, Tamas K Lengyel wrote:
>>
>> On Fri, Jul 29, 2016 at 11:38 AM, Stefano Stabellini
>> <sstabellini@kernel.org> wrote:
>>>
>>> On Fri, 29 Jul 2016, Tamas K Lengyel wrote:
>>>>
>>>> On Jul 29, 2016 02:50, "Julien Grall" <julien.grall@arm.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On 28/07/16 23:54, Tamas K Lengyel wrote:
>>>>>>
>>>>>>
>>>>>> On Thu, Jul 28, 2016 at 2:38 PM, Julien Grall <julien.grall@arm.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 28/07/2016 20:35, Tamas K Lengyel wrote:
>>>>>>> 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.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Well, the changes are largely cosmetic so doing a whole separate patch
>>>>>> IMHO is an overkill. How about adjusting the commit message to
>>>>>> something like "sanitize code surrounding sending mem_access
>>>>>> vm_events" to better describe the adjustments made in this patch?
>>>>>
>>>>>
>>>>>
>>>>> I think the wiki page "Submitting Xen Project patches" [1] should
>>>>> answer to your question.
>>>>>
>>>>> If not, trivial patches are easy to review, merging multiple trivial
>>>>> patches in a single patch is not. Moving
>>>>
>>>> code and at the same time as changing the behavior is fairly difficult
>>>> to review because it hides the real
>>>> modifications.
>>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> [1]
>>>>> http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches
>>>>>
>>>>
>>>> The behavior didn't change at all, this whole patch is code
>>>> sanitization. It's not worth doing a separate patch
>>>> for each minor change. The few change on the arm side is the vm_event
>>>> request allocation going from xzalloc to
>>>> stack based and using monitor_traps now in a split-out function. It
>>>> really should be no problem reviewing it. Even
>>>> Andrew requested minor adjustments to be included in this patch. Anyway,
>>>> I'm not looking to change this into a
>>>> series. If it's a no go from your side I'm just going to cut down the
>>>> ARM side sanitization to the bare minimum of
>>>> using monitor_traps as the rest just does not worth the effort.
>>>
>>>
>>> Hi Tamas,
>>> let me premise that, like you wrote, the patch is just code
>>> sanitization, certainly not worth turning it into an argument.
>>>
>>> I think different maintainers have different styles. I for one always
>>> dislike to have code movement, renaming or code style fixes together
>>> with any other more meaningful changes. In fact when people suggest
>>> "could you please also rename this variable" or "could you please also
>>> move the function to common code", I usually reply: "I can, but it will
>>> be in another patch".
>>>
>>> So I agree with Julien that I would prefer to see two patches. In fact
>>> if I were you, I would prefer to write two separate patches because it
>>> would be less troubles for me as a developer. But clearly not everybody
>>> agrees with this style as I get requests for cosmetic changes together
>>> with other changes by many other seasoned maintainers. And that's OK, it
>>> just means that different people think differently, which is a good
>>> thing for the project as a whole.
>>>
>>> You are a valued contributor and maintainer of this project -- if you
>>> strongly feel this way, I am sure we can find a way to make this work
>>> anyway.
>>
>>
>> I would highly appreciate that as I said it's just not worth the time
>> it takes to break this down into smaller patches. It really doubles
>> the effort it takes to test it.
>
>
> I find this paragraph really offensive for reviewers. This requires more
> efforts for reviewers to review a such patch. My time is as valuable as
> yours. I hope you will reconsider what you've written.
>

I don't know which part you find offensive and I certainly didn't mean
to offend anyone. I do value your time. I understand this may require
a bit more to review but honestly, I don't think the difference from
your side should be significant. From my side it is and it's not worth
the extra effort to go rounds about this and do a whole series so I'll
just drop the portions you are not happy about. It is your right as
maintainer to reject it as this code right now lives in a generic part
of the ARM code-base. However, as this patch really only touches
things that belong to mem_access/monitor components maybe we should
split these from the generic ARM code altogether to avoid such
conflicts in the future.

Tamas

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

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

[Julien Grall]

Hello Tamas,

On 29/07/16 23:26, Tamas K Lengyel wrote:
> However, as this patch really only touches
> things that belong to mem_access/monitor components maybe we should
> split these from the generic ARM code altogether to avoid such
> conflicts in the future.

I am not against moving part of mem_access outside of the P2M. However, 
the ARM (32-bit and 64-bit) code needs a lot more attention to make it 
work in *all* the case. I was able to break memaccess quite easily on 
various platform (include models).

For instance p2m_mem_access_check_and_get_page is using the hardware to 
translate a VA to an IPA. This only works if the memory where the 
stage-1 page tables reside is not protected. The upcoming support of 
altp2m will make things trickier.

The interface of p2m_mem_access_check_and_get_page should also be 
updated to take a VCPU in parameter.

Lastly getting/setting access in the the mem_access_settings radix tree 
should stay in the p2m code as it is tight to stage-2 page table 
implementation.

Regards,

-- 
Julien Grall

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

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

[Tamas K Lengyel]

On Mon, Aug 1, 2016 at 4:33 AM, Julien Grall <julien.grall@arm.com> wrote:
> Hello Tamas,
>
> On 29/07/16 23:26, Tamas K Lengyel wrote:
>>
>> However, as this patch really only touches
>> things that belong to mem_access/monitor components maybe we should
>> split these from the generic ARM code altogether to avoid such
>> conflicts in the future.
>
>
> I am not against moving part of mem_access outside of the P2M. However, the
> ARM (32-bit and 64-bit) code needs a lot more attention to make it work in
> *all* the case. I was able to break memaccess quite easily on various
> platform (include models).

Yeap, we were able to reproduce the issue on our XU as well. It
fortunately only occurs in a somewhat less-used part of mem_access
where the user changes the default_access and also doesn't pause the
domain while setting mem_access. If either of those conditions aren't
true the bug is avoided. I'm not aware of anyone actually using
default_access at this point, it is a legacy option present in
mem_access for a now unknown use-case.

>
> For instance p2m_mem_access_check_and_get_page is using the hardware to
> translate a VA to an IPA. This only works if the memory where the stage-1
> page tables reside is not protected. The upcoming support of altp2m will
> make things trickier.
>
> The interface of p2m_mem_access_check_and_get_page should also be updated to
> take a VCPU in parameter.

Agree.

>
> Lastly getting/setting access in the the mem_access_settings radix tree
> should stay in the p2m code as it is tight to stage-2 page table
> implementation.

Keeping the radix bookkeeping in p2m should be fine from our
perspective. We will need the p2m lock functions accessible though
from outside p2m (as well as the p2m_{set/get}_entry functions).

Thanks,
Tamas

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