[PATCH 1/2] x86/vmx: Introduce a bitfield structure for EPT_VIOLATION EXIT_QUALIFICATIONs

[PATCH 1/2] x86/vmx: Introduce a bitfield structure for EPT_VIOLATION EXIT_QUALIFICATIONs

[Andrew Cooper]

This results in rather more readable code.  No functional change.

All fields currently specified are included, but commented out as no support
for their use is present.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/vmx/vmx.c        | 41 ++++++++++++++++++---------------------
 xen/include/asm-x86/hvm/vmx/vmx.h | 27 +++++++++++---------------
 2 files changed, 30 insertions(+), 38 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 8177401..eca4f7d 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2986,7 +2986,7 @@ static void vmx_wbinvd_intercept(void)
         wbinvd();
 }
 
-static void ept_handle_violation(unsigned long qualification, paddr_t gpa)
+static void ept_handle_violation(ept_qual_t q, paddr_t gpa)
 {
     unsigned long gla, gfn = gpa >> PAGE_SHIFT;
     mfn_t mfn;
@@ -3006,13 +3006,10 @@ static void ept_handle_violation(unsigned long qualification, paddr_t gpa)
      *   Volume 3C: System Programming Guide, Part 3
      */
     struct npfec npfec = {
-        .read_access = !!(qualification & EPT_READ_VIOLATION) ||
-                       !!(qualification & EPT_WRITE_VIOLATION),
-        .write_access = !!(qualification & EPT_WRITE_VIOLATION),
-        .insn_fetch = !!(qualification & EPT_EXEC_VIOLATION),
-        .present = !!(qualification & (EPT_EFFECTIVE_READ |
-                                       EPT_EFFECTIVE_WRITE |
-                                       EPT_EFFECTIVE_EXEC))
+        .read_access = q.read || q.write,
+        .write_access = q.write,
+        .insn_fetch = q.fetch,
+        .present = q.eff_read || q.eff_write || q.eff_exec,
     };
 
     if ( tb_init_done )
@@ -3025,17 +3022,17 @@ static void ept_handle_violation(unsigned long qualification, paddr_t gpa)
         } _d;
 
         _d.gpa = gpa;
-        _d.qualification = qualification;
+        _d.qualification = q.raw;
         _d.mfn = mfn_x(get_gfn_query_unlocked(d, gfn, &_d.p2mt));
 
         __trace_var(TRC_HVM_NPF, 0, sizeof(_d), &_d);
     }
 
-    if ( qualification & EPT_GLA_VALID )
+    if ( q.gla_valid )
     {
         __vmread(GUEST_LINEAR_ADDRESS, &gla);
         npfec.gla_valid = 1;
-        if( qualification & EPT_GLA_FAULT )
+        if( q.gla_fault )
             npfec.kind = npfec_kind_with_gla;
         else
             npfec.kind = npfec_kind_in_gpt;
@@ -3065,18 +3062,18 @@ static void ept_handle_violation(unsigned long qualification, paddr_t gpa)
     mfn = get_gfn_query_unlocked(d, gfn, &p2mt);
     gprintk(XENLOG_ERR,
             "EPT violation %#lx (%c%c%c/%c%c%c) gpa %#"PRIpaddr" mfn %#lx type %i\n",
-            qualification,
-            (qualification & EPT_READ_VIOLATION) ? 'r' : '-',
-            (qualification & EPT_WRITE_VIOLATION) ? 'w' : '-',
-            (qualification & EPT_EXEC_VIOLATION) ? 'x' : '-',
-            (qualification & EPT_EFFECTIVE_READ) ? 'r' : '-',
-            (qualification & EPT_EFFECTIVE_WRITE) ? 'w' : '-',
-            (qualification & EPT_EFFECTIVE_EXEC) ? 'x' : '-',
+            q.raw,
+            q.read  ? 'r' : '-',
+            q.write ? 'w' : '-',
+            q.fetch ? 'x' : '-',
+            q.eff_read  ? 'r' : '-',
+            q.eff_write ? 'w' : '-',
+            q.eff_exec  ? 'x' : '-',
             gpa, mfn_x(mfn), p2mt);
 
     ept_walk_table(d, gfn);
 
-    if ( qualification & EPT_GLA_VALID )
+    if ( q.gla_valid )
         gprintk(XENLOG_ERR, " --- GLA %#lx\n", gla);
 
     domain_crash(d);
@@ -3792,11 +3789,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
 
     case EXIT_REASON_EPT_VIOLATION:
     {
-        paddr_t gpa;
+        paddr_t gpa; ept_qual_t q;
 
         __vmread(GUEST_PHYSICAL_ADDRESS, &gpa);
-        __vmread(EXIT_QUALIFICATION, &exit_qualification);
-        ept_handle_violation(exit_qualification, gpa);
+        __vmread(EXIT_QUALIFICATION, &q.raw);
+        ept_handle_violation(q, gpa);
         break;
     }
 
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index 4bf4d50..00e6172 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -578,22 +578,17 @@ void vmx_pi_hooks_assign(struct domain *d);
 void vmx_pi_hooks_deassign(struct domain *d);
 
 /* EPT violation qualifications definitions */
-#define _EPT_READ_VIOLATION         0
-#define EPT_READ_VIOLATION          (1UL<<_EPT_READ_VIOLATION)
-#define _EPT_WRITE_VIOLATION        1
-#define EPT_WRITE_VIOLATION         (1UL<<_EPT_WRITE_VIOLATION)
-#define _EPT_EXEC_VIOLATION         2
-#define EPT_EXEC_VIOLATION          (1UL<<_EPT_EXEC_VIOLATION)
-#define _EPT_EFFECTIVE_READ         3
-#define EPT_EFFECTIVE_READ          (1UL<<_EPT_EFFECTIVE_READ)
-#define _EPT_EFFECTIVE_WRITE        4
-#define EPT_EFFECTIVE_WRITE         (1UL<<_EPT_EFFECTIVE_WRITE)
-#define _EPT_EFFECTIVE_EXEC         5
-#define EPT_EFFECTIVE_EXEC          (1UL<<_EPT_EFFECTIVE_EXEC)
-#define _EPT_GLA_VALID              7
-#define EPT_GLA_VALID               (1UL<<_EPT_GLA_VALID)
-#define _EPT_GLA_FAULT              8
-#define EPT_GLA_FAULT               (1UL<<_EPT_GLA_FAULT)
+typedef union ept_qual {
+    unsigned long raw;
+    struct {
+        bool read:1, write:1, fetch:1,
+            eff_read:1, eff_write:1, eff_exec:1, /* eff_user_exec */:1,
+            gla_valid:1,
+            gla_fault:1, /* Valid iff gla_valid. */
+            /* gla_user */:1, /* gla_rw */:1, /* gla_nx */:1,
+            /* nmi_unblock */:1;
+    };
+} ept_qual_t;
 
 #define EPT_L4_PAGETABLE_SHIFT      39
 #define EPT_PAGETABLE_ENTRIES       512
-- 
2.1.4


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

[PATCH 2/2] x86/vmx: Drop ept_get_*() helpers

[Andrew Cooper]

The ept_get_*() helpers are not used consistently, and are more verbose than
the code they wrap.  Drop the wrappers and use the internal union names
consistently.

While making these adjustments, drop the redundant ept_* prefix from mt, wl
and ad, and rename the asr field to mfn for consistency with Xen's existing
terminology.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c        |  6 +++---
 xen/arch/x86/hvm/vmx/vmx.c         |  6 +++---
 xen/arch/x86/hvm/vmx/vvmx.c        |  9 +++-----
 xen/arch/x86/mm/p2m-ept.c          | 44 +++++++++++++++++++-------------------
 xen/include/asm-x86/hvm/vmx/vmcs.h | 16 ++++++--------
 5 files changed, 37 insertions(+), 44 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 59ef199..3fe1783 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1247,8 +1247,8 @@ static int construct_vmcs(struct vcpu *v)
         struct p2m_domain *p2m = p2m_get_hostp2m(d);
         struct ept_data *ept = &p2m->ept;
 
-        ept->asr  = pagetable_get_pfn(p2m_get_pagetable(p2m));
-        __vmwrite(EPT_POINTER, ept_get_eptp(ept));
+        ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
+        __vmwrite(EPT_POINTER, ept->eptp);
     }
 
     if ( paging_mode_hap(d) )
