[PATCH v6 1/4] x86: Consolidate boolean inputs in hvm and p2m into a shared bitmap.

[PATCH v6 1/4] x86: Consolidate boolean inputs in hvm and p2m into a shared bitmap.

[Tamas K Lengyel]

This patch consolidates the boolean input parameters of hvm_hap_nested_page_fault and p2m_mem_access_check into a common bitmap and defines the bitmap members accordingly.

v6: Rename shared structure to "struct npfec" and style fixes.
v5: Shared structure in mm.h, style fixes and moving gla fault type additions into next patch in the series.

Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
---
 xen/arch/x86/hvm/hvm.c        | 49 ++++++++++++++++++++++---------------------
 xen/arch/x86/hvm/svm/svm.c    | 15 +++++++------
 xen/arch/x86/hvm/vmx/vmx.c    | 15 ++++++++-----
 xen/arch/x86/mm/p2m.c         | 18 ++++++++--------
 xen/include/asm-x86/hvm/hvm.h |  7 ++-----
 xen/include/asm-x86/mm.h      | 10 +++++++++
 xen/include/asm-x86/p2m.h     |  6 +++---
 7 files changed, 68 insertions(+), 52 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e834406..94a6836 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2722,12 +2722,8 @@ void hvm_inject_page_fault(int errcode, unsigned long cr2)
     hvm_inject_trap(&trap);
 }
 
-int hvm_hap_nested_page_fault(paddr_t gpa,
-                              bool_t gla_valid,
-                              unsigned long gla,
-                              bool_t access_r,
-                              bool_t access_w,
-                              bool_t access_x)
+int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
+                              struct npfec npfec)
 {
     unsigned long gfn = gpa >> PAGE_SHIFT;
     p2m_type_t p2mt;
@@ -2756,8 +2752,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
          * into l1 guest if not fixable. The algorithm is
          * the same as for shadow paging.
          */
-        rv = nestedhvm_hap_nested_page_fault(v, &gpa,
-                                             access_r, access_w, access_x);
+
+         rv = nestedhvm_hap_nested_page_fault(v, &gpa,
+                                              npfec.read_access,
+                                              npfec.write_access,
+                                              npfec.insn_fetch);
         switch (rv) {
         case NESTEDHVM_PAGEFAULT_DONE:
         case NESTEDHVM_PAGEFAULT_RETRY:
@@ -2793,47 +2792,49 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
 
     p2m = p2m_get_hostp2m(v->domain);
     mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma, 
-                              P2M_ALLOC | (access_w ? P2M_UNSHARE : 0), NULL);
+                              P2M_ALLOC | npfec.write_access ? P2M_UNSHARE : 0,
+                              NULL);
 
     /* Check access permissions first, then handle faults */
     if ( mfn_x(mfn) != INVALID_MFN )
     {
-        int violation = 0;
+        bool_t violation;
+
         /* If the access is against the permissions, then send to mem_event */
-        switch (p2ma) 
+        switch (p2ma)
         {
         case p2m_access_n:
         case p2m_access_n2rwx:
         default:
-            violation = access_r || access_w || access_x;
+            violation = npfec.read_access || npfec.write_access || npfec.insn_fetch;
             break;
         case p2m_access_r:
-            violation = access_w || access_x;
+            violation = npfec.write_access || npfec.insn_fetch;
             break;
         case p2m_access_w:
-            violation = access_r || access_x;
+            violation = npfec.read_access || npfec.insn_fetch;
             break;
         case p2m_access_x:
-            violation = access_r || access_w;
+            violation = npfec.read_access || npfec.write_access;
             break;
         case p2m_access_rx:
         case p2m_access_rx2rw:
-            violation = access_w;
+            violation = npfec.write_access;
             break;
         case p2m_access_wx:
-            violation = access_r;
+            violation = npfec.read_access;
             break;
         case p2m_access_rw:
-            violation = access_x;
+            violation = npfec.insn_fetch;
             break;
         case p2m_access_rwx:
+            violation = 0;
             break;
         }
 
         if ( violation )
         {
-            if ( p2m_mem_access_check(gpa, gla_valid, gla, access_r, 
-                                        access_w, access_x, &req_ptr) )
+            if ( p2m_mem_access_check(gpa, gla, npfec, &req_ptr) )
             {
                 fall_through = 1;
             } else {
@@ -2849,7 +2850,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
      * to the mmio handler.
      */
     if ( (p2mt == p2m_mmio_dm) || 
-         (access_w && (p2mt == p2m_ram_ro)) )
+         (npfec.write_access && (p2mt == p2m_ram_ro)) )
     {
         put_gfn(p2m->domain, gfn);
 
@@ -2868,7 +2869,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
         paged = 1;
 
     /* Mem sharing: unshare the page and try again */
-    if ( access_w && (p2mt == p2m_ram_shared) )
+    if ( npfec.write_access && (p2mt == p2m_ram_shared) )
     {
         ASSERT(!p2m_is_nestedp2m(p2m));
         sharing_enomem = 
@@ -2885,7 +2886,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
          * a large page, we do not change other pages type within that large
          * page.
          */
-        if ( access_w )
+        if ( npfec.write_access )
         {
             paging_mark_dirty(v->domain, mfn_x(mfn));
             p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty, p2m_ram_rw);
@@ -2895,7 +2896,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
     }
 
     /* Shouldn't happen: Maybe the guest was writing to a r/o grant mapping? */
-    if ( access_w && (p2mt == p2m_grant_map_ro) )
+    if ( npfec.write_access && (p2mt == p2m_grant_map_ro) )
     {
         gdprintk(XENLOG_WARNING,
                  "trying to write to read-only grant mapping\n");
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 76616ac..1f72e19 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1394,7 +1394,7 @@ const struct hvm_function_table * __init start_svm(void)
 }
 
 static void svm_do_nested_pgfault(struct vcpu *v,
-    struct cpu_user_regs *regs, uint32_t npfec, paddr_t gpa)
+    struct cpu_user_regs *regs, uint32_t pfec, paddr_t gpa)
 {
     int ret;
     unsigned long gfn = gpa >> PAGE_SHIFT;
@@ -1403,10 +1403,13 @@ static void svm_do_nested_pgfault(struct vcpu *v,
     p2m_access_t p2ma;
     struct p2m_domain *p2m = NULL;
 
-    ret = hvm_hap_nested_page_fault(gpa, 0, ~0ul, 
-                                    1, /* All NPFs count as reads */
-                                    npfec & PFEC_write_access, 
-                                    npfec & PFEC_insn_fetch);
+    struct npfec npfec = {
+        .read_access = 1, /* All NPFs count as reads */
+        .write_access = !!(pfec & PFEC_write_access),
+        .insn_fetch = !!(pfec & PFEC_insn_fetch)
+    };
+
+    ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec);
 
     if ( tb_init_done )
     {
@@ -1434,7 +1437,7 @@ static void svm_do_nested_pgfault(struct vcpu *v,
     case -1:
         ASSERT(nestedhvm_enabled(v->domain) && nestedhvm_vcpu_in_guestmode(v));
         /* inject #VMEXIT(NPF) into guest. */
-        nestedsvm_vmexit_defer(v, VMEXIT_NPF, npfec, gpa);
+        nestedsvm_vmexit_defer(v, VMEXIT_NPF, pfec, gpa);
         return;
     }
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 2caa04a..656ce61 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2353,6 +2353,11 @@ static void ept_handle_violation(unsigned long qualification, paddr_t gpa)
     p2m_type_t p2mt;
     int ret;
     struct domain *d = current->domain;
+    struct npfec npfec = {
+        .read_access = !!(qualification & EPT_READ_VIOLATION),
+        .write_access = !!(qualification & EPT_WRITE_VIOLATION),
+        .insn_fetch = !!(qualification & EPT_EXEC_VIOLATION)
+    };
 
     if ( tb_init_done )
     {
@@ -2371,14 +2376,14 @@ static void ept_handle_violation(unsigned long qualification, paddr_t gpa)
     }
 
     if ( qualification & EPT_GLA_VALID )
+    {
         __vmread(GUEST_LINEAR_ADDRESS, &gla);
+        npfec.gla_valid = 1;
+    }
     else
         gla = ~0ull;
-    ret = hvm_hap_nested_page_fault(gpa,
-                                    !!(qualification & EPT_GLA_VALID), gla,
-                                    !!(qualification & EPT_READ_VIOLATION),
-                                    !!(qualification & EPT_WRITE_VIOLATION),
-                                    !!(qualification & EPT_EXEC_VIOLATION));
+
+    ret = hvm_hap_nested_page_fault(gpa, gla, npfec);
     switch ( ret )
     {
     case 0:         // Unhandled L1 EPT violation
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index bca9f0f..1f1f6cd 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1323,9 +1323,9 @@ void p2m_mem_paging_resume(struct domain *d)
     }
 }
 
-bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid, unsigned long gla, 
-                          bool_t access_r, bool_t access_w, bool_t access_x,
-                          mem_event_request_t **req_ptr)
+bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
+                            struct npfec npfec,
+                            mem_event_request_t **req_ptr)
 {
     struct vcpu *v = current;
     unsigned long gfn = gpa >> PAGE_SHIFT;
@@ -1343,7 +1343,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid, unsigned long gla,
     gfn_lock(p2m, gfn, 0);
     mfn = p2m->get_entry(p2m, gfn, &p2mt, &p2ma, 0, NULL);
 
-    if ( access_w && p2ma == p2m_access_rx2rw ) 
+    if ( npfec.write_access && p2ma == p2m_access_rx2rw ) 
     {
         rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, p2m_access_rw);
         ASSERT(rc == 0);
@@ -1352,7 +1352,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid, unsigned long gla,
     }
     else if ( p2ma == p2m_access_n2rwx )
     {
-        ASSERT(access_w || access_r || access_x);
+        ASSERT(npfec.write_access || npfec.read_access || npfec.insn_fetch);
         rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K,
                             p2mt, p2m_access_rwx);
         ASSERT(rc == 0);
@@ -1403,11 +1403,11 @@ bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid, unsigned long gla,
         /* Send request to mem event */
         req->gfn = gfn;
         req->offset = gpa & ((1 << PAGE_SHIFT) - 1);
-        req->gla_valid = gla_valid;
+        req->gla_valid = npfec.gla_valid;
         req->gla = gla;
-        req->access_r = access_r;
-        req->access_w = access_w;
-        req->access_x = access_x;
+        req->access_r = npfec.read_access;
+        req->access_w = npfec.write_access;
+        req->access_x = npfec.insn_fetch;
     
         req->vcpu_id = v->vcpu_id;
     }
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 0ebd478..1123857 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -455,11 +455,8 @@ static inline void hvm_invalidate_regs_fields(struct cpu_user_regs *regs)
 #endif
 }
 
-int hvm_hap_nested_page_fault(paddr_t gpa,
-                              bool_t gla_valid, unsigned long gla,
-                              bool_t access_r,
-                              bool_t access_w,
-                              bool_t access_x);
+int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
+                              struct npfec npfec);
 
 #define hvm_msr_tsc_aux(v) ({                                               \
     struct domain *__d = (v)->domain;                                       \
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index d253117..1889b25 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -551,6 +551,16 @@ void audit_domains(void);
 
 #endif
 
+/*
+ * Nested page fault exception codes.
+ */
+struct npfec {
+    unsigned int read_access:1;
+    unsigned int write_access:1;
+    unsigned int insn_fetch:1;
+    unsigned int gla_valid:1;
+};
+
 int new_guest_cr3(unsigned long pfn);
 void make_cr3(struct vcpu *v, unsigned long mfn);
 void update_cr3(struct vcpu *v);
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 0ddbadb..3975e32 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -597,9 +597,9 @@ void p2m_mem_paging_resume(struct domain *d);
  * 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. */
-bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid, unsigned long gla, 
-                          bool_t access_r, bool_t access_w, bool_t access_x,
-                          mem_event_request_t **req_ptr);
+bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
+                            struct npfec npfec,
+                            mem_event_request_t **req_ptr);
 /* Resumes the running of the VCPU, restarting the last instruction */
 void p2m_mem_access_resume(struct domain *d);
 
-- 
2.0.1

[PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Tamas K Lengyel]

As pointed out by Jan Beulich in http://lists.xen.org/archives/html/xen-devel/2014-08/msg01269.html: "Read-modify-write instructions absolutely need to be treated as read accesses, yet hardware doesn't guarantee to tell us so (they may surface as just write accesses)." This patch addresses the issue in both the VMX and the SVM side.

VMX: Treat all non-instruction fetch violations as read violations (in addition to those that were already reported as read violations).
SVM: Refine the handling to distinguish between read/write and instruction fetch access violations.

Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
---
 xen/arch/x86/hvm/svm/svm.c | 2 +-
 xen/arch/x86/hvm/vmx/vmx.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 1f72e19..9531248 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1404,7 +1404,7 @@ static void svm_do_nested_pgfault(struct vcpu *v,
     struct p2m_domain *p2m = NULL;
 
     struct npfec npfec = {
-        .read_access = 1, /* All NPFs count as reads */
+        .read_access = !(pfec & PFEC_insn_fetch),
         .write_access = !!(pfec & PFEC_write_access),
         .insn_fetch = !!(pfec & PFEC_insn_fetch)
     };
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 656ce61..af0ad7c 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2354,7 +2354,8 @@ static void ept_handle_violation(unsigned long qualification, paddr_t gpa)
     int ret;
     struct domain *d = current->domain;
     struct npfec npfec = {
-        .read_access = !!(qualification & EPT_READ_VIOLATION),
+        .read_access = !!(qualification & EPT_READ_VIOLATION) ||
+                        !(qualification & EPT_EXEC_VIOLATION),
         .write_access = !!(qualification & EPT_WRITE_VIOLATION),
         .insn_fetch = !!(qualification & EPT_EXEC_VIOLATION)
     };
-- 
2.0.1

[PATCH v6 3/4] x86/mem_event: Deliver gla fault EPT violation information

[Tamas K Lengyel]

On Intel EPT the exit qualification generated by a violation also includes a bit (EPT_GLA_FAULT) which describes the following information:
Set if the access causing the EPT violation is to a guest-physical address that is the translation of a linear address. Clear if the access causing the EPT violation is to a paging-structure entry as part of a page walk or the update of an accessed or dirty bit.

For more information see Table 27-7 in the Intel SDM.

This patch extends the mem_event system to deliver this extra information, which could be useful for determining the cause of a violation.

v6: Fixes regarding the enum usage.
v5: Add missing bits to the SVM side, style fixes and switching to shared struct+enum in mm.h.
v4: Use new bitmaps to pass information.
v3: Style fixes.
v2: Split gla_fault into fault_in_gpt and fault_gla to be more compatible with the AMD implementation.

Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
---
 xen/arch/x86/hvm/svm/svm.c        |  8 +++++++-
 xen/arch/x86/hvm/vmx/vmx.c        |  4 ++++
 xen/arch/x86/mm/p2m.c             |  8 +++++++-
 xen/include/asm-x86/hvm/svm/svm.h |  6 ++++++
 xen/include/asm-x86/mm.h          | 11 +++++++++++
 xen/include/public/mem_event.h    |  4 +++-
 6 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 9531248..41365d4 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1394,7 +1394,7 @@ const struct hvm_function_table * __init start_svm(void)
 }
 
 static void svm_do_nested_pgfault(struct vcpu *v,
-    struct cpu_user_regs *regs, uint32_t pfec, paddr_t gpa)
+    struct cpu_user_regs *regs, uint64_t pfec, paddr_t gpa)
 {
     int ret;
     unsigned long gfn = gpa >> PAGE_SHIFT;
@@ -1409,6 +1409,12 @@ static void svm_do_nested_pgfault(struct vcpu *v,
         .insn_fetch = !!(pfec & PFEC_insn_fetch)
     };
 
+    /* These bits are mutually exclusive */
+    if ( pfec & NPT_PFEC_FAULT_WITH_GLA )
+        npfec.npfec_kind = npfec_kind_with_gla;
+    else if ( pfec & NPT_PFEC_FAULT_IN_GPT )
+        npfec.npfec_kind = npfec_kind_in_gpt;
+
     ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec);
 
     if ( tb_init_done )
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index af0ad7c..df687a6 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2380,6 +2380,10 @@ static void ept_handle_violation(unsigned long qualification, paddr_t gpa)
     {
         __vmread(GUEST_LINEAR_ADDRESS, &gla);
         npfec.gla_valid = 1;
+        if( qualification & EPT_GLA_FAULT )
+            npfec.npfec_kind = npfec_kind_with_gla;
+        else
+            npfec.npfec_kind = npfec_kind_in_gpt;
     }
     else
         gla = ~0ull;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 1f1f6cd..fe7b782 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1408,7 +1408,13 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
         req->access_r = npfec.read_access;
         req->access_w = npfec.write_access;
         req->access_x = npfec.insn_fetch;
-    
+        if ( npfec.npfec_kind == npfec_kind_with_gla )
+            req->fault_with_gla = 1;
+        else if ( npfec.npfec_kind == npfec_kind_in_gpt )
+            req->fault_in_gpt = 1;
+        req->access_r = npfec.read_access;
+        req->access_w = npfec.write_access;
+        req->access_x = npfec.insn_fetch;
         req->vcpu_id = v->vcpu_id;
     }
 
diff --git a/xen/include/asm-x86/hvm/svm/svm.h b/xen/include/asm-x86/hvm/svm/svm.h
index 1ffe6d6..e4484b3 100644
--- a/xen/include/asm-x86/hvm/svm/svm.h
+++ b/xen/include/asm-x86/hvm/svm/svm.h
@@ -105,4 +105,10 @@ extern u32 svm_feature_flags;
 extern void svm_host_osvw_reset(void);
 extern void svm_host_osvw_init(void);
 
+/* EXITINFO1 fields on NPT faults */
+#define _NPT_PFEC_FAULT_WITH_GLA     32
+#define NPT_PFEC_FAULT_WITH_GLA      (1UL<<_NPT_PFEC_FAULT_WITH_GLA)
+#define _NPT_PFEC_FAULT_IN_GPT       33
+#define NPT_PFEC_FAULT_IN_GPT        (1UL<<_NPT_PFEC_FAULT_IN_GPT)
+
 #endif /* __ASM_X86_HVM_SVM_H__ */
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 1889b25..7261db7 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -552,6 +552,16 @@ void audit_domains(void);
 #endif
 
 /*
+ * Extra fault info types which are used to further describe
+ * the source of an access violation.
+ */
+typedef enum {
+    npfec_kind_unknown, /* must be first */
+    npfec_kind_in_gpt,  /* violation in guest page table */
+    npfec_kind_with_gla /* violation with guest linear address */
+} npfec_kind_t;
+
+/*
  * Nested page fault exception codes.
  */
 struct npfec {
@@ -559,6 +569,7 @@ struct npfec {
     unsigned int write_access:1;
     unsigned int insn_fetch:1;
     unsigned int gla_valid:1;
+    unsigned int npfec_kind:2;  /* npfec_kind_t */
 };
 
 int new_guest_cr3(unsigned long pfn);
diff --git a/xen/include/public/mem_event.h b/xen/include/public/mem_event.h
index 3831b41..fc12697 100644
--- a/xen/include/public/mem_event.h
+++ b/xen/include/public/mem_event.h
@@ -62,7 +62,9 @@ typedef struct mem_event_st {
     uint16_t access_w:1;
     uint16_t access_x:1;
     uint16_t gla_valid:1;
-    uint16_t available:12;
+    uint16_t fault_with_gla:1;
+    uint16_t fault_in_gpt:1;
+    uint16_t available:10;
 
     uint16_t reason;
 } mem_event_request_t, mem_event_response_t;
-- 
2.0.1

[PATCH v6 4/4] tools/xen-access: Print gla valid/fault information

[Tamas K Lengyel]

Extend the print-out of the memory violations to show gla valid/fault information.

v4: Change vcpu-id printf to unsigned and update to new fields.
v2: Update to new fields and change printing 1/0 to y/n.

Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
---
 tools/tests/xen-access/xen-access.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
index 090df5f..98b3a1c 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -566,13 +566,16 @@ int main(int argc, char *argv[])
                 }
 
                 printf("PAGE ACCESS: %c%c%c for GFN %"PRIx64" (offset %06"
-                       PRIx64") gla %016"PRIx64" (vcpu %d)\n",
+                       PRIx64") gla %016"PRIx64" (valid: %c) fault in gpt: %c fault with gla: %c (vcpu %u)\n",
                        req.access_r ? 'r' : '-',
                        req.access_w ? 'w' : '-',
                        req.access_x ? 'x' : '-',
                        req.gfn,
                        req.offset,
                        req.gla,
+                       req.gla_valid ? 'y' : 'n',
+                       req.fault_in_gpt ? 'y' : 'n',
+                       req.fault_with_gla ? 'y': 'n',
                        req.vcpu_id);
 
                 if ( default_access != after_first_access )
-- 
2.0.1

Re: [PATCH v6 1/4] x86: Consolidate boolean inputs in hvm and p2m into a shared bitmap.

[Jan Beulich]

>>> On 11.08.14 at 16:48, <tamas.lengyel@zentific.com> wrote:
> This patch consolidates the boolean input parameters of 
> hvm_hap_nested_page_fault and p2m_mem_access_check into a common bitmap and 
> defines the bitmap members accordingly.
> 
> v6: Rename shared structure to "struct npfec" and style fixes.
> v5: Shared structure in mm.h, style fixes and moving gla fault type 
> additions into next patch in the series.
> 
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Jan Beulich]

>>> On 11.08.14 at 16:48, <tamas.lengyel@zentific.com> wrote:
> As pointed out by Jan Beulich in 
> http://lists.xen.org/archives/html/xen-devel/2014-08/msg01269.html: 
> "Read-modify-write instructions absolutely need to be treated as read 
> accesses, yet hardware doesn't guarantee to tell us so (they may surface as 
> just write accesses)." This patch addresses the issue in both the VMX and the 
> SVM side.

i.e.
Suggested-by: Jan Beulich <jbeulich@suse.com>

> VMX: Treat all non-instruction fetch violations as read violations (in 
> addition to those that were already reported as read violations).
> SVM: Refine the handling to distinguish between read/write and instruction 
> fetch access violations.
> 
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v6 3/4] x86/mem_event: Deliver gla fault EPT violation information

[Jan Beulich]

>>> On 11.08.14 at 16:48, <tamas.lengyel@zentific.com> wrote:
> On Intel EPT the exit qualification generated by a violation also includes a 
> bit (EPT_GLA_FAULT) which describes the following information:
> Set if the access causing the EPT violation is to a guest-physical address 
> that is the translation of a linear address. Clear if the access causing the 
> EPT violation is to a paging-structure entry as part of a page walk or the 
> update of an accessed or dirty bit.
> 
> For more information see Table 27-7 in the Intel SDM.
> 
> This patch extends the mem_event system to deliver this extra information, 
> which could be useful for determining the cause of a violation.
> 
> v6: Fixes regarding the enum usage.
> v5: Add missing bits to the SVM side, style fixes and switching to shared 
> struct+enum in mm.h.
> v4: Use new bitmaps to pass information.
> v3: Style fixes.
> v2: Split gla_fault into fault_in_gpt and fault_gla to be more compatible 
> with the AMD implementation.
> 
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

albeit if I'll be the one to commit this I'm likely to change ...

> --- a/xen/include/asm-x86/hvm/svm/svm.h
> +++ b/xen/include/asm-x86/hvm/svm/svm.h
> @@ -105,4 +105,10 @@ extern u32 svm_feature_flags;
>  extern void svm_host_osvw_reset(void);
>  extern void svm_host_osvw_init(void);
>  
> +/* EXITINFO1 fields on NPT faults */
> +#define _NPT_PFEC_FAULT_WITH_GLA     32
> +#define NPT_PFEC_FAULT_WITH_GLA      (1UL<<_NPT_PFEC_FAULT_WITH_GLA)
> +#define _NPT_PFEC_FAULT_IN_GPT       33
> +#define NPT_PFEC_FAULT_IN_GPT        (1UL<<_NPT_PFEC_FAULT_IN_GPT)

... these to get rid of the FAULT_ and convert the non-prefix
portion of them to lower case, matching the PFEC_ ones.

Jan

Re: [PATCH v6 1/4] x86: Consolidate boolean inputs in hvm and p2m into a shared bitmap.

[Boris Ostrovsky]

On 08/11/2014 10:48 AM, Tamas K Lengyel wrote:

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 76616ac..1f72e19 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1394,7 +1394,7 @@ const struct hvm_function_table * __init start_svm(void)
  }
  
  static void svm_do_nested_pgfault(struct vcpu *v,
-    struct cpu_user_regs *regs, uint32_t npfec, paddr_t gpa)
+    struct cpu_user_regs *regs, uint32_t pfec, paddr_t gpa)


I am not sure I understand the reason behind this change. This *is* 
still a nested page fault error code, isn't it?

Was this to avoid name collision with npfec variable that is added below?

-boris


>   {
>       int ret;
>       unsigned long gfn = gpa >> PAGE_SHIFT;
> @@ -1403,10 +1403,13 @@ static void svm_do_nested_pgfault(struct vcpu *v,
>       p2m_access_t p2ma;
>       struct p2m_domain *p2m = NULL;
>   
> -    ret = hvm_hap_nested_page_fault(gpa, 0, ~0ul,
> -                                    1, /* All NPFs count as reads */
> -                                    npfec & PFEC_write_access,
> -                                    npfec & PFEC_insn_fetch);
> +    struct npfec npfec = {
> +        .read_access = 1, /* All NPFs count as reads */
> +        .write_access = !!(pfec & PFEC_write_access),
> +        .insn_fetch = !!(pfec & PFEC_insn_fetch)
> +    };
> +
> +    ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec);
>

Re: [PATCH v6 1/4] x86: Consolidate boolean inputs in hvm and p2m into a shared bitmap.

[Tamas Lengyel]

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

On Mon, Aug 11, 2014 at 7:13 PM, Boris Ostrovsky <boris.ostrovsky@oracle.com
> wrote:

> On 08/11/2014 10:48 AM, Tamas K Lengyel wrote:
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 76616ac..1f72e19 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1394,7 +1394,7 @@ const struct hvm_function_table * __init
> start_svm(void)
>  }
>   static void svm_do_nested_pgfault(struct vcpu *v,
> -    struct cpu_user_regs *regs, uint32_t npfec, paddr_t gpa)
> +    struct cpu_user_regs *regs, uint32_t pfec, paddr_t gpa)
>
>
> I am not sure I understand the reason behind this change. This *is* still
> a nested page fault error code, isn't it?
>
> Was this to avoid name collision with npfec variable that is added below?
>
> -boris


Yes, that's correct.


>
>
>
>    {
>>       int ret;
>>       unsigned long gfn = gpa >> PAGE_SHIFT;
>> @@ -1403,10 +1403,13 @@ static void svm_do_nested_pgfault(struct vcpu *v,
>>       p2m_access_t p2ma;
>>       struct p2m_domain *p2m = NULL;
>>   -    ret = hvm_hap_nested_page_fault(gpa, 0, ~0ul,
>> -                                    1, /* All NPFs count as reads */
>> -                                    npfec & PFEC_write_access,
>> -                                    npfec & PFEC_insn_fetch);
>> +    struct npfec npfec = {
>> +        .read_access = 1, /* All NPFs count as reads */
>> +        .write_access = !!(pfec & PFEC_write_access),
>> +        .insn_fetch = !!(pfec & PFEC_insn_fetch)
>> +    };
>> +
>> +    ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec);
>>
>>
>

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

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

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

Re: [PATCH v6 3/4] x86/mem_event: Deliver gla fault EPT violation information

[Boris Ostrovsky]

On 08/11/2014 10:48 AM, Tamas K Lengyel wrote:
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 1f1f6cd..fe7b782 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1408,7 +1408,13 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>           req->access_r = npfec.read_access;
>           req->access_w = npfec.write_access;
>           req->access_x = npfec.insn_fetch;
> -
> +        if ( npfec.npfec_kind == npfec_kind_with_gla )
> +            req->fault_with_gla = 1;
> +        else if ( npfec.npfec_kind == npfec_kind_in_gpt )
> +            req->fault_in_gpt = 1;
> +        req->access_r = npfec.read_access;
> +        req->access_w = npfec.write_access;
> +        req->access_x = npfec.insn_fetch;


You seem to be assigning req->access_? values twice.


-boris


>           req->vcpu_id = v->vcpu_id;
>       }
>   