@@ -1593,7 +1593,7 @@ void vmx_domain_update_eptp(struct domain *d)
     ASSERT(atomic_read(&d->pause_count));
 
     for_each_vcpu ( d, v )
-        vmx_vcpu_update_eptp(v, ept_get_eptp(&p2m->ept));
+        vmx_vcpu_update_eptp(v, p2m->ept.eptp);
 
     ept_sync_domain(p2m);
 }
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index eca4f7d..9078cf9 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1996,11 +1996,11 @@ static void vmx_vcpu_update_eptp(struct vcpu *v)
         p2m = p2m_get_hostp2m(d);
 
     ept = &p2m->ept;
-    ept->asr = pagetable_get_pfn(p2m_get_pagetable(p2m));
+    ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
 
     vmx_vmcs_enter(v);
 
-    __vmwrite(EPT_POINTER, ept_get_eptp(ept));
+    __vmwrite(EPT_POINTER, ept->eptp);
 
     if ( v->arch.hvm_vmx.secondary_exec_control &
          SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS )
@@ -3953,7 +3953,7 @@ void vmx_vmenter_helper(const struct cpu_user_regs *regs)
         if ( cpumask_test_cpu(cpu, ept->invalidate) )
         {
             cpumask_clear_cpu(cpu, ept->invalidate);
-            __invept(INVEPT_SINGLE_CONTEXT, ept_get_eptp(ept), 0);
+            __invept(INVEPT_SINGLE_CONTEXT, ept->eptp, 0);
         }
     }
 
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 9caebe5..5acb88a 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1043,16 +1043,13 @@ uint64_t get_shadow_eptp(struct vcpu *v)
     struct p2m_domain *p2m = p2m_get_nestedp2m(v, np2m_base);
     struct ept_data *ept = &p2m->ept;
 