Re: [PATCH v6 3/4] x86/mem_event: Deliver gla fault EPT violation information

[Tamas Lengyel]

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

Whops, that's just an accident. Will resubmit with the fix tomorrow to
leave some time for review.

Tamas


On Mon, Aug 11, 2014 at 7:27 PM, Boris Ostrovsky <boris.ostrovsky@oracle.com
> wrote:

> On 08/11/2014 10:48 AM, Tamas K Lengyel wrote:
>
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index 1f1f6cd..fe7b782 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -1408,7 +1408,13 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned
>> long gla,
>>           req->access_r = npfec.read_access;
>>           req->access_w = npfec.write_access;
>>           req->access_x = npfec.insn_fetch;
>> -
>> +        if ( npfec.npfec_kind == npfec_kind_with_gla )
>> +            req->fault_with_gla = 1;
>> +        else if ( npfec.npfec_kind == npfec_kind_in_gpt )
>> +            req->fault_in_gpt = 1;
>> +        req->access_r = npfec.read_access;
>> +        req->access_w = npfec.write_access;
>> +        req->access_x = npfec.insn_fetch;
>>
>
>
> You seem to be assigning req->access_? values twice.
>
>
> -boris
>
>
>
>            req->vcpu_id = v->vcpu_id;
>>       }
>>
>>
>
>

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

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

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

Re: [PATCH v6 3/4] x86/mem_event: Deliver gla fault EPT violation information

[Tamas Lengyel]

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

On Mon, Aug 11, 2014 at 6:08 PM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 11.08.14 at 16:48, <tamas.lengyel@zentific.com> wrote:
> > On Intel EPT the exit qualification generated by a violation also
> includes a
> > bit (EPT_GLA_FAULT) which describes the following information:
> > Set if the access causing the EPT violation is to a guest-physical
> address
> > that is the translation of a linear address. Clear if the access causing
> the
> > EPT violation is to a paging-structure entry as part of a page walk or
> the
> > update of an accessed or dirty bit.
> >
> > For more information see Table 27-7 in the Intel SDM.
> >
> > This patch extends the mem_event system to deliver this extra
> information,
> > which could be useful for determining the cause of a violation.
> >
> > v6: Fixes regarding the enum usage.
> > v5: Add missing bits to the SVM side, style fixes and switching to shared
> > struct+enum in mm.h.
> > v4: Use new bitmaps to pass information.
> > v3: Style fixes.
> > v2: Split gla_fault into fault_in_gpt and fault_gla to be more compatible
> > with the AMD implementation.
> >
> > Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> albeit if I'll be the one to commit this I'm likely to change ...
>
> > --- a/xen/include/asm-x86/hvm/svm/svm.h
> > +++ b/xen/include/asm-x86/hvm/svm/svm.h
> > @@ -105,4 +105,10 @@ extern u32 svm_feature_flags;
> >  extern void svm_host_osvw_reset(void);
> >  extern void svm_host_osvw_init(void);
> >
> > +/* EXITINFO1 fields on NPT faults */
> > +#define _NPT_PFEC_FAULT_WITH_GLA     32
> > +#define NPT_PFEC_FAULT_WITH_GLA      (1UL<<_NPT_PFEC_FAULT_WITH_GLA)
> > +#define _NPT_PFEC_FAULT_IN_GPT       33
> > +#define NPT_PFEC_FAULT_IN_GPT        (1UL<<_NPT_PFEC_FAULT_IN_GPT)
>
> ... these to get rid of the FAULT_ and convert the non-prefix
> portion of them to lower case, matching the PFEC_ ones.
>
> Jan
>

As I'll need to resend this patch anyway I'll just include this suggestion
in it.

Tamas

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

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

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

Re: [PATCH v6 3/4] x86/mem_event: Deliver gla fault EPT violation information

[Jan Beulich]

>>> On 12.08.14 at 13:18, <tamas.lengyel@zentific.com> wrote:
> On Mon, Aug 11, 2014 at 6:08 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 11.08.14 at 16:48, <tamas.lengyel@zentific.com> wrote:
>> > --- a/xen/include/asm-x86/hvm/svm/svm.h
>> > +++ b/xen/include/asm-x86/hvm/svm/svm.h
>> > @@ -105,4 +105,10 @@ extern u32 svm_feature_flags;
>> >  extern void svm_host_osvw_reset(void);
>> >  extern void svm_host_osvw_init(void);
>> >
>> > +/* EXITINFO1 fields on NPT faults */
>> > +#define _NPT_PFEC_FAULT_WITH_GLA     32
>> > +#define NPT_PFEC_FAULT_WITH_GLA      (1UL<<_NPT_PFEC_FAULT_WITH_GLA)
>> > +#define _NPT_PFEC_FAULT_IN_GPT       33
>> > +#define NPT_PFEC_FAULT_IN_GPT        (1UL<<_NPT_PFEC_FAULT_IN_GPT)
>>
>> ... these to get rid of the FAULT_ and convert the non-prefix
>> portion of them to lower case, matching the PFEC_ ones.
> 
> As I'll need to resend this patch anyway I'll just include this suggestion
> in it.

Thanks - that's much appreciated.

Jan

Re: [PATCH v6 4/4] tools/xen-access: Print gla valid/fault information

[Ian Jackson]

Tamas K Lengyel writes ("[PATCH v6 4/4] tools/xen-access: Print gla valid/fault information"):
> Extend the print-out of the memory violations to show gla valid/fault information.
> 
> v4: Change vcpu-id printf to unsigned and update to new fields.
> v2: Update to new fields and change printing 1/0 to y/n.
> 
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
> ---
>  tools/tests/xen-access/xen-access.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
> index 090df5f..98b3a1c 100644
> --- a/tools/tests/xen-access/xen-access.c
> +++ b/tools/tests/xen-access/xen-access.c
> @@ -566,13 +566,16 @@ int main(int argc, char *argv[])
>                  }
>  
>                  printf("PAGE ACCESS: %c%c%c for GFN %"PRIx64" (offset %06"
> -                       PRIx64") gla %016"PRIx64" (vcpu %d)\n",
> +                       PRIx64") gla %016"PRIx64" (valid: %c) fault in gpt: %c fault with gla: %c (vcpu %u)\n",

This patch is fine except that the message punctuation could do with
being slightly improved, I think.  Something like this:

   PRIx64") gla %016"PRIx64" (valid: %c; fault in gpt: %c; fault with gla: %c; vcpu %u)\n",

unless I have misunderstood the nature of these fields.

Thanks,
Ian.

Re: [PATCH v6 4/4] tools/xen-access: Print gla valid/fault information

[Tamas Lengyel]

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

>
> >                  printf("PAGE ACCESS: %c%c%c for GFN %"PRIx64" (offset
> %06"
> > -                       PRIx64") gla %016"PRIx64" (vcpu %d)\n",
> > +                       PRIx64") gla %016"PRIx64" (valid: %c) fault in
> gpt: %c fault with gla: %c (vcpu %u)\n",
>
> This patch is fine except that the message punctuation could do with
> being slightly improved, I think.  Something like this:
>
>    PRIx64") gla %016"PRIx64" (valid: %c; fault in gpt: %c; fault with gla:
> %c; vcpu %u)\n",
>
> unless I have misunderstood the nature of these fields.
>
> Thanks,
> Ian.
>

I agree, that print format may be more intuitive. I will reroll the patch
to include these changes.

Thanks,
Tamas

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

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

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

Re: [PATCH v6 1/4] x86: Consolidate boolean inputs in hvm and p2m into a shared bitmap.

[Tian, Kevin]

> From: Tamas K Lengyel [mailto:tamas.lengyel@zentific.com]
> Sent: Monday, August 11, 2014 7:49 AM
> 
> This patch consolidates the boolean input parameters of
> hvm_hap_nested_page_fault and p2m_mem_access_check into a common
> bitmap and defines the bitmap members accordingly.
> 
> v6: Rename shared structure to "struct npfec" and style fixes.
> v5: Shared structure in mm.h, style fixes and moving gla fault type additions
> into next patch in the series.
> 
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

> ---
>  xen/arch/x86/hvm/hvm.c        | 49
> ++++++++++++++++++++++---------------------
>  xen/arch/x86/hvm/svm/svm.c    | 15 +++++++------
>  xen/arch/x86/hvm/vmx/vmx.c    | 15 ++++++++-----
>  xen/arch/x86/mm/p2m.c         | 18 ++++++++--------
>  xen/include/asm-x86/hvm/hvm.h |  7 ++-----
>  xen/include/asm-x86/mm.h      | 10 +++++++++
>  xen/include/asm-x86/p2m.h     |  6 +++---
>  7 files changed, 68 insertions(+), 52 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index e834406..94a6836 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2722,12 +2722,8 @@ void hvm_inject_page_fault(int errcode, unsigned
> long cr2)
>      hvm_inject_trap(&trap);
>  }
> 
> -int hvm_hap_nested_page_fault(paddr_t gpa,
> -                              bool_t gla_valid,
> -                              unsigned long gla,
> -                              bool_t access_r,
> -                              bool_t access_w,
> -                              bool_t access_x)
> +int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
> +                              struct npfec npfec)
>  {
>      unsigned long gfn = gpa >> PAGE_SHIFT;
>      p2m_type_t p2mt;
> @@ -2756,8 +2752,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>           * into l1 guest if not fixable. The algorithm is
>           * the same as for shadow paging.
>           */
> -        rv = nestedhvm_hap_nested_page_fault(v, &gpa,
> -                                             access_r, access_w,
> access_x);
> +
> +         rv = nestedhvm_hap_nested_page_fault(v, &gpa,
> +                                              npfec.read_access,
> +                                              npfec.write_access,
> +                                              npfec.insn_fetch);
>          switch (rv) {
>          case NESTEDHVM_PAGEFAULT_DONE:
>          case NESTEDHVM_PAGEFAULT_RETRY:
> @@ -2793,47 +2792,49 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
> 
>      p2m = p2m_get_hostp2m(v->domain);
>      mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma,
> -                              P2M_ALLOC | (access_w ?
> P2M_UNSHARE : 0), NULL);
> +                              P2M_ALLOC | npfec.write_access ?
> P2M_UNSHARE : 0,
> +                              NULL);
> 
>      /* Check access permissions first, then handle faults */
>      if ( mfn_x(mfn) != INVALID_MFN )
>      {
> -        int violation = 0;
> +        bool_t violation;
> +
>          /* If the access is against the permissions, then send to mem_event
> */
> -        switch (p2ma)
> +        switch (p2ma)
>          {
>          case p2m_access_n:
>          case p2m_access_n2rwx:
>          default:
> -            violation = access_r || access_w || access_x;
> +            violation = npfec.read_access || npfec.write_access ||
> npfec.insn_fetch;
>              break;
>          case p2m_access_r:
> -            violation = access_w || access_x;
> +            violation = npfec.write_access || npfec.insn_fetch;
>              break;
>          case p2m_access_w:
> -            violation = access_r || access_x;
> +            violation = npfec.read_access || npfec.insn_fetch;
>              break;
>          case p2m_access_x:
> -            violation = access_r || access_w;
> +            violation = npfec.read_access || npfec.write_access;
>              break;
>          case p2m_access_rx:
>          case p2m_access_rx2rw:
> -            violation = access_w;
> +            violation = npfec.write_access;
>              break;
>          case p2m_access_wx:
> -            violation = access_r;
> +            violation = npfec.read_access;
>              break;
>          case p2m_access_rw:
> -            violation = access_x;
> +            violation = npfec.insn_fetch;
>              break;
>          case p2m_access_rwx:
> +            violation = 0;
>              break;
>          }
> 
>          if ( violation )
>          {
> -            if ( p2m_mem_access_check(gpa, gla_valid, gla, access_r,
> -                                        access_w, access_x,
> &req_ptr) )
> +            if ( p2m_mem_access_check(gpa, gla, npfec, &req_ptr) )
>              {
>                  fall_through = 1;
>              } else {
> @@ -2849,7 +2850,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>       * to the mmio handler.
>       */
>      if ( (p2mt == p2m_mmio_dm) ||
> -         (access_w && (p2mt == p2m_ram_ro)) )
> +         (npfec.write_access && (p2mt == p2m_ram_ro)) )
>      {
>          put_gfn(p2m->domain, gfn);
> 
> @@ -2868,7 +2869,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>          paged = 1;
> 
>      /* Mem sharing: unshare the page and try again */
> -    if ( access_w && (p2mt == p2m_ram_shared) )
> +    if ( npfec.write_access && (p2mt == p2m_ram_shared) )
>      {
>          ASSERT(!p2m_is_nestedp2m(p2m));
>          sharing_enomem =
> @@ -2885,7 +2886,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>           * a large page, we do not change other pages type within that
> large
>           * page.
>           */
> -        if ( access_w )
> +        if ( npfec.write_access )
>          {
>              paging_mark_dirty(v->domain, mfn_x(mfn));
>              p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty,
> p2m_ram_rw);
> @@ -2895,7 +2896,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>      }
> 
>      /* Shouldn't happen: Maybe the guest was writing to a r/o grant
> mapping? */
> -    if ( access_w && (p2mt == p2m_grant_map_ro) )
> +    if ( npfec.write_access && (p2mt == p2m_grant_map_ro) )
>      {
>          gdprintk(XENLOG_WARNING,
>                   "trying to write to read-only grant mapping\n");
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 76616ac..1f72e19 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1394,7 +1394,7 @@ const struct hvm_function_table * __init
> start_svm(void)
>  }
> 
>  static void svm_do_nested_pgfault(struct vcpu *v,
> -    struct cpu_user_regs *regs, uint32_t npfec, paddr_t gpa)
> +    struct cpu_user_regs *regs, uint32_t pfec, paddr_t gpa)
>  {
>      int ret;
>      unsigned long gfn = gpa >> PAGE_SHIFT;
> @@ -1403,10 +1403,13 @@ static void svm_do_nested_pgfault(struct vcpu *v,
>      p2m_access_t p2ma;
>      struct p2m_domain *p2m = NULL;
> 
> -    ret = hvm_hap_nested_page_fault(gpa, 0, ~0ul,
> -                                    1, /* All NPFs count as reads */
> -                                    npfec & PFEC_write_access,
> -                                    npfec & PFEC_insn_fetch);
> +    struct npfec npfec = {
> +        .read_access = 1, /* All NPFs count as reads */
> +        .write_access = !!(pfec & PFEC_write_access),
> +        .insn_fetch = !!(pfec & PFEC_insn_fetch)
> +    };
> +
> +    ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec);
> 
>      if ( tb_init_done )
>      {
> @@ -1434,7 +1437,7 @@ static void svm_do_nested_pgfault(struct vcpu *v,
>      case -1:
>          ASSERT(nestedhvm_enabled(v->domain) &&
> nestedhvm_vcpu_in_guestmode(v));
>          /* inject #VMEXIT(NPF) into guest. */
> -        nestedsvm_vmexit_defer(v, VMEXIT_NPF, npfec, gpa);
> +        nestedsvm_vmexit_defer(v, VMEXIT_NPF, pfec, gpa);
>          return;
>      }
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 2caa04a..656ce61 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2353,6 +2353,11 @@ static void ept_handle_violation(unsigned long
> qualification, paddr_t gpa)
>      p2m_type_t p2mt;
>      int ret;
>      struct domain *d = current->domain;
> +    struct npfec npfec = {
> +        .read_access = !!(qualification & EPT_READ_VIOLATION),
> +        .write_access = !!(qualification & EPT_WRITE_VIOLATION),
> +        .insn_fetch = !!(qualification & EPT_EXEC_VIOLATION)
> +    };
> 
>      if ( tb_init_done )
>      {
> @@ -2371,14 +2376,14 @@ static void ept_handle_violation(unsigned long
> qualification, paddr_t gpa)
>      }
> 
>      if ( qualification & EPT_GLA_VALID )
> +    {
>          __vmread(GUEST_LINEAR_ADDRESS, &gla);
> +        npfec.gla_valid = 1;
> +    }
>      else
>          gla = ~0ull;
> -    ret = hvm_hap_nested_page_fault(gpa,
> -                                    !!(qualification &
> EPT_GLA_VALID), gla,
> -                                    !!(qualification &
> EPT_READ_VIOLATION),
> -                                    !!(qualification &
> EPT_WRITE_VIOLATION),
> -                                    !!(qualification &
> EPT_EXEC_VIOLATION));
> +
> +    ret = hvm_hap_nested_page_fault(gpa, gla, npfec);
>      switch ( ret )
>      {
>      case 0:         // Unhandled L1 EPT violation
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index bca9f0f..1f1f6cd 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1323,9 +1323,9 @@ void p2m_mem_paging_resume(struct domain *d)
>      }
>  }
> 
> -bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid, unsigned long
> gla,
> -                          bool_t access_r, bool_t access_w, bool_t
> access_x,
> -                          mem_event_request_t **req_ptr)
> +bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
> +                            struct npfec npfec,
> +                            mem_event_request_t **req_ptr)
>  {
>      struct vcpu *v = current;
>      unsigned long gfn = gpa >> PAGE_SHIFT;
> @@ -1343,7 +1343,7 @@ bool_t p2m_mem_access_check(paddr_t gpa,
> bool_t gla_valid, unsigned long gla,
>      gfn_lock(p2m, gfn, 0);
>      mfn = p2m->get_entry(p2m, gfn, &p2mt, &p2ma, 0, NULL);
> 
> -    if ( access_w && p2ma == p2m_access_rx2rw )
> +    if ( npfec.write_access && p2ma == p2m_access_rx2rw )
>      {
>          rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt,
> p2m_access_rw);
>          ASSERT(rc == 0);
> @@ -1352,7 +1352,7 @@ bool_t p2m_mem_access_check(paddr_t gpa,
> bool_t gla_valid, unsigned long gla,
>      }
>      else if ( p2ma == p2m_access_n2rwx )
>      {
> -        ASSERT(access_w || access_r || access_x);
> +        ASSERT(npfec.write_access || npfec.read_access ||
> npfec.insn_fetch);
>          rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K,
>                              p2mt, p2m_access_rwx);
>          ASSERT(rc == 0);
> @@ -1403,11 +1403,11 @@ bool_t p2m_mem_access_check(paddr_t gpa,
> bool_t gla_valid, unsigned long gla,
>          /* Send request to mem event */
>          req->gfn = gfn;
>          req->offset = gpa & ((1 << PAGE_SHIFT) - 1);
> -        req->gla_valid = gla_valid;
> +        req->gla_valid = npfec.gla_valid;
>          req->gla = gla;
> -        req->access_r = access_r;
> -        req->access_w = access_w;
> -        req->access_x = access_x;
> +        req->access_r = npfec.read_access;
> +        req->access_w = npfec.write_access;
> +        req->access_x = npfec.insn_fetch;
> 
>          req->vcpu_id = v->vcpu_id;
>      }
> diff --git a/xen/include/asm-x86/hvm/hvm.h
> b/xen/include/asm-x86/hvm/hvm.h
> index 0ebd478..1123857 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -455,11 +455,8 @@ static inline void hvm_invalidate_regs_fields(struct
> cpu_user_regs *regs)
>  #endif
>  }
> 
> -int hvm_hap_nested_page_fault(paddr_t gpa,
> -                              bool_t gla_valid, unsigned long gla,
> -                              bool_t access_r,
> -                              bool_t access_w,
> -                              bool_t access_x);
> +int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
> +                              struct npfec npfec);
> 
>  #define hvm_msr_tsc_aux(v)
> ({                                               \
>      struct domain *__d = (v)->domain;
> \
> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> index d253117..1889b25 100644
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -551,6 +551,16 @@ void audit_domains(void);
> 
>  #endif
> 
> +/*
> + * Nested page fault exception codes.
> + */
> +struct npfec {
> +    unsigned int read_access:1;
> +    unsigned int write_access:1;
> +    unsigned int insn_fetch:1;
> +    unsigned int gla_valid:1;
> +};
> +
>  int new_guest_cr3(unsigned long pfn);
>  void make_cr3(struct vcpu *v, unsigned long mfn);
>  void update_cr3(struct vcpu *v);
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 0ddbadb..3975e32 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -597,9 +597,9 @@ void p2m_mem_paging_resume(struct domain *d);
>   * 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. */
> -bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid, unsigned long
> gla,
> -                          bool_t access_r, bool_t access_w, bool_t
> access_x,
> -                          mem_event_request_t **req_ptr);
> +bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
> +                            struct npfec npfec,
> +                            mem_event_request_t **req_ptr);
>  /* Resumes the running of the VCPU, restarting the last instruction */
>  void p2m_mem_access_resume(struct domain *d);
> 
> --
> 2.0.1

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Tian, Kevin]

> From: Tamas K Lengyel [mailto:tamas.lengyel@zentific.com]
> Sent: Monday, August 11, 2014 7:49 AM
> 
> As pointed out by Jan Beulich in
> http://lists.xen.org/archives/html/xen-devel/2014-08/msg01269.html:
> "Read-modify-write instructions absolutely need to be treated as read
> accesses, yet hardware doesn't guarantee to tell us so (they may surface as
> just write accesses)." This patch addresses the issue in both the VMX and the
> SVM side.
> 
> VMX: Treat all non-instruction fetch violations as read violations (in addition to
> those that were already reported as read violations).

so as a result all the write violations are also treated as read violations here? 
it's more than fixing the comment above...

> SVM: Refine the handling to distinguish between read/write and instruction
> fetch access violations.
> 
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
> ---
>  xen/arch/x86/hvm/svm/svm.c | 2 +-
>  xen/arch/x86/hvm/vmx/vmx.c | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 1f72e19..9531248 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1404,7 +1404,7 @@ static void svm_do_nested_pgfault(struct vcpu *v,
>      struct p2m_domain *p2m = NULL;
> 
>      struct npfec npfec = {
> -        .read_access = 1, /* All NPFs count as reads */
> +        .read_access = !(pfec & PFEC_insn_fetch),
>          .write_access = !!(pfec & PFEC_write_access),
>          .insn_fetch = !!(pfec & PFEC_insn_fetch)
>      };
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 656ce61..af0ad7c 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2354,7 +2354,8 @@ static void ept_handle_violation(unsigned long
> qualification, paddr_t gpa)
>      int ret;
>      struct domain *d = current->domain;
>      struct npfec npfec = {
> -        .read_access = !!(qualification & EPT_READ_VIOLATION),
> +        .read_access = !!(qualification & EPT_READ_VIOLATION) ||
> +                        !(qualification & EPT_EXEC_VIOLATION),
>          .write_access = !!(qualification & EPT_WRITE_VIOLATION),
>          .insn_fetch = !!(qualification & EPT_EXEC_VIOLATION)
>      };
> --
> 2.0.1

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Tamas Lengyel]

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

On Thu, Aug 14, 2014 at 2:31 AM, Tian, Kevin <kevin.tian@intel.com> wrote:

> > From: Tamas K Lengyel [mailto:tamas.lengyel@zentific.com]
> > Sent: Monday, August 11, 2014 7:49 AM
> >
> > As pointed out by Jan Beulich in
> > http://lists.xen.org/archives/html/xen-devel/2014-08/msg01269.html:
> > "Read-modify-write instructions absolutely need to be treated as read
> > accesses, yet hardware doesn't guarantee to tell us so (they may surface
> as
> > just write accesses)." This patch addresses the issue in both the VMX
> and the
> > SVM side.
> >
> > VMX: Treat all non-instruction fetch violations as read violations (in
> addition to
> > those that were already reported as read violations).
>
> so as a result all the write violations are also treated as read
> violations here?
> it's more than fixing the comment above...
>

That's correct (all non instruction fetch violation mean all write
violations are reported as read+write violations). Ideally we would be able
to differentiate between write violations that were triggered by
read-modify-write instructions and only mark those as read+write
violations, but as we can't, here we take a safer approach to assume all
write violations to be a read violation as well. It has the added benefit
that now the VMX and SVM side both report violations the same way.


>
> > SVM: Refine the handling to distinguish between read/write and
> instruction
> > fetch access violations.
> >
> > Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
> > ---
> >  xen/arch/x86/hvm/svm/svm.c | 2 +-
> >  xen/arch/x86/hvm/vmx/vmx.c | 3 ++-
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> > index 1f72e19..9531248 100644
> > --- a/xen/arch/x86/hvm/svm/svm.c
> > +++ b/xen/arch/x86/hvm/svm/svm.c
> > @@ -1404,7 +1404,7 @@ static void svm_do_nested_pgfault(struct vcpu *v,
> >      struct p2m_domain *p2m = NULL;
> >
> >      struct npfec npfec = {
> > -        .read_access = 1, /* All NPFs count as reads */
> > +        .read_access = !(pfec & PFEC_insn_fetch),
> >          .write_access = !!(pfec & PFEC_write_access),
> >          .insn_fetch = !!(pfec & PFEC_insn_fetch)
> >      };
> > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> > index 656ce61..af0ad7c 100644
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -2354,7 +2354,8 @@ static void ept_handle_violation(unsigned long
> > qualification, paddr_t gpa)
> >      int ret;
> >      struct domain *d = current->domain;
> >      struct npfec npfec = {
> > -        .read_access = !!(qualification & EPT_READ_VIOLATION),
> > +        .read_access = !!(qualification & EPT_READ_VIOLATION) ||
> > +                        !(qualification & EPT_EXEC_VIOLATION),
> >          .write_access = !!(qualification & EPT_WRITE_VIOLATION),
> >          .insn_fetch = !!(qualification & EPT_EXEC_VIOLATION)
> >      };
> > --
> > 2.0.1
>
>

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

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

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

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Tian, Kevin]

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

but doing so just moves from one incomplete solution (where read-modify-write is not treated as read-violation) to another incomplete solution (where all writes are treated read-violation). If there's actual usage relying on accurate read-violation information, either solution doesn't work. So I don't see the value of this change.

From: Tamas Lengyel [mailto:tamas.lengyel@zentific.com]
Sent: Thursday, August 14, 2014 1:02 AM
To: Tian, Kevin
Cc: xen-devel@lists.xen.org; ian.jackson@eu.citrix.com; stefano.stabellini@eu.citrix.com; ian.campbell@citrix.com; boris.ostrovsky@oracle.com; suravee.suthikulpanit@amd.com; Aravind.Gopalakrishnan@amd.com; Nakajima, Jun; Dong, Eddie; JBeulich@suse.com
Subject: Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