-    ept->asr = pagetable_get_pfn(p2m_get_pagetable(p2m));
-    return ept_get_eptp(ept);
+    ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
+    return ept->eptp;
 }
 
 static uint64_t get_host_eptp(struct vcpu *v)
 {
-    struct domain *d = v->domain;
-    struct ept_data *ept_data = &p2m_get_hostp2m(d)->ept;
-
-    return ept_get_eptp(ept_data);
+    return p2m_get_hostp2m(v->domain)->ept.eptp;
 }
 
 static bool_t nvmx_vpid_enabled(const struct vcpu *v)
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 04878f5..6c1b835 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -430,7 +430,7 @@ static int ept_invalidate_emt_range(struct p2m_domain *p2m,
     int wrc, rc = 0, ret = GUEST_TABLE_MAP_FAILED;
 
     table = map_domain_page(_mfn(pagetable_get_pfn(p2m_get_pagetable(p2m))));
-    for ( i = ept_get_wl(&p2m->ept); i > target; --i )
+    for ( i = p2m->ept.wl; i > target; --i )
     {
         ret = ept_next_level(p2m, 1, &table, &gfn_remainder, i);
         if ( ret == GUEST_TABLE_MAP_FAILED )
@@ -500,8 +500,8 @@ static int ept_invalidate_emt_range(struct p2m_domain *p2m,
 static int resolve_misconfig(struct p2m_domain *p2m, unsigned long gfn)
 {
     struct ept_data *ept = &p2m->ept;
-    unsigned int level = ept_get_wl(ept);
-    unsigned long mfn = ept_get_asr(ept);
+    unsigned int level = ept->wl;
+    unsigned long mfn = ept->mfn;
     ept_entry_t *epte;
     int wrc, rc = 0;
 
@@ -687,7 +687,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
      * 3. passing a valid order.
      */
     if ( ((gfn | mfn_x(mfn)) & ((1UL << order) - 1)) ||
-         ((u64)gfn >> ((ept_get_wl(ept) + 1) * EPT_TABLE_ORDER)) ||
+         ((u64)gfn >> ((ept->wl + 1) * EPT_TABLE_ORDER)) ||
          (order % EPT_TABLE_ORDER) )
         return -EINVAL;
 
@@ -704,7 +704,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
     table = map_domain_page(_mfn(pagetable_get_pfn(p2m_get_pagetable(p2m))));
 
     ret = GUEST_TABLE_MAP_FAILED;
-    for ( i = ept_get_wl(ept); i > target; i-- )
+    for ( i = ept->wl; i > target; i-- )
     {
         ret = ept_next_level(p2m, 0, &table, &gfn_remainder, i);
         if ( !ret )
@@ -898,7 +898,7 @@ static mfn_t ept_get_entry(struct p2m_domain *p2m,
     /* This pfn is higher than the highest the p2m map currently holds */
     if ( gfn > p2m->max_mapped_pfn )
     {
-        for ( i = ept_get_wl(ept); i > 0; --i )
+        for ( i = ept->wl; i > 0; --i )
             if ( (gfn & ~((1UL << (i * EPT_TABLE_ORDER)) - 1)) >
                  p2m->max_mapped_pfn )
                 break;
@@ -907,7 +907,7 @@ static mfn_t ept_get_entry(struct p2m_domain *p2m,
 
     /* Should check if gfn obeys GAW here. */
 
-    for ( i = ept_get_wl(ept); i > 0; i-- )
+    for ( i = ept->wl; i > 0; i-- )
     {
     retry:
         if ( table[gfn_remainder >> (i * EPT_TABLE_ORDER)].recalc )
@@ -1009,7 +1009,7 @@ void ept_walk_table(struct domain *d, unsigned long gfn)
         goto out;
     }
 
-    for ( i = ept_get_wl(ept); i >= 0; i-- )
+    for ( i = ept->wl; i >= 0; i-- )
     {
         ept_entry_t *ept_entry, *next;
         u32 index;
@@ -1043,12 +1043,12 @@ void ept_walk_table(struct domain *d, unsigned long gfn)
 static void ept_change_entry_type_global(struct p2m_domain *p2m,
                                          p2m_type_t ot, p2m_type_t nt)
 {
-    unsigned long mfn = ept_get_asr(&p2m->ept);
+    unsigned long mfn = p2m->ept.mfn;
 
     if ( !mfn )
         return;
 
-    if ( ept_invalidate_emt(_mfn(mfn), 1, ept_get_wl(&p2m->ept)) )
+    if ( ept_invalidate_emt(_mfn(mfn), 1, p2m->ept.wl) )
         ept_sync_domain(p2m);
 }
 
@@ -1057,11 +1057,11 @@ static int ept_change_entry_type_range(struct p2m_domain *p2m,
                                        unsigned long first_gfn,
                                        unsigned long last_gfn)
 {
-    unsigned int i, wl = ept_get_wl(&p2m->ept);
+    unsigned int i, wl = p2m->ept.wl;
     unsigned long mask = (1 << EPT_TABLE_ORDER) - 1;
     int rc = 0, sync = 0;
 
-    if ( !ept_get_asr(&p2m->ept) )
+    if ( p2m->ept.mfn )
         return -EINVAL;
 
     for ( i = 0; i <= wl; )
@@ -1101,12 +1101,12 @@ static int ept_change_entry_type_range(struct p2m_domain *p2m,
 
 static void ept_memory_type_changed(struct p2m_domain *p2m)
 {
-    unsigned long mfn = ept_get_asr(&p2m->ept);
+    unsigned long mfn = p2m->ept.mfn;
 
     if ( !mfn )
         return;
 
-    if ( ept_invalidate_emt(_mfn(mfn), 0, ept_get_wl(&p2m->ept)) )
+    if ( ept_invalidate_emt(_mfn(mfn), 0, p2m->ept.wl) )
         ept_sync_domain(p2m);
 }
 
@@ -1179,7 +1179,7 @@ static void ept_enable_pml(struct p2m_domain *p2m)
         return;
 
     /* Enable EPT A/D bit for PML */
-    p2m->ept.ept_ad = 1;
+    p2m->ept.ad = 1;
     vmx_domain_update_eptp(p2m->domain);
 }
 
@@ -1191,7 +1191,7 @@ static void ept_disable_pml(struct p2m_domain *p2m)
     vmx_domain_disable_pml(p2m->domain);
 
     /* Disable EPT A/D bit */
-    p2m->ept.ept_ad = 0;
+    p2m->ept.ad = 0;
     vmx_domain_update_eptp(p2m->domain);
 }
 
@@ -1216,10 +1216,10 @@ int ept_p2m_init(struct p2m_domain *p2m)
     p2m->tlb_flush = ept_tlb_flush;
 
     /* Set the memory type used when accessing EPT paging structures. */
-    ept->ept_mt = EPT_DEFAULT_MT;
+    ept->mt = EPT_DEFAULT_MT;
 
     /* set EPT page-walk length, now it's actual walk length - 1, i.e. 3 */
-    ept->ept_wl = 3;
+    ept->wl = 3;
 
     if ( cpu_has_vmx_pml )
     {
@@ -1289,7 +1289,7 @@ static void ept_dump_p2m_table(unsigned char key)
             gfn_remainder = gfn;
             table = map_domain_page(_mfn(pagetable_get_pfn(p2m_get_pagetable(p2m))));
 
-            for ( i = ept_get_wl(ept); i > 0; i-- )
+            for ( i = ept->wl; i > 0; i-- )
             {
                 ept_entry = table + (gfn_remainder >> (i * EPT_TABLE_ORDER));
                 if ( ept_entry->emt == MTRR_NUM_TYPES )
@@ -1337,8 +1337,8 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
     p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
     p2m->max_remapped_gfn = 0;
     ept = &p2m->ept;
-    ept->asr = pagetable_get_pfn(p2m_get_pagetable(p2m));
-    d->arch.altp2m_eptp[i] = ept_get_eptp(ept);
+    ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
+    d->arch.altp2m_eptp[i] = ept->eptp;
 }
 
 unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
@@ -1357,7 +1357,7 @@ unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
         p2m = d->arch.altp2m_p2m[i];
         ept = &p2m->ept;
 
-        if ( eptp == ept_get_eptp(ept) )
+        if ( eptp == ept->eptp )
             goto out;
     }
 
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index d71de04..33fe7d6 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -58,12 +58,12 @@ struct vmx_msr_state {
 
 struct ept_data {
     union {
-    struct {
-            u64 ept_mt :3,
-                ept_wl :3,
-                ept_ad :1,  /* bit 6 - enable EPT A/D bits */
-                rsvd   :5,
-                asr    :52;
+        struct {
+            uint64_t mt:3,   /* Memory Type. */
+                     wl:3,   /* Walk length -1. */
+                     ad:1,   /* Enable EPT A/D bits. */
+                     :5,     /* rsvd. */
+                     mfn:52;
         };
         u64 eptp;
     };
@@ -95,10 +95,6 @@ struct pi_desc {
     u32 rsvd[6];
 } __attribute__ ((aligned (64)));
 
-#define ept_get_wl(ept)   ((ept)->ept_wl)
-#define ept_get_asr(ept)  ((ept)->asr)
-#define ept_get_eptp(ept) ((ept)->eptp)
-
 #define NR_PML_ENTRIES   512
 
 struct pi_blocking_vcpu {
-- 
2.1.4


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

Re: [PATCH 2/2] x86/vmx: Drop ept_get_*() helpers

[George Dunlap]

On 30/01/17 16:54, Andrew Cooper wrote:
> The ept_get_*() helpers are not used consistently, and are more verbose than
> the code they wrap.  Drop the wrappers and use the internal union names
> consistently.
> 
> While making these adjustments, drop the redundant ept_* prefix from mt, wl
> and ad, and rename the asr field to mfn for consistency with Xen's existing
> terminology.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: George Dunlap <george.dunlap@citrix.com>


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

Re: [PATCH 1/2] x86/vmx: Introduce a bitfield structure for EPT_VIOLATION EXIT_QUALIFICATIONs

[Jan Beulich]

>>> On 30.01.17 at 17:54, <andrew.cooper3@citrix.com> wrote:
> This results in rather more readable code.  No functional change.
> 
> All fields currently specified are included, but commented out as no support
> for their use is present.

I'd rather not see them be commented out: Why should the first user
of them have to touch the structure declaration another time?

> @@ -3792,11 +3789,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>  
>      case EXIT_REASON_EPT_VIOLATION:
>      {
> -        paddr_t gpa;
> +        paddr_t gpa; ept_qual_t q;
>  
>          __vmread(GUEST_PHYSICAL_ADDRESS, &gpa);
> -        __vmread(EXIT_QUALIFICATION, &exit_qualification);
> -        ept_handle_violation(exit_qualification, gpa);
> +        __vmread(EXIT_QUALIFICATION, &q.raw);
> +        ept_handle_violation(q, gpa);
>          break;
>      }

If you made the union a transparent one, I think you wouldn't have
to touch this code at all.

Jan


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

Re: [PATCH 2/2] x86/vmx: Drop ept_get_*() helpers

[Jan Beulich]

>>> On 30.01.17 at 17:54, <andrew.cooper3@citrix.com> wrote:
> The ept_get_*() helpers are not used consistently, and are more verbose than
> the code they wrap.  Drop the wrappers and use the internal union names
> consistently.
> 
> While making these adjustments, drop the redundant ept_* prefix from mt, wl
> and ad, and rename the asr field to mfn for consistency with Xen's existing
> terminology.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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



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

Re: [PATCH 1/2] x86/vmx: Introduce a bitfield structure for EPT_VIOLATION EXIT_QUALIFICATIONs

[Andrew Cooper]

On 31/01/17 10:19, Jan Beulich wrote:
>>>> On 30.01.17 at 17:54, <andrew.cooper3@citrix.com> wrote:
>> This results in rather more readable code.  No functional change.
>>
>> All fields currently specified are included, but commented out as no support
>> for their use is present.
> I'd rather not see them be commented out: Why should the first user
> of them have to touch the structure declaration another time?

I purposefully don't want someone to think they can use .eff_user_exec
before doing the work to enable the feature (which amongst other things
will involve shuffling the position of bits in an ept_entry_t as the
hardware user exec bit is currently where Xen's .recalc bit currently
lives).

>
>> @@ -3792,11 +3789,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>  
>>      case EXIT_REASON_EPT_VIOLATION:
>>      {
>> -        paddr_t gpa;
>> +        paddr_t gpa; ept_qual_t q;
>>  
>>          __vmread(GUEST_PHYSICAL_ADDRESS, &gpa);
>> -        __vmread(EXIT_QUALIFICATION, &exit_qualification);
>> -        ept_handle_violation(exit_qualification, gpa);
>> +        __vmread(EXIT_QUALIFICATION, &q.raw);
>> +        ept_handle_violation(q, gpa);
>>          break;
>>      }
> If you made the union a transparent one, I think you wouldn't have
> to touch this code at all.

Is that liable to work on all supported compilers?

~Andrew

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

Re: [PATCH 1/2] x86/vmx: Introduce a bitfield structure for EPT_VIOLATION EXIT_QUALIFICATIONs

[Jan Beulich]

>>> On 31.01.17 at 11:39, <andrew.cooper3@citrix.com> wrote:
> On 31/01/17 10:19, Jan Beulich wrote:
>>>>> On 30.01.17 at 17:54, <andrew.cooper3@citrix.com> wrote:
>>> This results in rather more readable code.  No functional change.
>>>
>>> All fields currently specified are included, but commented out as no support
>>> for their use is present.
>> I'd rather not see them be commented out: Why should the first user
>> of them have to touch the structure declaration another time?
> 
> I purposefully don't want someone to think they can use .eff_user_exec
> before doing the work to enable the feature (which amongst other things
> will involve shuffling the position of bits in an ept_entry_t as the
> hardware user exec bit is currently where Xen's .recalc bit currently
> lives).

For this bit I understand the reasoning. I don't think the same applies
to bits 9..12 though.

>>> @@ -3792,11 +3789,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>>  
>>>      case EXIT_REASON_EPT_VIOLATION:
>>>      {
>>> -        paddr_t gpa;
>>> +        paddr_t gpa; ept_qual_t q;
>>>  
>>>          __vmread(GUEST_PHYSICAL_ADDRESS, &gpa);
>>> -        __vmread(EXIT_QUALIFICATION, &exit_qualification);
>>> -        ept_handle_violation(exit_qualification, gpa);
>>> +        __vmread(EXIT_QUALIFICATION, &q.raw);
>>> +        ept_handle_violation(q, gpa);
>>>          break;
>>>      }
>> If you made the union a transparent one, I think you wouldn't have
>> to touch this code at all.
> 
> Is that liable to work on all supported compilers?

Transparent unions are rather old a feature of gcc; I can't speak
of clang, though, but we certainly have uses of them already in
our tree.

Jan


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

Re: [PATCH 1/2] x86/vmx: Introduce a bitfield structure for EPT_VIOLATION EXIT_QUALIFICATIONs

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, January 31, 2017 6:56 PM
> 
> >>> On 31.01.17 at 11:39, <andrew.cooper3@citrix.com> wrote:
> > On 31/01/17 10:19, Jan Beulich wrote:
> >>>>> On 30.01.17 at 17:54, <andrew.cooper3@citrix.com> wrote:
> >>> This results in rather more readable code.  No functional change.
> >>>
> >>> All fields currently specified are included, but commented out as no support
> >>> for their use is present.
> >> I'd rather not see them be commented out: Why should the first user
> >> of them have to touch the structure declaration another time?
> >
> > I purposefully don't want someone to think they can use .eff_user_exec
> > before doing the work to enable the feature (which amongst other things
> > will involve shuffling the position of bits in an ept_entry_t as the
> > hardware user exec bit is currently where Xen's .recalc bit currently
> > lives).
> 
> For this bit I understand the reasoning. I don't think the same applies
> to bits 9..12 though.
> 

I'm with Jan on this front. Could you resend the patch with comment
only applying to above bit?

Thanks
Kevin

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

Re: [PATCH 2/2] x86/vmx: Drop ept_get_*() helpers

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Tuesday, January 31, 2017 12:54 AM
> 
> The ept_get_*() helpers are not used consistently, and are more verbose than
> the code they wrap.  Drop the wrappers and use the internal union names
> consistently.
> 
> While making these adjustments, drop the redundant ept_* prefix from mt, wl
> and ad, and rename the asr field to mfn for consistency with Xen's existing
> terminology.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

[PATCH v2 1/2] x86/vmx: Introduce a bitfield structure for EPT_VIOLATION EXIT_QUALIFICATIONs

[Andrew Cooper]

This results in rather more readable code.  No functional change.

All fields currently specified are included, but commented out as no support
for their use is present.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>

v2:
 * Use a transparent union rather than modifying the caller of
   ept_handle_violation()
 * Drop the extranious commented out bitfield names, but keep eff_user_exec so
   gla_{valid,fault} are appropriately located.
---
 xen/arch/x86/hvm/vmx/vmx.c        | 35 ++++++++++++++++-------------------
 xen/include/asm-x86/hvm/vmx/vmx.h | 25 +++++++++----------------
 xen/include/xen/compiler.h        |  1 +
 3 files changed, 26 insertions(+), 35 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 5aada98..d3d98da 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2986,7 +2986,7 @@ static void vmx_wbinvd_intercept(void)
         wbinvd();
 }
 
-static void ept_handle_violation(unsigned long qualification, paddr_t gpa)
+static void ept_handle_violation(ept_qual_t q, paddr_t gpa)
 {
     unsigned long gla, gfn = gpa >> PAGE_SHIFT;
     mfn_t mfn;
@@ -3006,13 +3006,10 @@ static void ept_handle_violation(unsigned long qualification, paddr_t gpa)
      *   Volume 3C: System Programming Guide, Part 3
      */
     struct npfec npfec = {
-        .read_access = !!(qualification & EPT_READ_VIOLATION) ||
-                       !!(qualification & EPT_WRITE_VIOLATION),
-        .write_access = !!(qualification & EPT_WRITE_VIOLATION),
-        .insn_fetch = !!(qualification & EPT_EXEC_VIOLATION),
-        .present = !!(qualification & (EPT_EFFECTIVE_READ |
-                                       EPT_EFFECTIVE_WRITE |
-                                       EPT_EFFECTIVE_EXEC))
+        .read_access = q.read || q.write,
+        .write_access = q.write,
+        .insn_fetch = q.fetch,
+        .present = q.eff_read || q.eff_write || q.eff_exec,
     };
 
     if ( tb_init_done )
@@ -3025,17 +3022,17 @@ static void ept_handle_violation(unsigned long qualification, paddr_t gpa)
         } _d;
 
         _d.gpa = gpa;
-        _d.qualification = qualification;
+        _d.qualification = q.raw;
         _d.mfn = mfn_x(get_gfn_query_unlocked(d, gfn, &_d.p2mt));
 
         __trace_var(TRC_HVM_NPF, 0, sizeof(_d), &_d);
     }
 
-    if ( qualification & EPT_GLA_VALID )
+    if ( q.gla_valid )
     {
         __vmread(GUEST_LINEAR_ADDRESS, &gla);
         npfec.gla_valid = 1;
-        if( qualification & EPT_GLA_FAULT )
+        if( q.gla_fault )
             npfec.kind = npfec_kind_with_gla;
         else
             npfec.kind = npfec_kind_in_gpt;
@@ -3065,18 +3062,18 @@ static void ept_handle_violation(unsigned long qualification, paddr_t gpa)
     mfn = get_gfn_query_unlocked(d, gfn, &p2mt);
     gprintk(XENLOG_ERR,
             "EPT violation %#lx (%c%c%c/%c%c%c) gpa %#"PRIpaddr" mfn %#lx type %i\n",
-            qualification,
-            (qualification & EPT_READ_VIOLATION) ? 'r' : '-',
-            (qualification & EPT_WRITE_VIOLATION) ? 'w' : '-',
-            (qualification & EPT_EXEC_VIOLATION) ? 'x' : '-',
-            (qualification & EPT_EFFECTIVE_READ) ? 'r' : '-',
-            (qualification & EPT_EFFECTIVE_WRITE) ? 'w' : '-',
-            (qualification & EPT_EFFECTIVE_EXEC) ? 'x' : '-',
+            q.raw,
+            q.read  ? 'r' : '-',
+            q.write ? 'w' : '-',
+            q.fetch ? 'x' : '-',
+            q.eff_read  ? 'r' : '-',
+            q.eff_write ? 'w' : '-',
+            q.eff_exec  ? 'x' : '-',
             gpa, mfn_x(mfn), p2mt);
 
     ept_walk_table(d, gfn);
 
-    if ( qualification & EPT_GLA_VALID )
+    if ( q.gla_valid )
         gprintk(XENLOG_ERR, " --- GLA %#lx\n", gla);
 
     domain_crash(d);
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index 4bf4d50..1c5ae11 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -578,22 +578,15 @@ void vmx_pi_hooks_assign(struct domain *d);
 void vmx_pi_hooks_deassign(struct domain *d);
 
 /* EPT violation qualifications definitions */
-#define _EPT_READ_VIOLATION         0
-#define EPT_READ_VIOLATION          (1UL<<_EPT_READ_VIOLATION)
-#define _EPT_WRITE_VIOLATION        1
-#define EPT_WRITE_VIOLATION         (1UL<<_EPT_WRITE_VIOLATION)
-#define _EPT_EXEC_VIOLATION         2
-#define EPT_EXEC_VIOLATION          (1UL<<_EPT_EXEC_VIOLATION)
-#define _EPT_EFFECTIVE_READ         3
-#define EPT_EFFECTIVE_READ          (1UL<<_EPT_EFFECTIVE_READ)
-#define _EPT_EFFECTIVE_WRITE        4
-#define EPT_EFFECTIVE_WRITE         (1UL<<_EPT_EFFECTIVE_WRITE)
-#define _EPT_EFFECTIVE_EXEC         5
-#define EPT_EFFECTIVE_EXEC          (1UL<<_EPT_EFFECTIVE_EXEC)
-#define _EPT_GLA_VALID              7
-#define EPT_GLA_VALID               (1UL<<_EPT_GLA_VALID)
-#define _EPT_GLA_FAULT              8
-#define EPT_GLA_FAULT               (1UL<<_EPT_GLA_FAULT)
+typedef union __transparent__ ept_qual {
+    unsigned long raw;
+    struct {
+        bool read:1, write:1, fetch:1,
+            eff_read:1, eff_write:1, eff_exec:1, /* eff_user_exec */:1,
+            gla_valid:1,
+            gla_fault:1; /* Valid iff gla_valid. */
+    };
+} ept_qual_t;
 
 #define EPT_L4_PAGETABLE_SHIFT      39
 #define EPT_PAGETABLE_ENTRIES       512
diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index 33f0b96..e800709 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -47,6 +47,7 @@
 
 #define __attribute_pure__  __attribute__((__pure__))
 #define __attribute_const__ __attribute__((__const__))
+#define __transparent__     __attribute__((__transparent_union__))
 
 /*
  * The difference between the following two attributes is that __used is
-- 
2.1.4


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

Re: [PATCH v2 1/2] x86/vmx: Introduce a bitfield structure for EPT_VIOLATION EXIT_QUALIFICATIONs

[Andrew Cooper]

On 08/02/17 10:42, Andrew Cooper wrote:
> This results in rather more readable code.  No functional change.
>
> All fields currently specified are included, but commented out as no support
> for their use is present.

Apologies - sent a slightly stale version of the patch.  I have dropped
this paragraph from the commit message, but the code is correct for v2.

~Andrew

>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
>
> v2:
>  * Use a transparent union rather than modifying the caller of
>    ept_handle_violation()
>  * Drop the extranious commented out bitfield names, but keep eff_user_exec so
>    gla_{valid,fault} are appropriately located.
> ---
>  xen/arch/x86/hvm/vmx/vmx.c        | 35 ++++++++++++++++-------------------
>  xen/include/asm-x86/hvm/vmx/vmx.h | 25 +++++++++----------------
>  xen/include/xen/compiler.h        |  1 +
>  3 files changed, 26 insertions(+), 35 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 5aada98..d3d98da 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2986,7 +2986,7 @@ static void vmx_wbinvd_intercept(void)
>          wbinvd();
>  }
>  
> -static void ept_handle_violation(unsigned long qualification, paddr_t gpa)
> +static void ept_handle_violation(ept_qual_t q, paddr_t gpa)
>  {
>      unsigned long gla, gfn = gpa >> PAGE_SHIFT;
>      mfn_t mfn;
> @@ -3006,13 +3006,10 @@ static void ept_handle_violation(unsigned long qualification, paddr_t gpa)
>       *   Volume 3C: System Programming Guide, Part 3
>       */
>      struct npfec npfec = {
> -        .read_access = !!(qualification & EPT_READ_VIOLATION) ||
> -                       !!(qualification & EPT_WRITE_VIOLATION),
> -        .write_access = !!(qualification & EPT_WRITE_VIOLATION),
> -        .insn_fetch = !!(qualification & EPT_EXEC_VIOLATION),
> -        .present = !!(qualification & (EPT_EFFECTIVE_READ |
> -                                       EPT_EFFECTIVE_WRITE |
> -                                       EPT_EFFECTIVE_EXEC))
> +        .read_access = q.read || q.write,
> +        .write_access = q.write,
> +        .insn_fetch = q.fetch,
> +        .present = q.eff_read || q.eff_write || q.eff_exec,
>      };
>  
>      if ( tb_init_done )
> @@ -3025,17 +3022,17 @@ static void ept_handle_violation(unsigned long qualification, paddr_t gpa)
>          } _d;
>  
>          _d.gpa = gpa;
> -        _d.qualification = qualification;
> +        _d.qualification = q.raw;
>          _d.mfn = mfn_x(get_gfn_query_unlocked(d, gfn, &_d.p2mt));
>  
>          __trace_var(TRC_HVM_NPF, 0, sizeof(_d), &_d);
>      }
>  
> -    if ( qualification & EPT_GLA_VALID )
> +    if ( q.gla_valid )
>      {
>          __vmread(GUEST_LINEAR_ADDRESS, &gla);
>          npfec.gla_valid = 1;
> -        if( qualification & EPT_GLA_FAULT )
> +        if( q.gla_fault )
>              npfec.kind = npfec_kind_with_gla;
>          else
>              npfec.kind = npfec_kind_in_gpt;
> @@ -3065,18 +3062,18 @@ static void ept_handle_violation(unsigned long qualification, paddr_t gpa)
>      mfn = get_gfn_query_unlocked(d, gfn, &p2mt);
>      gprintk(XENLOG_ERR,
>              "EPT violation %#lx (%c%c%c/%c%c%c) gpa %#"PRIpaddr" mfn %#lx type %i\n",
> -            qualification,
> -            (qualification & EPT_READ_VIOLATION) ? 'r' : '-',
> -            (qualification & EPT_WRITE_VIOLATION) ? 'w' : '-',
> -            (qualification & EPT_EXEC_VIOLATION) ? 'x' : '-',
> -            (qualification & EPT_EFFECTIVE_READ) ? 'r' : '-',
> -            (qualification & EPT_EFFECTIVE_WRITE) ? 'w' : '-',
> -            (qualification & EPT_EFFECTIVE_EXEC) ? 'x' : '-',
> +            q.raw,
> +            q.read  ? 'r' : '-',
> +            q.write ? 'w' : '-',
> +            q.fetch ? 'x' : '-',
> +            q.eff_read  ? 'r' : '-',
> +            q.eff_write ? 'w' : '-',
> +            q.eff_exec  ? 'x' : '-',
>              gpa, mfn_x(mfn), p2mt);
>  
>      ept_walk_table(d, gfn);
>  
> -    if ( qualification & EPT_GLA_VALID )
> +    if ( q.gla_valid )
>          gprintk(XENLOG_ERR, " --- GLA %#lx\n", gla);
>  
>      domain_crash(d);
> diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
> index 4bf4d50..1c5ae11 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -578,22 +578,15 @@ void vmx_pi_hooks_assign(struct domain *d);
>  void vmx_pi_hooks_deassign(struct domain *d);
>  
>  /* EPT violation qualifications definitions */
> -#define _EPT_READ_VIOLATION         0
> -#define EPT_READ_VIOLATION          (1UL<<_EPT_READ_VIOLATION)
> -#define _EPT_WRITE_VIOLATION        1
> -#define EPT_WRITE_VIOLATION         (1UL<<_EPT_WRITE_VIOLATION)
> -#define _EPT_EXEC_VIOLATION         2
> -#define EPT_EXEC_VIOLATION          (1UL<<_EPT_EXEC_VIOLATION)
> -#define _EPT_EFFECTIVE_READ         3
> -#define EPT_EFFECTIVE_READ          (1UL<<_EPT_EFFECTIVE_READ)
> -#define _EPT_EFFECTIVE_WRITE        4
> -#define EPT_EFFECTIVE_WRITE         (1UL<<_EPT_EFFECTIVE_WRITE)
> -#define _EPT_EFFECTIVE_EXEC         5
> -#define EPT_EFFECTIVE_EXEC          (1UL<<_EPT_EFFECTIVE_EXEC)
> -#define _EPT_GLA_VALID              7
> -#define EPT_GLA_VALID               (1UL<<_EPT_GLA_VALID)
> -#define _EPT_GLA_FAULT              8
> -#define EPT_GLA_FAULT               (1UL<<_EPT_GLA_FAULT)
> +typedef union __transparent__ ept_qual {
> +    unsigned long raw;
> +    struct {
> +        bool read:1, write:1, fetch:1,
> +            eff_read:1, eff_write:1, eff_exec:1, /* eff_user_exec */:1,
> +            gla_valid:1,
> +            gla_fault:1; /* Valid iff gla_valid. */
> +    };
> +} ept_qual_t;
>  
>  #define EPT_L4_PAGETABLE_SHIFT      39
>  #define EPT_PAGETABLE_ENTRIES       512
> diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
> index 33f0b96..e800709 100644
> --- a/xen/include/xen/compiler.h
> +++ b/xen/include/xen/compiler.h
> @@ -47,6 +47,7 @@
>  
>  #define __attribute_pure__  __attribute__((__pure__))
>  #define __attribute_const__ __attribute__((__const__))
> +#define __transparent__     __attribute__((__transparent_union__))
>  
>  /*
>   * The difference between the following two attributes is that __used is


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

Re: [PATCH v2 1/2] x86/vmx: Introduce a bitfield structure for EPT_VIOLATION EXIT_QUALIFICATIONs

[Jan Beulich]

>>> On 08.02.17 at 11:44, <andrew.cooper3@citrix.com> wrote:
> On 08/02/17 10:42, Andrew Cooper wrote:
>> This results in rather more readable code.  No functional change.
>>
>> All fields currently specified are included, but commented out as no support
>> for their use is present.
> 
> Apologies - sent a slightly stale version of the patch.  I have dropped
> this paragraph from the commit message, but the code is correct for v2.

With that and despite ...

>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Jun Nakajima <jun.nakajima@intel.com>
>> CC: Kevin Tian <kevin.tian@intel.com>
>>
>> v2:
>>  * Use a transparent union rather than modifying the caller of
>>    ept_handle_violation()
>>  * Drop the extranious commented out bitfield names, but keep eff_user_exec so
>>    gla_{valid,fault} are appropriately located.

... this not really being what Kevin and I had asked for,
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH v2 1/2] x86/vmx: Introduce a bitfield structure for EPT_VIOLATION EXIT_QUALIFICATIONs

[Andrew Cooper]

On 08/02/17 11:53, Jan Beulich wrote:
>>>> On 08.02.17 at 11:44, <andrew.cooper3@citrix.com> wrote:
>> On 08/02/17 10:42, Andrew Cooper wrote:
>>> This results in rather more readable code.  No functional change.
>>>
>>> All fields currently specified are included, but commented out as no support
>>> for their use is present.
>> Apologies - sent a slightly stale version of the patch.  I have dropped
>> this paragraph from the commit message, but the code is correct for v2.
> With that and despite ...
>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Jun Nakajima <jun.nakajima@intel.com>
>>> CC: Kevin Tian <kevin.tian@intel.com>
>>>
>>> v2:
>>>  * Use a transparent union rather than modifying the caller of
>>>    ept_handle_violation()
>>>  * Drop the extranious commented out bitfield names, but keep eff_user_exec so
>>>    gla_{valid,fault} are appropriately located.
> ... this not really being what Kevin and I had asked for,
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

In which case I am confused?  What were you asking for if it isn't this?

~Andrew

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

Re: [PATCH v2 1/2] x86/vmx: Introduce a bitfield structure for EPT_VIOLATION EXIT_QUALIFICATIONs

[Jan Beulich]

>>> On 08.02.17 at 12:56, <andrew.cooper3@citrix.com> wrote:
> On 08/02/17 11:53, Jan Beulich wrote:
>>>>> On 08.02.17 at 11:44, <andrew.cooper3@citrix.com> wrote:
>>> On 08/02/17 10:42, Andrew Cooper wrote:
>>>> This results in rather more readable code.  No functional change.
>>>>
>>>> All fields currently specified are included, but commented out as no support
>>>> for their use is present.
>>> Apologies - sent a slightly stale version of the patch.  I have dropped
>>> this paragraph from the commit message, but the code is correct for v2.
>> With that and despite ...
>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Jun Nakajima <jun.nakajima@intel.com>
>>>> CC: Kevin Tian <kevin.tian@intel.com>
>>>>
>>>> v2:
>>>>  * Use a transparent union rather than modifying the caller of
>>>>    ept_handle_violation()
>>>>  * Drop the extranious commented out bitfield names, but keep eff_user_exec so
>>>>    gla_{valid,fault} are appropriately located.
>> ... this not really being what Kevin and I had asked for,
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> In which case I am confused?  What were you asking for if it isn't this?

The request was to keep commented out just the one field use of
which needs enabling work elsewhere, but present as normal
(named) fields the three higher ones we currently lack.

Jan


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

Re: [PATCH v2 1/2] x86/vmx: Introduce a bitfield structure for EPT_VIOLATION EXIT_QUALIFICATIONs

[Andrew Cooper]

On 08/02/17 12:30, Jan Beulich wrote:
>>>> On 08.02.17 at 12:56, <andrew.cooper3@citrix.com> wrote:
>> On 08/02/17 11:53, Jan Beulich wrote:
>>>>>> On 08.02.17 at 11:44, <andrew.cooper3@citrix.com> wrote:
>>>> On 08/02/17 10:42, Andrew Cooper wrote:
>>>>> This results in rather more readable code.  No functional change.
>>>>>
>>>>> All fields currently specified are included, but commented out as no support
>>>>> for their use is present.
>>>> Apologies - sent a slightly stale version of the patch.  I have dropped
>>>> this paragraph from the commit message, but the code is correct for v2.
>>> With that and despite ...
>>>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> ---
>>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>>> CC: Jun Nakajima <jun.nakajima@intel.com>
>>>>> CC: Kevin Tian <kevin.tian@intel.com>
>>>>>
>>>>> v2:
>>>>>  * Use a transparent union rather than modifying the caller of
>>>>>    ept_handle_violation()
>>>>>  * Drop the extranious commented out bitfield names, but keep eff_user_exec so
>>>>>    gla_{valid,fault} are appropriately located.
>>> ... this not really being what Kevin and I had asked for,
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> In which case I am confused?  What were you asking for if it isn't this?
> The request was to keep commented out just the one field use of
> which needs enabling work elsewhere, but present as normal
> (named) fields the three higher ones we currently lack.

All of the originally commented out bits need further work in Xen to enable.

The higher bits are undefined if advanced EPT Violation VM-exit
information is unavailable in hardware, meaning that they are not safe
to use currently.

~Andrew

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

Re: [PATCH v2 1/2] x86/vmx: Introduce a bitfield structure for EPT_VIOLATION EXIT_QUALIFICATIONs

[Jan Beulich]

>>> On 08.02.17 at 14:03, <andrew.cooper3@citrix.com> wrote:
> On 08/02/17 12:30, Jan Beulich wrote:
>>>>> On 08.02.17 at 12:56, <andrew.cooper3@citrix.com> wrote:
>>> On 08/02/17 11:53, Jan Beulich wrote:
>>>>>>> On 08.02.17 at 11:44, <andrew.cooper3@citrix.com> wrote:
>>>>> On 08/02/17 10:42, Andrew Cooper wrote:
>>>>>> This results in rather more readable code.  No functional change.
>>>>>>
>>>>>> All fields currently specified are included, but commented out as no support
>>>>>> for their use is present.
>>>>> Apologies - sent a slightly stale version of the patch.  I have dropped
>>>>> this paragraph from the commit message, but the code is correct for v2.
>>>> With that and despite ...
>>>>
>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>> ---
>>>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>>>> CC: Jun Nakajima <jun.nakajima@intel.com>
>>>>>> CC: Kevin Tian <kevin.tian@intel.com>
>>>>>>
>>>>>> v2:
>>>>>>  * Use a transparent union rather than modifying the caller of
>>>>>>    ept_handle_violation()
>>>>>>  * Drop the extranious commented out bitfield names, but keep eff_user_exec so
>>>>>>    gla_{valid,fault} are appropriately located.
>>>> ... this not really being what Kevin and I had asked for,
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> In which case I am confused?  What were you asking for if it isn't this?
>> The request was to keep commented out just the one field use of
>> which needs enabling work elsewhere, but present as normal
>> (named) fields the three higher ones we currently lack.
> 
> All of the originally commented out bits need further work in Xen to enable.
> 
> The higher bits are undefined if advanced EPT Violation VM-exit
> information is unavailable in hardware, meaning that they are not safe
> to use currently.

Bit 6 requires a control to be turned on, but the same doesn't hold
for bits 9..11 according to my reading of the SDM. So consuming the
bits would, as an integral part, require an additional feature check,
but that is entirely independent of naming these fields.

And for bit 12 I don't see how it would be dangerous to give a name
in any event (it's even shared with at least one other exit qual).

Jan


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

Re: [PATCH v2 1/2] x86/vmx: Introduce a bitfield structure for EPT_VIOLATION EXIT_QUALIFICATIONs

[Andrew Cooper]

On 08/02/17 13:28, Jan Beulich wrote:
>>>> On 08.02.17 at 14:03, <andrew.cooper3@citrix.com> wrote:
>> On 08/02/17 12:30, Jan Beulich wrote:
>>>>>> On 08.02.17 at 12:56, <andrew.cooper3@citrix.com> wrote:
>>>> On 08/02/17 11:53, Jan Beulich wrote:
>>>>>>>> On 08.02.17 at 11:44, <andrew.cooper3@citrix.com> wrote:
>>>>>> On 08/02/17 10:42, Andrew Cooper wrote:
>>>>>>> This results in rather more readable code.  No functional change.
>>>>>>>
>>>>>>> All fields currently specified are included, but commented out as no support
>>>>>>> for their use is present.
>>>>>> Apologies - sent a slightly stale version of the patch.  I have dropped
>>>>>> this paragraph from the commit message, but the code is correct for v2.
>>>>> With that and despite ...
>>>>>
>>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>>> ---
>>>>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>>>>> CC: Jun Nakajima <jun.nakajima@intel.com>
>>>>>>> CC: Kevin Tian <kevin.tian@intel.com>
>>>>>>>
>>>>>>> v2:
>>>>>>>  * Use a transparent union rather than modifying the caller of
>>>>>>>    ept_handle_violation()
>>>>>>>  * Drop the extranious commented out bitfield names, but keep eff_user_exec so
>>>>>>>    gla_{valid,fault} are appropriately located.
>>>>> ... this not really being what Kevin and I had asked for,
>>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>> In which case I am confused?  What were you asking for if it isn't this?
>>> The request was to keep commented out just the one field use of
>>> which needs enabling work elsewhere, but present as normal
>>> (named) fields the three higher ones we currently lack.
>> All of the originally commented out bits need further work in Xen to enable.
>>
>> The higher bits are undefined if advanced EPT Violation VM-exit
>> information is unavailable in hardware, meaning that they are not safe
>> to use currently.
> Bit 6 requires a control to be turned on, but the same doesn't hold
> for bits 9..11 according to my reading of the SDM.

This matches my reading.

> So consuming the bits would, as an integral part, require an additional feature check,
> but that is entirely independent of naming these fields.

Without a new vmx_ept_vpid_cap-based predicate, it is unsafe to read
those bits, because they are not guaranteed to be zero.  (It is curious
that some fields are documented as reserved, and should be zero, while
these are documented as undefined.)

I purposefully don't want to make them available, because forcing
someone to modify ept_qual_t to use them will make it less likely that
we miss the above requirement on the review of the first patch which
starts to use them.

> And for bit 12 I don't see how it would be dangerous to give a name
> in any event (it's even shared with at least one other exit qual).

This particular bit falls into the grey category of "despite having a
clear name, its not obvious what to do with it".  I haven't yet found
any indication in the manuals as to whether it is purely informative, or
whether action needs to be taken.

~Andrew

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

Re: [PATCH v2 1/2] x86/vmx: Introduce a bitfield structure for EPT_VIOLATION EXIT_QUALIFICATIONs

[Jan Beulich]

>>> On 08.02.17 at 16:04, <andrew.cooper3@citrix.com> wrote:
> On 08/02/17 13:28, Jan Beulich wrote:
>>>>> On 08.02.17 at 14:03, <andrew.cooper3@citrix.com> wrote:
>>> On 08/02/17 12:30, Jan Beulich wrote:
>>>>>>> On 08.02.17 at 12:56, <andrew.cooper3@citrix.com> wrote:
>>>>> On 08/02/17 11:53, Jan Beulich wrote:
>>>>>>>>> On 08.02.17 at 11:44, <andrew.cooper3@citrix.com> wrote:
>>>>>>> On 08/02/17 10:42, Andrew Cooper wrote:
>>>>>>>> This results in rather more readable code.  No functional change.
>>>>>>>>
>>>>>>>> All fields currently specified are included, but commented out as no support
>>>>>>>> for their use is present.
>>>>>>> Apologies - sent a slightly stale version of the patch.  I have dropped
>>>>>>> this paragraph from the commit message, but the code is correct for v2.
>>>>>> With that and despite ...
>>>>>>
>>>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>>>> ---
>>>>>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>>>>>> CC: Jun Nakajima <jun.nakajima@intel.com>
>>>>>>>> CC: Kevin Tian <kevin.tian@intel.com>
>>>>>>>>
>>>>>>>> v2:
>>>>>>>>  * Use a transparent union rather than modifying the caller of
>>>>>>>>    ept_handle_violation()
>>>>>>>>  * Drop the extranious commented out bitfield names, but keep eff_user_exec 
> so
>>>>>>>>    gla_{valid,fault} are appropriately located.
>>>>>> ... this not really being what Kevin and I had asked for,
>>>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>>> In which case I am confused?  What were you asking for if it isn't this?
>>>> The request was to keep commented out just the one field use of
>>>> which needs enabling work elsewhere, but present as normal
>>>> (named) fields the three higher ones we currently lack.
>>> All of the originally commented out bits need further work in Xen to enable.
>>>
>>> The higher bits are undefined if advanced EPT Violation VM-exit
>>> information is unavailable in hardware, meaning that they are not safe
>>> to use currently.
>> Bit 6 requires a control to be turned on, but the same doesn't hold
>> for bits 9..11 according to my reading of the SDM.
> 
> This matches my reading.
> 
>> So consuming the bits would, as an integral part, require an additional 
> feature check,
>> but that is entirely independent of naming these fields.
> 
> Without a new vmx_ept_vpid_cap-based predicate, it is unsafe to read
> those bits, because they are not guaranteed to be zero.  (It is curious
> that some fields are documented as reserved, and should be zero, while
> these are documented as undefined.)
> 
> I purposefully don't want to make them available, because forcing
> someone to modify ept_qual_t to use them will make it less likely that
> we miss the above requirement on the review of the first patch which
> starts to use them.

Well, as indicated by my R-b I'm fine with the patch going on as is.

>> And for bit 12 I don't see how it would be dangerous to give a name
>> in any event (it's even shared with at least one other exit qual).
> 
> This particular bit falls into the grey category of "despite having a
> clear name, its not obvious what to do with it".  I haven't yet found
> any indication in the manuals as to whether it is purely informative, or
> whether action needs to be taken.

I'm pretty sure it means we need to take action, but we simply
neglect to do so right now. Part of the reason likely being that
people involved here either don't know what action we're meant
to take or don't care enough to provide a patch.

Jan


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

Re: [PATCH v2 1/2] x86/vmx: Introduce a bitfield structure for EPT_VIOLATION EXIT_QUALIFICATIONs

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Wednesday, February 08, 2017 6:43 PM
> 
> This results in rather more readable code.  No functional change.
> 
> All fields currently specified are included, but commented out as no support
> for their use is present.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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