On Thu, Aug 14, 2014 at 2:31 AM, Tian, Kevin <kevin.tian@intel.com<mailto:kevin.tian@intel.com>> wrote:
> From: Tamas K Lengyel [mailto:tamas.lengyel@zentific.com<mailto:tamas.lengyel@zentific.com>]
> Sent: Monday, August 11, 2014 7:49 AM
>
> As pointed out by Jan Beulich in
> http://lists.xen.org/archives/html/xen-devel/2014-08/msg01269.html:
> "Read-modify-write instructions absolutely need to be treated as read
> accesses, yet hardware doesn't guarantee to tell us so (they may surface as
> just write accesses)." This patch addresses the issue in both the VMX and the
> SVM side.
>
> VMX: Treat all non-instruction fetch violations as read violations (in addition to
> those that were already reported as read violations).
so as a result all the write violations are also treated as read violations here?
it's more than fixing the comment above...

That's correct (all non instruction fetch violation mean all write violations are reported as read+write violations). Ideally we would be able to differentiate between write violations that were triggered by read-modify-write instructions and only mark those as read+write violations, but as we can't, here we take a safer approach to assume all write violations to be a read violation as well. It has the added benefit that now the VMX and SVM side both report violations the same way.


> SVM: Refine the handling to distinguish between read/write and instruction
> fetch access violations.
>
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com<mailto:tamas.lengyel@zentific.com>>
> ---
>  xen/arch/x86/hvm/svm/svm.c | 2 +-
>  xen/arch/x86/hvm/vmx/vmx.c | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 1f72e19..9531248 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1404,7 +1404,7 @@ static void svm_do_nested_pgfault(struct vcpu *v,
>      struct p2m_domain *p2m = NULL;
>
>      struct npfec npfec = {
> -        .read_access = 1, /* All NPFs count as reads */
> +        .read_access = !(pfec & PFEC_insn_fetch),
>          .write_access = !!(pfec & PFEC_write_access),
>          .insn_fetch = !!(pfec & PFEC_insn_fetch)
>      };
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 656ce61..af0ad7c 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2354,7 +2354,8 @@ static void ept_handle_violation(unsigned long
> qualification, paddr_t gpa)
>      int ret;
>      struct domain *d = current->domain;
>      struct npfec npfec = {
> -        .read_access = !!(qualification & EPT_READ_VIOLATION),
> +        .read_access = !!(qualification & EPT_READ_VIOLATION) ||
> +                        !(qualification & EPT_EXEC_VIOLATION),
>          .write_access = !!(qualification & EPT_WRITE_VIOLATION),
>          .insn_fetch = !!(qualification & EPT_EXEC_VIOLATION)
>      };
> --
> 2.0.1


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

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

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

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Andrew Cooper]

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

On 14/08/14 17:43, Tian, Kevin wrote:
>
> but doing so just moves from one incomplete solution (where
> read-modify-write is not treated as read-violation) to another
> incomplete solution (where all writes are treated read-violation). If
> there's actual usage relying on accurate read-violation information,
> either solution doesn't work. So I don't see the value of this change.
>

I would agree.  Anything using this information will have to have
detailed knowledge of what the hardware is capable of reporting, to
understand the information it has to hand.

I think Xen should faithfully pass on what hardware reports.  It will be
more useful to the consumer than blurring the details like this.

~Andrew

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

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

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

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Tamas Lengyel]

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

On Thu, Aug 14, 2014 at 6:49 PM, Andrew Cooper <andrew.cooper3@citrix.com>
wrote:

>  On 14/08/14 17:43, Tian, Kevin wrote:
>
>  but doing so just moves from one incomplete solution (where
> read-modify-write is not treated as read-violation) to another incomplete
> solution (where all writes are treated read-violation). If there's actual
> usage relying on accurate read-violation information, either solution
> doesn't work. So I don't see the value of this change.
>
>
> I would agree.  Anything using this information will have to have detailed
> knowledge of what the hardware is capable of reporting, to understand the
> information it has to hand.
>
> I think Xen should faithfully pass on what hardware reports.  It will be
> more useful to the consumer than blurring the details like this.
>
> ~Andrew
>

I can get behind both arguments (including this patch or not) so I leave it
up to the maintainers to decide. This patch itself is not critical for the
rest of the patches in series so it can be omitted if need be.

Thanks,
Tamas

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

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

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

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Jan Beulich]

>>> On 14.08.14 at 18:49, <andrew.cooper3@citrix.com> wrote:
> On 14/08/14 17:43, Tian, Kevin wrote:
>>
>> but doing so just moves from one incomplete solution (where
>> read-modify-write is not treated as read-violation) to another
>> incomplete solution (where all writes are treated read-violation). If
>> there's actual usage relying on accurate read-violation information,
>> either solution doesn't work. So I don't see the value of this change.
>>
> 
> I would agree.  Anything using this information will have to have
> detailed knowledge of what the hardware is capable of reporting, to
> understand the information it has to hand.
> 
> I think Xen should faithfully pass on what hardware reports.  It will be
> more useful to the consumer than blurring the details like this.

Not if it's unreliable. Plus on x86 elsewhere write access implies
read access anyway. If you look at the draft patch I had sent
Tamas (which I intend to rebase on his series), you'll see that
there the change here is actually strictly needed.

Jan

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, August 14, 2014 1:40 PM
> 
> >>> On 14.08.14 at 18:49, <andrew.cooper3@citrix.com> wrote:
> > On 14/08/14 17:43, Tian, Kevin wrote:
> >>
> >> but doing so just moves from one incomplete solution (where
> >> read-modify-write is not treated as read-violation) to another
> >> incomplete solution (where all writes are treated read-violation). If
> >> there's actual usage relying on accurate read-violation information,
> >> either solution doesn't work. So I don't see the value of this change.
> >>
> >
> > I would agree.  Anything using this information will have to have
> > detailed knowledge of what the hardware is capable of reporting, to
> > understand the information it has to hand.
> >
> > I think Xen should faithfully pass on what hardware reports.  It will be
> > more useful to the consumer than blurring the details like this.
> 
> Not if it's unreliable. Plus on x86 elsewhere write access implies
> read access anyway. If you look at the draft patch I had sent
> Tamas (which I intend to rebase on his series), you'll see that
> there the change here is actually strictly needed.
> 

I think you're mixing the behavior and policy here. from behavior p.o.v,
we should keep whatever hardware reports, which describes the behavior
of the instruction causing violation whether it's a write operation or read 
operation. From policy p.o.v, you may treat a write operation as read 
operation in specific code paths (if access==read || access ==write).

Thanks
Kevin

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Jan Beulich]

>>> On 15.08.14 at 00:34, <kevin.tian@intel.com> wrote:
>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Thursday, August 14, 2014 1:40 PM
>> 
>> >>> On 14.08.14 at 18:49, <andrew.cooper3@citrix.com> wrote:
>> > On 14/08/14 17:43, Tian, Kevin wrote:
>> >>
>> >> but doing so just moves from one incomplete solution (where
>> >> read-modify-write is not treated as read-violation) to another
>> >> incomplete solution (where all writes are treated read-violation). If
>> >> there's actual usage relying on accurate read-violation information,
>> >> either solution doesn't work. So I don't see the value of this change.
>> >>
>> >
>> > I would agree.  Anything using this information will have to have
>> > detailed knowledge of what the hardware is capable of reporting, to
>> > understand the information it has to hand.
>> >
>> > I think Xen should faithfully pass on what hardware reports.  It will be
>> > more useful to the consumer than blurring the details like this.
>> 
>> Not if it's unreliable. Plus on x86 elsewhere write access implies
>> read access anyway. If you look at the draft patch I had sent
>> Tamas (which I intend to rebase on his series), you'll see that
>> there the change here is actually strictly needed.
>> 
> 
> I think you're mixing the behavior and policy here. from behavior p.o.v,
> we should keep whatever hardware reports, which describes the behavior
> of the instruction causing violation whether it's a write operation or read 
> operation. From policy p.o.v, you may treat a write operation as read 
> operation in specific code paths (if access==read || access ==write).

No - the hardware specifically does _not_ guarantee to report the
actual characteristics of a read-modify-write instruction. Or at least
that's what your documentation warns about. And to be on the safe
side, treating all writes as also being reads is the better option than
to mistakenly treat r-m-w as just w.

Jan

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, August 14, 2014 4:00 PM
> 
> >>> On 15.08.14 at 00:34, <kevin.tian@intel.com> wrote:
> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Thursday, August 14, 2014 1:40 PM
> >>
> >> >>> On 14.08.14 at 18:49, <andrew.cooper3@citrix.com> wrote:
> >> > On 14/08/14 17:43, Tian, Kevin wrote:
> >> >>
> >> >> but doing so just moves from one incomplete solution (where
> >> >> read-modify-write is not treated as read-violation) to another
> >> >> incomplete solution (where all writes are treated read-violation). If
> >> >> there's actual usage relying on accurate read-violation information,
> >> >> either solution doesn't work. So I don't see the value of this change.
> >> >>
> >> >
> >> > I would agree.  Anything using this information will have to have
> >> > detailed knowledge of what the hardware is capable of reporting, to
> >> > understand the information it has to hand.
> >> >
> >> > I think Xen should faithfully pass on what hardware reports.  It will be
> >> > more useful to the consumer than blurring the details like this.
> >>
> >> Not if it's unreliable. Plus on x86 elsewhere write access implies
> >> read access anyway. If you look at the draft patch I had sent
> >> Tamas (which I intend to rebase on his series), you'll see that
> >> there the change here is actually strictly needed.
> >>
> >
> > I think you're mixing the behavior and policy here. from behavior p.o.v,
> > we should keep whatever hardware reports, which describes the behavior
> > of the instruction causing violation whether it's a write operation or read
> > operation. From policy p.o.v, you may treat a write operation as read
> > operation in specific code paths (if access==read || access ==write).
> 
> No - the hardware specifically does _not_ guarantee to report the
> actual characteristics of a read-modify-write instruction. Or at least
> that's what your documentation warns about. And to be on the safe
> side, treating all writes as also being reads is the better option than
> to mistakenly treat r-m-w as just w.
> 

but then you are mistakenly treating all other writes as reads too...

Thanks
Kevin

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Jan Beulich]

>>> On 15.08.14 at 01:04, <kevin.tian@intel.com> wrote:
>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Thursday, August 14, 2014 4:00 PM
>> 
>> >>> On 15.08.14 at 00:34, <kevin.tian@intel.com> wrote:
>> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> Sent: Thursday, August 14, 2014 1:40 PM
>> >>
>> >> >>> On 14.08.14 at 18:49, <andrew.cooper3@citrix.com> wrote:
>> >> > On 14/08/14 17:43, Tian, Kevin wrote:
>> >> >>
>> >> >> but doing so just moves from one incomplete solution (where
>> >> >> read-modify-write is not treated as read-violation) to another
>> >> >> incomplete solution (where all writes are treated read-violation). If
>> >> >> there's actual usage relying on accurate read-violation information,
>> >> >> either solution doesn't work. So I don't see the value of this change.
>> >> >>
>> >> >
>> >> > I would agree.  Anything using this information will have to have
>> >> > detailed knowledge of what the hardware is capable of reporting, to
>> >> > understand the information it has to hand.
>> >> >
>> >> > I think Xen should faithfully pass on what hardware reports.  It will be
>> >> > more useful to the consumer than blurring the details like this.
>> >>
>> >> Not if it's unreliable. Plus on x86 elsewhere write access implies
>> >> read access anyway. If you look at the draft patch I had sent
>> >> Tamas (which I intend to rebase on his series), you'll see that
>> >> there the change here is actually strictly needed.
>> >>
>> >
>> > I think you're mixing the behavior and policy here. from behavior p.o.v,
>> > we should keep whatever hardware reports, which describes the behavior
>> > of the instruction causing violation whether it's a write operation or read
>> > operation. From policy p.o.v, you may treat a write operation as read
>> > operation in specific code paths (if access==read || access ==write).
>> 
>> No - the hardware specifically does _not_ guarantee to report the
>> actual characteristics of a read-modify-write instruction. Or at least
>> that's what your documentation warns about. And to be on the safe
>> side, treating all writes as also being reads is the better option than
>> to mistakenly treat r-m-w as just w.
>> 
> 
> but then you are mistakenly treating all other writes as reads too...

Right, but as said - that's the more safe of the two alternatives.

Jan

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, August 14, 2014 4:08 PM
> 
> >>> On 15.08.14 at 01:04, <kevin.tian@intel.com> wrote:
> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Thursday, August 14, 2014 4:00 PM
> >>
> >> >>> On 15.08.14 at 00:34, <kevin.tian@intel.com> wrote:
> >> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >> Sent: Thursday, August 14, 2014 1:40 PM
> >> >>
> >> >> >>> On 14.08.14 at 18:49, <andrew.cooper3@citrix.com> wrote:
> >> >> > On 14/08/14 17:43, Tian, Kevin wrote:
> >> >> >>
> >> >> >> but doing so just moves from one incomplete solution (where
> >> >> >> read-modify-write is not treated as read-violation) to another
> >> >> >> incomplete solution (where all writes are treated read-violation). If
> >> >> >> there's actual usage relying on accurate read-violation information,
> >> >> >> either solution doesn't work. So I don't see the value of this change.
> >> >> >>
> >> >> >
> >> >> > I would agree.  Anything using this information will have to have
> >> >> > detailed knowledge of what the hardware is capable of reporting, to
> >> >> > understand the information it has to hand.
> >> >> >
> >> >> > I think Xen should faithfully pass on what hardware reports.  It will be
> >> >> > more useful to the consumer than blurring the details like this.
> >> >>
> >> >> Not if it's unreliable. Plus on x86 elsewhere write access implies
> >> >> read access anyway. If you look at the draft patch I had sent
> >> >> Tamas (which I intend to rebase on his series), you'll see that
> >> >> there the change here is actually strictly needed.
> >> >>
> >> >
> >> > I think you're mixing the behavior and policy here. from behavior p.o.v,
> >> > we should keep whatever hardware reports, which describes the behavior
> >> > of the instruction causing violation whether it's a write operation or read
> >> > operation. From policy p.o.v, you may treat a write operation as read
> >> > operation in specific code paths (if access==read || access ==write).
> >>
> >> No - the hardware specifically does _not_ guarantee to report the
> >> actual characteristics of a read-modify-write instruction. Or at least
> >> that's what your documentation warns about. And to be on the safe
> >> side, treating all writes as also being reads is the better option than
> >> to mistakenly treat r-m-w as just w.
> >>
> >
> > but then you are mistakenly treating all other writes as reads too...
> 
> Right, but as said - that's the more safe of the two alternatives.
> 

to decide which one is more 'safe', could you give some examples of
how read violation is used today? If there's an usage very relying on 
the exact read behavior, treating 'w' as 'r' could lead to significant
difference. 

Thanks
Kevin

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Jan Beulich]

>>> On 15.08.14 at 01:20, <kevin.tian@intel.com> wrote:
>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Thursday, August 14, 2014 4:08 PM
>> 
>> >>> On 15.08.14 at 01:04, <kevin.tian@intel.com> wrote:
>> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> Sent: Thursday, August 14, 2014 4:00 PM
>> >>
>> >> >>> On 15.08.14 at 00:34, <kevin.tian@intel.com> wrote:
>> >> > I think you're mixing the behavior and policy here. from behavior p.o.v,
>> >> > we should keep whatever hardware reports, which describes the behavior
>> >> > of the instruction causing violation whether it's a write operation or read
>> >> > operation. From policy p.o.v, you may treat a write operation as read
>> >> > operation in specific code paths (if access==read || access ==write).
>> >>
>> >> No - the hardware specifically does _not_ guarantee to report the
>> >> actual characteristics of a read-modify-write instruction. Or at least
>> >> that's what your documentation warns about. And to be on the safe
>> >> side, treating all writes as also being reads is the better option than
>> >> to mistakenly treat r-m-w as just w.
>> >>
>> >
>> > but then you are mistakenly treating all other writes as reads too...
>> 
>> Right, but as said - that's the more safe of the two alternatives.
>> 
> 
> to decide which one is more 'safe', could you give some examples of
> how read violation is used today? If there's an usage very relying on 
> the exact read behavior, treating 'w' as 'r' could lead to significant
> difference. 

The intended use is that pending patch I referred to, aiming at
reducing the number of GLA->GPA translations we're doing during
instruction emulation, which makes a severe difference to many-
vCPU guests e.g. heavily using the HPET from all their vCPU-s.

Jan

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Tian, Kevin]

[-- Attachment #1: Type: text/plain, Size: 2051 bytes --]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, August 14, 2014 4:37 PM
> 
> >>> On 15.08.14 at 01:20, <kevin.tian@intel.com> wrote:
> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Thursday, August 14, 2014 4:08 PM
> >>
> >> >>> On 15.08.14 at 01:04, <kevin.tian@intel.com> wrote:
> >> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >> Sent: Thursday, August 14, 2014 4:00 PM
> >> >>
> >> >> >>> On 15.08.14 at 00:34, <kevin.tian@intel.com> wrote:
> >> >> > I think you're mixing the behavior and policy here. from behavior p.o.v,
> >> >> > we should keep whatever hardware reports, which describes the
> behavior
> >> >> > of the instruction causing violation whether it's a write operation or
> read
> >> >> > operation. From policy p.o.v, you may treat a write operation as read
> >> >> > operation in specific code paths (if access==read || access ==write).
> >> >>
> >> >> No - the hardware specifically does _not_ guarantee to report the
> >> >> actual characteristics of a read-modify-write instruction. Or at least
> >> >> that's what your documentation warns about. And to be on the safe
> >> >> side, treating all writes as also being reads is the better option than
> >> >> to mistakenly treat r-m-w as just w.
> >> >>
> >> >
> >> > but then you are mistakenly treating all other writes as reads too...
> >>
> >> Right, but as said - that's the more safe of the two alternatives.
> >>
> >
> > to decide which one is more 'safe', could you give some examples of
> > how read violation is used today? If there's an usage very relying on
> > the exact read behavior, treating 'w' as 'r' could lead to significant
> > difference.
> 
> The intended use is that pending patch I referred to, aiming at
> reducing the number of GLA->GPA translations we're doing during
> instruction emulation, which makes a severe difference to many-
> vCPU guests e.g. heavily using the HPET from all their vCPU-s.
> 

Is attached patch what you're talking here?

Thanks,
Kevin

[-- Attachment #2: EPT-fault-GLA.patch --]
[-- Type: application/octet-stream, Size: 17234 bytes --]

TODO: remove //temp-s

There are a few intentional but not necessarily obvious (and possibly
subtle) adjustments to behavior:
- VMX now marks all write access faults also as read accesses (whether
  the hardware would do so for read-modify-write instructions depends
  on CPU model and the particular instruction)
- SVM no longer marks instruction fetch faults as read accesses
- __hvmemul_read() no longer blindly bails on instruction fetches
  matching the MMIO GVA (the callers of handle_mmio_with_translation()
  now control the behavior via the NPFEC_* flags they pass, and it
  didn't seem right to bail here rather than just falling through to
  the unaccelerated path)

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -462,6 +462,7 @@ static int __hvmemul_read(
     struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io;
     paddr_t gpa;
     int rc;
+static unsigned long tot, cnt, thr, xlat;//temp
 
     rc = hvmemul_virtual_to_linear(
         seg, offset, bytes, &reps, access_type, hvmemul_ctxt, &addr);
@@ -481,11 +482,16 @@ static int __hvmemul_read(
         while ( off & (chunk - 1) )
             chunk >>= 1;
 
-    if ( unlikely(vio->mmio_gva == (addr & PAGE_MASK)) && vio->mmio_gva )
+++tot;//temp
+    if ( (vio->mmio_access & (access_type != hvm_access_insn_fetch ?
+                              NPFEC_read_access : NPFEC_insn_fetch)) &&
+         (vio->mmio_gva == (addr & PAGE_MASK)) )
     {
-        if ( access_type == hvm_access_insn_fetch )
-            return X86EMUL_UNHANDLEABLE;
         gpa = (((paddr_t)vio->mmio_gpfn << PAGE_SHIFT) | off);
+if(++cnt > thr) {//temp
+ thr |= cnt;
+ printk("read %lx/%lx/%lx [%pv:%lx:%x]\n", cnt, tot, xlat, curr, gpa, bytes);
+}
         while ( (off + chunk) <= PAGE_SIZE )
         {
             rc = hvmemul_do_mmio(gpa, &reps, chunk, 0, IOREQ_READ, 0, p_data);
@@ -522,6 +528,7 @@ static int __hvmemul_read(
                                     hvmemul_ctxt);
         while ( rc == X86EMUL_OKAY )
         {
+++xlat;//temp
             rc = hvmemul_do_mmio(gpa, &reps, chunk, 0, IOREQ_READ, 0, p_data);
             if ( rc != X86EMUL_OKAY || bytes == chunk )
                 break;
@@ -535,6 +542,7 @@ static int __hvmemul_read(
                 gpa += chunk;
             else
             {
+++xlat;//temp
                 rc = hvmemul_linear_to_phys(addr, &gpa, chunk, &reps, pfec,
                                             hvmemul_ctxt);
                 off = 0;
@@ -611,6 +619,7 @@ static int hvmemul_write(
     struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io;
     paddr_t gpa;
     int rc;
+static unsigned long tot, cnt, thr, xlat;//temp
 
     rc = hvmemul_virtual_to_linear(
         seg, offset, bytes, &reps, hvm_access_write, hvmemul_ctxt, &addr);
@@ -624,9 +633,15 @@ static int hvmemul_write(
         while ( off & (chunk - 1) )
             chunk >>= 1;
 
-    if ( unlikely(vio->mmio_gva == (addr & PAGE_MASK)) && vio->mmio_gva )
+++tot;//temp
+    if ( (vio->mmio_access & NPFEC_write_access) &&
+         (vio->mmio_gva == (addr & PAGE_MASK)) )
     {
         gpa = (((paddr_t)vio->mmio_gpfn << PAGE_SHIFT) | off);
+if(++cnt > thr) {//temp
+ thr |= cnt;
+ printk("write %lx/%lx/%lx [%pv:%lx:%x]\n", cnt, tot, xlat, curr, gpa, bytes);
+}
         while ( (off + chunk) <= PAGE_SIZE )
         {
             rc = hvmemul_do_mmio(gpa, &reps, chunk, 0, IOREQ_WRITE, 0, p_data);
@@ -655,6 +670,7 @@ static int hvmemul_write(
     case HVMCOPY_bad_gva_to_gfn:
         return X86EMUL_EXCEPTION;
     case HVMCOPY_bad_gfn_to_mfn:
+++xlat;//temp
         rc = hvmemul_linear_to_phys(addr, &gpa, chunk, &reps, pfec,
                                     hvmemul_ctxt);
         while ( rc == X86EMUL_OKAY )
@@ -672,6 +688,7 @@ static int hvmemul_write(
                 gpa += chunk;
             else
             {
+++xlat;//temp
                 rc = hvmemul_linear_to_phys(addr, &gpa, chunk, &reps, pfec,
                                             hvmemul_ctxt);
                 off = 0;
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2684,11 +2684,8 @@ void hvm_inject_page_fault(int errcode, 
 }
 
 int hvm_hap_nested_page_fault(paddr_t gpa,
-                              bool_t gla_valid,
-                              unsigned long gla,
-                              bool_t access_r,
-                              bool_t access_w,
-                              bool_t access_x)
+                              unsigned int npfec,
+                              unsigned long gla)
 {
     unsigned long gfn = gpa >> PAGE_SHIFT;
     p2m_type_t p2mt;
@@ -2718,7 +2715,9 @@ int hvm_hap_nested_page_fault(paddr_t gp
          * the same as for shadow paging.
          */
         rv = nestedhvm_hap_nested_page_fault(v, &gpa,
-                                             access_r, access_w, access_x);
+                                             !!(npfec & NPFEC_read_access),
+                                             !!(npfec & NPFEC_write_access),
+                                             !!(npfec & NPFEC_insn_fetch));
         switch (rv) {
         case NESTEDHVM_PAGEFAULT_DONE:
         case NESTEDHVM_PAGEFAULT_RETRY:
@@ -2749,7 +2748,7 @@ int hvm_hap_nested_page_fault(paddr_t gp
          && is_hvm_vcpu(v)
          && hvm_mmio_internal(gpa) )
     {
-        if ( !handle_mmio() )
+        if ( !handle_mmio_with_translation(gla, gpa >> PAGE_SHIFT, npfec) )
             hvm_inject_hw_exception(TRAP_gp_fault, 0);
         rc = 1;
         goto out;
@@ -2757,7 +2756,9 @@ int hvm_hap_nested_page_fault(paddr_t gp
 
     p2m = p2m_get_hostp2m(v->domain);
     mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma, 
-                              P2M_ALLOC | (access_w ? P2M_UNSHARE : 0), NULL);
+                              P2M_ALLOC | (npfec & NPFEC_write_access ?
+                                           P2M_UNSHARE : 0),
+                              NULL);
 
     /* Check access permissions first, then handle faults */
     if ( mfn_x(mfn) != INVALID_MFN )
@@ -2769,26 +2770,27 @@ int hvm_hap_nested_page_fault(paddr_t gp
         case p2m_access_n:
         case p2m_access_n2rwx:
         default:
-            violation = access_r || access_w || access_x;
+            violation = !!(npfec & (NPFEC_read_access | NPFEC_write_access |
+                                    NPFEC_insn_fetch));
             break;
         case p2m_access_r:
-            violation = access_w || access_x;
+            violation = !!(npfec & (NPFEC_write_access | NPFEC_insn_fetch));
             break;
         case p2m_access_w:
-            violation = access_r || access_x;
+            violation = !!(npfec & (NPFEC_read_access | NPFEC_insn_fetch));
             break;
         case p2m_access_x:
-            violation = access_r || access_w;
+            violation = !!(npfec & (NPFEC_read_access | NPFEC_write_access));
             break;
         case p2m_access_rx:
         case p2m_access_rx2rw:
-            violation = access_w;
+            violation = !!(npfec & NPFEC_write_access);
             break;
         case p2m_access_wx:
-            violation = access_r;
+            violation = !!(npfec & NPFEC_read_access);
             break;
         case p2m_access_rw:
-            violation = access_x;
+            violation = !!(npfec & NPFEC_insn_fetch);
             break;
         case p2m_access_rwx:
             break;
@@ -2796,8 +2798,11 @@ int hvm_hap_nested_page_fault(paddr_t gp
 
         if ( violation )
         {
-            if ( p2m_mem_access_check(gpa, gla_valid, gla, access_r, 
-                                        access_w, access_x, &req_ptr) )
+            if ( p2m_mem_access_check(gpa, !!(npfec & NPFEC_gla_valid), gla,
+                                      !!(npfec & NPFEC_read_access),
+                                      !!(npfec & NPFEC_write_access),
+                                      !!(npfec & NPFEC_insn_fetch),
+                                      &req_ptr) )
             {
                 fall_through = 1;
             } else {
@@ -2813,7 +2818,7 @@ int hvm_hap_nested_page_fault(paddr_t gp
      * to the mmio handler.
      */
     if ( (p2mt == p2m_mmio_dm) || 
-         (access_w && (p2mt == p2m_ram_ro)) )
+         ((npfec & NPFEC_write_access) && (p2mt == p2m_ram_ro)) )
     {
         put_gfn(p2m->domain, gfn);
 
@@ -2821,7 +2826,7 @@ int hvm_hap_nested_page_fault(paddr_t gp
         if ( unlikely(is_pvh_vcpu(v)) )
             goto out;
 
-        if ( !handle_mmio() )
+        if ( !handle_mmio_with_translation(gla, gpa >> PAGE_SHIFT, npfec) )
             hvm_inject_hw_exception(TRAP_gp_fault, 0);
         rc = 1;
         goto out;
@@ -2832,7 +2837,7 @@ int hvm_hap_nested_page_fault(paddr_t gp
         paged = 1;
 
     /* Mem sharing: unshare the page and try again */
-    if ( access_w && (p2mt == p2m_ram_shared) )
+    if ( (npfec & NPFEC_write_access) && (p2mt == p2m_ram_shared) )
     {
         ASSERT(!p2m_is_nestedp2m(p2m));
         sharing_enomem = 
@@ -2849,7 +2854,7 @@ int hvm_hap_nested_page_fault(paddr_t gp
          * a large page, we do not change other pages type within that large
          * page.
          */
-        if ( access_w )
+        if ( npfec & NPFEC_write_access )
         {
             paging_mark_dirty(v->domain, mfn_x(mfn));
             p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty, p2m_ram_rw);
@@ -2859,7 +2864,7 @@ int hvm_hap_nested_page_fault(paddr_t gp
     }
 
     /* Shouldn't happen: Maybe the guest was writing to a r/o grant mapping? */
-    if ( access_w && (p2mt == p2m_grant_map_ro) )
+    if ( (npfec & NPFEC_write_access) && (p2mt == p2m_grant_map_ro) )
     {
         gdprintk(XENLOG_WARNING,
                  "trying to write to read-only grant mapping\n");
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -95,7 +95,7 @@ int handle_mmio(void)
     if ( vio->io_state == HVMIO_awaiting_completion )
         vio->io_state = HVMIO_handle_mmio_awaiting_completion;
     else
-        vio->mmio_gva = 0;
+        vio->mmio_access = 0;
 
     switch ( rc )
     {
@@ -124,9 +124,12 @@ int handle_mmio(void)
     return 1;
 }
 
-int handle_mmio_with_translation(unsigned long gva, unsigned long gpfn)
+int handle_mmio_with_translation(unsigned long gva, unsigned long gpfn,
+                                 unsigned int access)
 {
     struct hvm_vcpu_io *vio = &current->arch.hvm_vcpu.hvm_io;
+
+    vio->mmio_access = access & NPFEC_gla_translation ? access : 0;
     vio->mmio_gva = gva & PAGE_MASK;
     vio->mmio_gpfn = gpfn;
     return handle_mmio();
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1396,18 +1396,13 @@ const struct hvm_function_table * __init
 static void svm_do_nested_pgfault(struct vcpu *v,
     struct cpu_user_regs *regs, uint32_t npfec, paddr_t gpa)
 {
-    int ret;
+    unsigned int hapec;
     unsigned long gfn = gpa >> PAGE_SHIFT;
     mfn_t mfn;
     p2m_type_t p2mt;
     p2m_access_t p2ma;
     struct p2m_domain *p2m = NULL;
 
-    ret = hvm_hap_nested_page_fault(gpa, 0, ~0ul, 
-                                    1, /* All NPFs count as reads */
-                                    npfec & PFEC_write_access, 
-                                    npfec & PFEC_insn_fetch);
-
     if ( tb_init_done )
     {
         struct {
@@ -1426,7 +1421,12 @@ static void svm_do_nested_pgfault(struct
         __trace_var(TRC_HVM_NPF, 0, sizeof(_d), &_d);
     }
 
-    switch (ret) {
+    hapec = npfec & PFEC_insn_fetch ? NPFEC_insn_fetch : NPFEC_read_access;
+    if ( npfec & PFEC_write_access )
+        hapec |= NPFEC_write_access;
+
+    switch ( hvm_hap_nested_page_fault(gpa, hapec, ~0ul) )
+    {
     case 0:
         break;
     case 1:
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2351,7 +2351,7 @@ static void ept_handle_violation(unsigne
     unsigned long gla, gfn = gpa >> PAGE_SHIFT;
     mfn_t mfn;
     p2m_type_t p2mt;
-    int ret;
+    unsigned int npfec = 0;
     struct domain *d = current->domain;
 
     if ( tb_init_done )
@@ -2370,16 +2370,23 @@ static void ept_handle_violation(unsigne
         __trace_var(TRC_HVM_NPF, 0, sizeof(_d), &_d);
     }
 
+    if ( qualification & EPT_WRITE_VIOLATION )
+        npfec |= NPFEC_read_access | NPFEC_write_access;
+    else if ( qualification & EPT_READ_VIOLATION )
+        npfec |= NPFEC_read_access;
+    if ( qualification & EPT_EXEC_VIOLATION )
+        npfec |= NPFEC_insn_fetch;
     if ( qualification & EPT_GLA_VALID )
+    {
         __vmread(GUEST_LINEAR_ADDRESS, &gla);
+        npfec |= NPFEC_gla_valid;
+        if ( qualification & EPT_GLA_FAULT )
+            npfec |= NPFEC_gla_translation;
+    }
     else
         gla = ~0ull;
-    ret = hvm_hap_nested_page_fault(gpa,
-                                    !!(qualification & EPT_GLA_VALID), gla,
-                                    !!(qualification & EPT_READ_VIOLATION),
-                                    !!(qualification & EPT_WRITE_VIOLATION),
-                                    !!(qualification & EPT_EXEC_VIOLATION));
-    switch ( ret )
+
+    switch ( hvm_hap_nested_page_fault(gpa, npfec, gla) )
     {
     case 0:         // Unhandled L1 EPT violation
         break;
@@ -2412,7 +2419,8 @@ static void ept_handle_violation(unsigne
     ept_walk_table(d, gfn);
 
     if ( qualification & EPT_GLA_VALID )
-        gdprintk(XENLOG_ERR, " --- GLA %#lx\n", gla);
+        gdprintk(XENLOG_ERR, " --- GLA %#lx (%s)\n", gla,
+                 qualification & EPT_GLA_FAULT ? "translation" : "walk");
 
     domain_crash(d);
 }
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -2824,6 +2824,8 @@ static int sh_page_fault(struct vcpu *v,
     p2m_type_t p2mt;
     uint32_t rc;
     int version;
+    unsigned int npfec = NPFEC_read_access | NPFEC_gla_valid |
+                         NPFEC_gla_translation;
 #if SHADOW_OPTIMIZATIONS & SHOPT_FAST_EMULATION
     int fast_emul = 0;
 #endif
@@ -2834,6 +2836,9 @@ static int sh_page_fault(struct vcpu *v,
 
     perfc_incr(shadow_fault);
 
+    if ( regs->error_code & PFEC_write_access )
+        npfec |= NPFEC_write_access;
+
 #if SHADOW_OPTIMIZATIONS & SHOPT_FAST_EMULATION
     /* If faulting frame is successfully emulated in last shadow fault
      * it's highly likely to reach same emulation action for this frame.
@@ -2935,7 +2940,7 @@ static int sh_page_fault(struct vcpu *v,
             SHADOW_PRINTK("fast path mmio %#"PRIpaddr"\n", gpa);
             reset_early_unshadow(v);
             trace_shadow_gen(TRC_SHADOW_FAST_MMIO, va);
-            return (handle_mmio_with_translation(va, gpa >> PAGE_SHIFT)
+            return (handle_mmio_with_translation(va, gpa >> PAGE_SHIFT, npfec)
                     ? EXCRET_fault_fixed : 0);
         }
         else
@@ -3424,7 +3429,7 @@ static int sh_page_fault(struct vcpu *v,
     paging_unlock(d);
     put_gfn(d, gfn_x(gfn));
     trace_shadow_gen(TRC_SHADOW_MMIO, va);
-    return (handle_mmio_with_translation(va, gpa >> PAGE_SHIFT)
+    return (handle_mmio_with_translation(va, gpa >> PAGE_SHIFT, npfec)
             ? EXCRET_fault_fixed : 0);
 
  not_a_shadow_fault:
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -455,11 +455,14 @@ static inline void hvm_invalidate_regs_f
 #endif
 }
 
-int hvm_hap_nested_page_fault(paddr_t gpa,
-                              bool_t gla_valid, unsigned long gla,
-                              bool_t access_r,
-                              bool_t access_w,
-                              bool_t access_x);
+int hvm_hap_nested_page_fault(paddr_t gpa, unsigned int npfec,
+                              unsigned long gla);
+
+#define NPFEC_read_access     (1U << 0)
+#define NPFEC_write_access    (1U << 1)
+#define NPFEC_insn_fetch      (1U << 2)
+#define NPFEC_gla_valid       (1U << 3)
+#define NPFEC_gla_translation (1U << 4)
 
 #define hvm_msr_tsc_aux(v) ({                                               \
     struct domain *__d = (v)->domain;                                       \
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -119,7 +119,8 @@ static inline void register_buffered_io_
 void send_timeoffset_req(unsigned long timeoff);
 void send_invalidate_req(void);
 int handle_mmio(void);
-int handle_mmio_with_translation(unsigned long gva, unsigned long gpfn);
+int handle_mmio_with_translation(unsigned long gva, unsigned long gpfn,
+                                 unsigned int access);
 int handle_pio(uint16_t port, unsigned int size, int dir);
 void hvm_interrupt_post(struct vcpu *v, int vector, int type);
 void hvm_io_assist(ioreq_t *p);
--- a/xen/include/asm-x86/hvm/vcpu.h
+++ b/xen/include/asm-x86/hvm/vcpu.h
@@ -54,8 +54,9 @@ struct hvm_vcpu_io {
      * HVM emulation:
      *  Virtual address @mmio_gva maps to MMIO physical frame @mmio_gpfn.
      *  The latter is known to be an MMIO frame (not RAM).
-     *  This translation is only valid if @mmio_gva is non-zero.
+     *  This translation is only valid for accesses as per @mmio_access.
      */
+    unsigned int        mmio_access; /* using NPFEC_* values */
     unsigned long       mmio_gva;
     unsigned long       mmio_gpfn;
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Boris Ostrovsky]

On 08/14/2014 06:59 PM, Jan Beulich wrote:
>>>> On 15.08.14 at 00:34, <kevin.tian@intel.com> wrote:
>>>   From: Jan Beulich [mailto:JBeulich@suse.com]
>>> Sent: Thursday, August 14, 2014 1:40 PM
>>>
>>>>>> On 14.08.14 at 18:49, <andrew.cooper3@citrix.com> wrote:
>>>> On 14/08/14 17:43, Tian, Kevin wrote:
>>>>> but doing so just moves from one incomplete solution (where
>>>>> read-modify-write is not treated as read-violation) to another
>>>>> incomplete solution (where all writes are treated read-violation). If
>>>>> there's actual usage relying on accurate read-violation information,
>>>>> either solution doesn't work. So I don't see the value of this change.
>>>>>
>>>> I would agree.  Anything using this information will have to have
>>>> detailed knowledge of what the hardware is capable of reporting, to
>>>> understand the information it has to hand.
>>>>
>>>> I think Xen should faithfully pass on what hardware reports.  It will be
>>>> more useful to the consumer than blurring the details like this.
>>> Not if it's unreliable. Plus on x86 elsewhere write access implies
>>> read access anyway. If you look at the draft patch I had sent
>>> Tamas (which I intend to rebase on his series), you'll see that
>>> there the change here is actually strictly needed.
>>>
>> I think you're mixing the behavior and policy here. from behavior p.o.v,
>> we should keep whatever hardware reports, which describes the behavior
>> of the instruction causing violation whether it's a write operation or read
>> operation. From policy p.o.v, you may treat a write operation as read
>> operation in specific code paths (if access==read || access ==write).
> No - the hardware specifically does _not_ guarantee to report the
> actual characteristics of a read-modify-write instruction. Or at least
> that's what your documentation warns about. And to be on the safe
> side, treating all writes as also being reads is the better option than
> to mistakenly treat r-m-w as just w.

Is this specific to VMX or does SVM have the same problem (I am not 
aware of this but I might be wrong). Because if it doesn't then I think 
Tamas' [PATCH v6 2/4] should have SVM report actual bits.

If, OTOH, you need both return same results for consistency then I 
wonder whether we could move this up the stack into HVM common code.

-boris

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Jan Beulich]

>>> On 15.08.14 at 01:40, <kevin.tian@intel.com> wrote:
>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> The intended use is that pending patch I referred to, aiming at
>> reducing the number of GLA->GPA translations we're doing during
>> instruction emulation, which makes a severe difference to many-
>> vCPU guests e.g. heavily using the HPET from all their vCPU-s.
> 
> Is attached patch what you're talking here?

Yes, that one.

Jan

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Jan Beulich]

>>> On 15.08.14 at 16:31, <boris.ostrovsky@oracle.com> wrote:
> On 08/14/2014 06:59 PM, Jan Beulich wrote:
>> No - the hardware specifically does _not_ guarantee to report the
>> actual characteristics of a read-modify-write instruction. Or at least
>> that's what your documentation warns about. And to be on the safe
>> side, treating all writes as also being reads is the better option than
>> to mistakenly treat r-m-w as just w.
> 
> Is this specific to VMX or does SVM have the same problem (I am not 
> aware of this but I might be wrong). Because if it doesn't then I think 
> Tamas' [PATCH v6 2/4] should have SVM report actual bits.

You as the SVM maintainer should know better than me... With
NPT using "normal" page fault error codes, there is not even an
indication for read access. Tamas's patches adjust the current
misbehavior too in that at least instruction fetches no longer get
reported as reads.

Jan

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Boris Ostrovsky]

On 08/15/2014 11:01 AM, Jan Beulich wrote:
>>>> On 15.08.14 at 16:31, <boris.ostrovsky@oracle.com> wrote:
>> On 08/14/2014 06:59 PM, Jan Beulich wrote:
>>> No - the hardware specifically does _not_ guarantee to report the
>>> actual characteristics of a read-modify-write instruction. Or at least
>>> that's what your documentation warns about. And to be on the safe
>>> side, treating all writes as also being reads is the better option than
>>> to mistakenly treat r-m-w as just w.
>> Is this specific to VMX or does SVM have the same problem (I am not
>> aware of this but I might be wrong). Because if it doesn't then I think
>> Tamas' [PATCH v6 2/4] should have SVM report actual bits.
> You as the SVM maintainer should know better than me... With
> NPT using "normal" page fault error codes, there is not even an
> indication for read access. Tamas's patches adjust the current
> misbehavior too in that at least instruction fetches no longer get
> reported as reads.

What I am asking is whether

    .read_access = !(pfec & (PFEC_insn_fetch | PFEC_write_access))

would be more appropriate.

-boris

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Jan Beulich]

>>> On 15.08.14 at 17:09, <boris.ostrovsky@oracle.com> wrote:
> On 08/15/2014 11:01 AM, Jan Beulich wrote:
>>>>> On 15.08.14 at 16:31, <boris.ostrovsky@oracle.com> wrote:
>>> On 08/14/2014 06:59 PM, Jan Beulich wrote:
>>>> No - the hardware specifically does _not_ guarantee to report the
>>>> actual characteristics of a read-modify-write instruction. Or at least
>>>> that's what your documentation warns about. And to be on the safe
>>>> side, treating all writes as also being reads is the better option than
>>>> to mistakenly treat r-m-w as just w.
>>> Is this specific to VMX or does SVM have the same problem (I am not
>>> aware of this but I might be wrong). Because if it doesn't then I think
>>> Tamas' [PATCH v6 2/4] should have SVM report actual bits.
>> You as the SVM maintainer should know better than me... With
>> NPT using "normal" page fault error codes, there is not even an
>> indication for read access. Tamas's patches adjust the current
>> misbehavior too in that at least instruction fetches no longer get
>> reported as reads.
> 
> What I am asking is whether
> 
>     .read_access = !(pfec & (PFEC_insn_fetch | PFEC_write_access))
> 
> would be more appropriate.

Ah, no, clearly not: Again - read-modify-write instructions _have_
to be reported as being reads and writes. Reporting simply writes
as reads too is the smaller of the two "evils" here. If anything we
could introduce a "maybe-read" flag that gets set when don't know
for sure.

Jan

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Boris Ostrovsky]

On 08/15/2014 11:29 AM, Jan Beulich wrote:
>>>> On 15.08.14 at 17:09, <boris.ostrovsky@oracle.com> wrote:
>> On 08/15/2014 11:01 AM, Jan Beulich wrote:
>>>>>> On 15.08.14 at 16:31, <boris.ostrovsky@oracle.com> wrote:
>>>> On 08/14/2014 06:59 PM, Jan Beulich wrote:
>>>>> No - the hardware specifically does _not_ guarantee to report the
>>>>> actual characteristics of a read-modify-write instruction. Or at least
>>>>> that's what your documentation warns about. And to be on the safe
>>>>> side, treating all writes as also being reads is the better option than
>>>>> to mistakenly treat r-m-w as just w.
>>>> Is this specific to VMX or does SVM have the same problem (I am not
>>>> aware of this but I might be wrong). Because if it doesn't then I think
>>>> Tamas' [PATCH v6 2/4] should have SVM report actual bits.
>>> You as the SVM maintainer should know better than me... With
>>> NPT using "normal" page fault error codes, there is not even an
>>> indication for read access. Tamas's patches adjust the current
>>> misbehavior too in that at least instruction fetches no longer get
>>> reported as reads.
>> What I am asking is whether
>>
>>      .read_access = !(pfec & (PFEC_insn_fetch | PFEC_write_access))
>>
>> would be more appropriate.
> Ah, no, clearly not: Again - read-modify-write instructions _have_
> to be reported as being reads and writes. Reporting simply writes
> as reads too is the smaller of the two "evils" here. If anything we
> could introduce a "maybe-read" flag that gets set when don't know
> for sure.

I think an explicit comment in VMX and SVM code explaining why the bits 
are set the way they are may be sufficient (I know this is mentioned in 
the commit message but having it in the code is better IMO).

-boris

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Tamas Lengyel]

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

>
>
>>>  Ah, no, clearly not: Again - read-modify-write instructions _have_
>> to be reported as being reads and writes. Reporting simply writes
>> as reads too is the smaller of the two "evils" here. If anything we
>> could introduce a "maybe-read" flag that gets set when don't know
>> for sure.
>>
>
> I think an explicit comment in VMX and SVM code explaining why the bits
> are set the way they are may be sufficient (I know this is mentioned in the
> commit message but having it in the code is better IMO).
>
> -boris
>

I can certainly do that if the consensus is to include the patch.

Tamas

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

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

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

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Jan Beulich]

>>> On 15.08.14 at 18:33, <tamas.lengyel@zentific.com> wrote:
>> 
>>
>>>>  Ah, no, clearly not: Again - read-modify-write instructions _have_
>>> to be reported as being reads and writes. Reporting simply writes
>>> as reads too is the smaller of the two "evils" here. If anything we
>>> could introduce a "maybe-read" flag that gets set when don't know
>>> for sure.
>>>
>>
>> I think an explicit comment in VMX and SVM code explaining why the bits
>> are set the way they are may be sufficient (I know this is mentioned in the
>> commit message but having it in the code is better IMO).
> 
> I can certainly do that if the consensus is to include the patch.

The patch is a necessary prerequisite for the patch that I sent you
earlier (which I'll rebase on top of yours as soon as yours reached a
state that can go in - which, afaic, is already the case), so it will
have to go in (as said in another reply, in the worst case with a
maybe-read flag instead of the current solution, but personally I
don't see a point to distinguish the two cases until a consumer
appears that can't tolerate plain writes to also be marked as being
reads).

Jan

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, August 15, 2014 7:58 AM
> 
> >>> On 15.08.14 at 01:40, <kevin.tian@intel.com> wrote:
> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> >> The intended use is that pending patch I referred to, aiming at
> >> reducing the number of GLA->GPA translations we're doing during
> >> instruction emulation, which makes a severe difference to many-
> >> vCPU guests e.g. heavily using the HPET from all their vCPU-s.
> >
> > Is attached patch what you're talking here?
> 
> Yes, that one.
> 

Though I understand the merit of your overall patch, the discussion
around r/w violations are not directly related to the optimization
your patch brings (the real reason is from available GLA info at
VM-exit). It' just because you code the patch that way, so 
__hvmemul_read purely assumes NPFEC_read_access if not is
NPFEC_insn_fetch, and then read-modify-write can't enter the
fast path w/o faking read.

If my understanding is correct, why not just do whatever tricks
here in nestedhvm_hap_nested_page_fault, while keeping arch
specific code unchanged?

Thanks
Kevin

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Jan Beulich]

>>> On 15.08.14 at 22:17, <kevin.tian@intel.com> wrote:
> Though I understand the merit of your overall patch, the discussion
> around r/w violations are not directly related to the optimization
> your patch brings (the real reason is from available GLA info at
> VM-exit). It' just because you code the patch that way, so 
> __hvmemul_read purely assumes NPFEC_read_access if not is
> NPFEC_insn_fetch, and then read-modify-write can't enter the
> fast path w/o faking read.

That's right, yes.

> If my understanding is correct, why not just do whatever tricks
> here in nestedhvm_hap_nested_page_fault, while keeping arch
> specific code unchanged?

Because the net effect is the same, and because getting is into
final shape right away rather than having two layers fiddling with
it seems cleaner? Counter question: Why can't the hardware
report true characteristics right away?

Jan

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, August 15, 2014 2:24 PM
> 
> >>> On 15.08.14 at 22:17, <kevin.tian@intel.com> wrote:
> > Though I understand the merit of your overall patch, the discussion
> > around r/w violations are not directly related to the optimization
> > your patch brings (the real reason is from available GLA info at
> > VM-exit). It' just because you code the patch that way, so
> > __hvmemul_read purely assumes NPFEC_read_access if not is
> > NPFEC_insn_fetch, and then read-modify-write can't enter the
> > fast path w/o faking read.
> 
> That's right, yes.
> 
> > If my understanding is correct, why not just do whatever tricks
> > here in nestedhvm_hap_nested_page_fault, while keeping arch
> > specific code unchanged?
> 
> Because the net effect is the same, and because getting is into
> final shape right away rather than having two layers fiddling with
> it seems cleaner? 

I still think another way cleaner since you can enforce whatever
policy in a centralized place, and it's more flexible if there does
have some usages relying on the raw hardware reported violations
(say statistics of r/w violations... though either way is inaccurate
but my gut-feeling is the # of pure writes is much higher than # of
read-modify-writes, so treating all writes as reads just cause more
bias)

> Counter question: Why can't the hardware
> report true characteristics right away?
> 

when spec says so, there is a reason but I can't tell here. :-)


Thanks
Kevin

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Tamas K Lengyel]

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

On Fri, Aug 15, 2014 at 6:48 PM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 15.08.14 at 18:33, <tamas.lengyel@zentific.com> wrote:
> >>
> >>
> >>>>  Ah, no, clearly not: Again - read-modify-write instructions _have_
> >>> to be reported as being reads and writes. Reporting simply writes
> >>> as reads too is the smaller of the two "evils" here. If anything we
> >>> could introduce a "maybe-read" flag that gets set when don't know
> >>> for sure.
> >>>
> >>
> >> I think an explicit comment in VMX and SVM code explaining why the bits
> >> are set the way they are may be sufficient (I know this is mentioned in
> the
> >> commit message but having it in the code is better IMO).
> >
> > I can certainly do that if the consensus is to include the patch.
>
> The patch is a necessary prerequisite for the patch that I sent you
> earlier (which I'll rebase on top of yours as soon as yours reached a
> state that can go in - which, afaic, is already the case), so it will
> have to go in (as said in another reply, in the worst case with a
> maybe-read flag instead of the current solution, but personally I
> don't see a point to distinguish the two cases until a consumer
> appears that can't tolerate plain writes to also be marked as being
> reads).
>
> Jan
>

I think the best comment for the VMX side is to just actually copy-paste
the warning from the Intel manual (which sounds actually a lot more severe,
since it isn't even consistent in marking r-m-w instructions as w only):

/* We treat all write violations also as read violations.  * The reason why
this is required is the following warning:  * "An EPT violation that occurs
during as a result of execution of a  * read-modify-write operation sets
bit 1 (data write). Whether it also  * sets bit 0 (data read) is
implementation-specific and, for a given  * implementation, may differ for
different kinds of read-modify-write  * operations."  * - Intel(R) 64 and
IA-32 Architectures Software Developer's Manual  * Volume 3C: System
Programming Guide, Part 3 */

As for the SVM side, I'm thinking something along these lines:

/* We distinguis between data read-access violations  * and instruction
fetch violations here, albeit the fact  * that the hardware doesn't
explicitely differentiate them.  * This is required in order to provide
appropriate abstraction  * of vendor-specific NPT implementations. */
Tamas

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

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

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

Re: [PATCH v6 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Jan Beulich]

>>> Tamas K Lengyel <tamas.lengyel@zentific.com> 08/18/14 9:37 PM >>>
>On Fri, Aug 15, 2014 at 6:48 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 15.08.14 at 18:33, <tamas.lengyel@zentific.com> wrote:
>> >>>>  Ah, no, clearly not: Again - read-modify-write instructions _have_
>> >>> to be reported as being reads and writes. Reporting simply writes
>> >>> as reads too is the smaller of the two "evils" here. If anything we
>> >>> could introduce a "maybe-read" flag that gets set when don't know
>> >>> for sure.
>> >>
>> >> I think an explicit comment in VMX and SVM code explaining why the bits
>> >> are set the way they are may be sufficient (I know this is mentioned in the
>> >> commit message but having it in the code is better IMO).
>> >
>> > I can certainly do that if the consensus is to include the patch.
>>
>> The patch is a necessary prerequisite for the patch that I sent you
>> earlier (which I'll rebase on top of yours as soon as yours reached a
>> state that can go in - which, afaic, is already the case), so it will
>> have to go in (as said in another reply, in the worst case with a
>> maybe-read flag instead of the current solution, but personally I
>> don't see a point to distinguish the two cases until a consumer
>> appears that can't tolerate plain writes to also be marked as being
>> reads).
>
>I think the best comment for the VMX side is to just actually copy-paste
>the warning from the Intel manual (which sounds actually a lot more severe,
>since it isn't even consistent in marking r-m-w instructions as w only):
>
>/* We treat all write violations also as read violations.  * The reason why
>this is required is the following warning:  * "An EPT violation that occurs
>during as a result of execution of a  * read-modify-write operation sets
>bit 1 (data write). Whether it also  * sets bit 0 (data read) is
>implementation-specific and, for a given  * implementation, may differ for
>different kinds of read-modify-write  * operations."  * - Intel(R) 64 and
>IA-32 Architectures Software Developer's Manual  * Volume 3C: System
>Programming Guide, Part 3 */

I'm fine with this, but wasn't insisting on the comment anyway.

>As for the SVM side, I'm thinking something along these lines:
>
>/* We distinguis between data read-access violations  * and instruction
>fetch violations here, albeit the fact  * that the hardware doesn't
>explicitely differentiate them.  * This is required in order to provide
>appropriate abstraction  * of vendor-specific NPT implementations. */

I'm not sure this is correct, or even needed. The hardware does very well
distinguish between reads and instruction fetches (after all we've got an
instruction fetch bit in the error code), it's rather that there's no specific
read access bit. (And then it has got a couple of spelling mistakes, which
would be better to get fixed if it is to go in in this or some similar shape; I
also think you mean "despite" instead of "albeit".)

Jan

Re: [PATCH v6 1/4] x86: Consolidate boolean inputs in hvm and p2m into a shared bitmap.

[Tim Deegan]

At 16:48 +0200 on 11 Aug (1407772123), Tamas K Lengyel wrote:
> This patch consolidates the boolean input parameters of hvm_hap_nested_page_fault and p2m_mem_access_check into a common bitmap and defines the bitmap members accordingly.
> 
> v6: Rename shared structure to "struct npfec" and style fixes.
> v5: Shared structure in mm.h, style fixes and moving gla fault type additions into next patch in the series.
> 
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>

Reviewed-by: Tim Deegan <tim@xen.org>