[PATCH 0/4] x86/vvmx: correctly emulate VMREAD and VMWRITE

[PATCH 0/4] x86/vvmx: correctly emulate VMREAD and VMWRITE

[Sergey Dyasli]

Currently, emulation of vmread and vmwrite for a guest leads to BUG()
in cases when actual VMREAD or VMWRITE ends up in VMfail due to invalid
arguments.  The goal of this patch series is to prevent the BUG() from
happening and report any kind of VMfail back to the guest, just like
it would be done by H/W.

Sergey Dyasli (4):
  x86/vmx: introduce __vmwrite_safe()
  x86/vmx: improve __vmread_safe()
  x86/vvmx: correctly emulate VMWRITE
  x86/vvmx: correctly emulate VMREAD

 tools/tests/x86_emulator/x86_emulate.h |  6 +++
 xen/arch/x86/hvm/vmx/vmcs.c            | 20 ++++++----
 xen/arch/x86/hvm/vmx/vvmx.c            | 46 +++++++++++++++++-----
 xen/arch/x86/x86_emulate/x86_emulate.c |  6 ---
 xen/include/asm-x86/hvm/vmx/vmcs.h     |  6 ++-
 xen/include/asm-x86/hvm/vmx/vmx.h      | 72 ++++++++++++++++++++++------------
 xen/include/asm-x86/hvm/vmx/vvmx.h     | 11 +++++-
 xen/include/xen/lib.h                  |  6 +++
 8 files changed, 123 insertions(+), 50 deletions(-)

-- 
2.7.4


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

[PATCH 1/4] x86/vmx: introduce __vmwrite_safe()

[Sergey Dyasli]

Any fail during the original __vmwrite() leads to BUG() which can be
easily exploited from a guest in the nested vmx mode.

The new function returns error code depending on the outcome:

          VMsucceed: 0
        VMfailValid: VM Instruction Error Number
      VMfailInvalid: a new VMX_INSN_FAIL_INVALID

A new macro GAS_VMX_OP is introduced in order to improve the readability
of asm.  Existing ASM_FLAG_OUT macro is reused and moved into lib.h
and x86_emulate.h

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 tools/tests/x86_emulator/x86_emulate.h |  6 ++++++
 xen/arch/x86/hvm/vmx/vvmx.c            |  3 ++-
 xen/arch/x86/x86_emulate/x86_emulate.c |  6 ------
 xen/include/asm-x86/hvm/vmx/vmcs.h     |  1 +
 xen/include/asm-x86/hvm/vmx/vmx.h      | 29 +++++++++++++++++++++++++++++
 xen/include/xen/lib.h                  |  6 ++++++
 6 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/tools/tests/x86_emulator/x86_emulate.h b/tools/tests/x86_emulator/x86_emulate.h
index 3a6bade..6524bb3 100644
--- a/tools/tests/x86_emulator/x86_emulate.h
+++ b/tools/tests/x86_emulator/x86_emulate.h
@@ -46,6 +46,12 @@
 #define MMAP_SZ 16384
 bool emul_test_make_stack_executable(void);
 
+#ifdef __GCC_ASM_FLAG_OUTPUTS__
+# define ASM_FLAG_OUT(yes, no) yes
+#else
+# define ASM_FLAG_OUT(yes, no) no
+#endif
+
 #include "x86_emulate/x86_emulate.h"
 
 static inline uint64_t xgetbv(uint32_t xcr)
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 9caebe5..105a3c0 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -483,7 +483,8 @@ static void vmfail_invalid(struct cpu_user_regs *regs)
 
 static void vmfail(struct cpu_user_regs *regs, enum vmx_insn_errno errno)
 {
-    if ( vcpu_nestedhvm(current).nv_vvmcxaddr != INVALID_PADDR )
+    if ( vcpu_nestedhvm(current).nv_vvmcxaddr != INVALID_PADDR &&
+         errno != VMX_INSN_FAIL_INVALID )
         vmfail_valid(regs, errno);
     else
         vmfail_invalid(regs);
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 5bb5bdf..d133f09 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -409,12 +409,6 @@ typedef union {
         (void *)((long)(__##var + __alignof(type) - __alignof(long)) \
                  & -__alignof(type))
 
-#ifdef __GCC_ASM_FLAG_OUTPUTS__
-# define ASM_FLAG_OUT(yes, no) yes
-#else
-# define ASM_FLAG_OUT(yes, no) no
-#endif
-
 /* MSRs. */
 #define MSR_TSC          0x00000010
 #define MSR_SYSENTER_CS  0x00000174
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index d71de04..cb02d53 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -526,6 +526,7 @@ enum vmx_insn_errno
     VMX_INSN_VMPTRLD_INVALID_PHYADDR       = 9,
     VMX_INSN_UNSUPPORTED_VMCS_COMPONENT    = 12,
     VMX_INSN_VMXON_IN_VMX_ROOT             = 15,
+    VMX_INSN_FAIL_INVALID                  = ~0UL,
 };
 
 void vmx_disable_intercept_for_msr(struct vcpu *v, u32 msr, int type);
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index e5c6499..b0f3f3c 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -306,6 +306,12 @@ extern uint8_t posted_intr_vector;
 #define INVVPID_ALL_CONTEXT                     2
 #define INVVPID_SINGLE_CONTEXT_RETAINING_GLOBAL 3
 
+#ifdef HAVE_GAS_VMX
+# define GAS_VMX_OP(yes, no) yes
+#else
+# define GAS_VMX_OP(yes, no) no
+#endif
+
 static always_inline void __vmptrld(u64 addr)
 {
     asm volatile (
@@ -423,6 +429,29 @@ static inline bool_t __vmread_safe(unsigned long field, unsigned long *value)
     return okay;
 }
 
+static always_inline unsigned long __vmwrite_safe(unsigned long field,
+                                                  unsigned long value)
+{
+    unsigned long ret = 0;
+    bool fail_invalid, fail_valid;
+
+    asm volatile ( GAS_VMX_OP("vmwrite %[value], %[field]\n",
+                              VMWRITE_OPCODE MODRM_EAX_ECX)
+                   ASM_FLAG_OUT(, "setb %[fail_invalid]\n")
+                   ASM_FLAG_OUT(, "sete %[fail_valid]\n")
+                   : ASM_FLAG_OUT("=@ccb", [fail_invalid] "=rm") (fail_invalid),
+                     ASM_FLAG_OUT("=@cce", [fail_valid] "=rm") (fail_valid)
+                   : [field] GAS_VMX_OP("r", "a") (field),
+                     [value] GAS_VMX_OP("rm", "c") (value));
+
+    if ( fail_invalid )
+        ret = VMX_INSN_FAIL_INVALID;
+    else if ( fail_valid )
+        __vmread(VM_INSTRUCTION_ERROR, &ret);
+
+    return ret;
+}
+
 static always_inline void __invept(unsigned long type, u64 eptp, u64 gpa)
 {
     struct {
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index d1171b7..6f6cac2 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -162,4 +162,10 @@ void init_constructors(void);
 void *bsearch(const void *key, const void *base, size_t num, size_t size,
               int (*cmp)(const void *key, const void *elt));
 
+#ifdef __GCC_ASM_FLAG_OUTPUTS__
+# define ASM_FLAG_OUT(yes, no) yes
+#else
+# define ASM_FLAG_OUT(yes, no) no
+#endif
+
 #endif /* __LIB_H__ */
-- 
2.7.4


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

[PATCH 2/4] x86/vmx: improve __vmread_safe()

[Sergey Dyasli]

The original function doesn't distinguish between Valid and Invalid
VMfails.  Improved function returns error code depending on the outcome:

        VMsucceed: 0
      VMfailValid: VM Instruction Error Number
    VMfailInvalid: VMX_INSN_FAIL_INVALID (~0UL)

Existing users of __vmread_safe() are updated.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c       |  2 +-
 xen/arch/x86/hvm/vmx/vvmx.c       |  2 +-
 xen/include/asm-x86/hvm/vmx/vmx.h | 41 +++++++++++++++++----------------------
 3 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 59ef199..641e2ef 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1711,7 +1711,7 @@ static inline unsigned long vmr(unsigned long field)
 {
     unsigned long val;
 
-    return __vmread_safe(field, &val) ? val : 0;
+    return __vmread_safe(field, &val) ? 0 : val;
 }
 
 #define vmr16(fld) ({             \
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 105a3c0..e0cf0fb 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -932,7 +932,7 @@ static inline void shadow_to_vvmcs(const struct vcpu *v, unsigned int field)
 {
     unsigned long value;
 
-    if ( __vmread_safe(field, &value) )
+    if ( __vmread_safe(field, &value) == 0 )
         set_vvmcs(v, field, value);
 }
 
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index b0f3f3c..8d871cf 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -401,32 +401,27 @@ static always_inline void __vmwrite(unsigned long field, unsigned long value)
         );
 }
 
-static inline bool_t __vmread_safe(unsigned long field, unsigned long *value)
+static inline unsigned long __vmread_safe(unsigned long field,
+                                          unsigned long *value)
 {
-    bool_t okay;
+    unsigned long ret = 0;
+    bool fail_invalid, fail_valid;
 
-    asm volatile (
-#ifdef HAVE_GAS_VMX
-                   "vmread %2, %1\n\t"
-#else
-                   VMREAD_OPCODE MODRM_EAX_ECX
-#endif
-                   /* CF==1 or ZF==1 --> rc = 0 */
-#ifdef __GCC_ASM_FLAG_OUTPUTS__
-                   : "=@ccnbe" (okay),
-#else
-                   "setnbe %0"
-                   : "=qm" (okay),
-#endif
-#ifdef HAVE_GAS_VMX
-                     "=rm" (*value)
-                   : "r" (field));
-#else
-                     "=c" (*value)
-                   : "a" (field));
-#endif
+    asm volatile ( GAS_VMX_OP("vmread %[field], %[value]\n",
+                              VMREAD_OPCODE MODRM_EAX_ECX)
+                   ASM_FLAG_OUT(, "setb %[fail_invalid]\n")
+                   ASM_FLAG_OUT(, "sete %[fail_valid]\n")
+                   : ASM_FLAG_OUT("=@ccb", [fail_invalid] "=rm") (fail_invalid),
+                     ASM_FLAG_OUT("=@cce", [fail_valid] "=rm") (fail_valid),
+                     [value] GAS_VMX_OP("=rm", "=c") (*value)
+                   : [field] GAS_VMX_OP("r", "a") (field));
+
+    if ( fail_invalid )
+        ret = VMX_INSN_FAIL_INVALID;
+    else if ( fail_valid )
+        __vmread(VM_INSTRUCTION_ERROR, &ret);
 
-    return okay;
+    return ret;
 }
 
 static always_inline unsigned long __vmwrite_safe(unsigned long field,
-- 
2.7.4


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

[PATCH 3/4] x86/vvmx: correctly emulate VMWRITE

[Sergey Dyasli]

There is an issue with the original __vmwrite() in nested vmx mode:
emulation of a guest's VMWRITE with invalid arguments leads to BUG().

Fix this by using __vmwrite_safe() and reporting any kind of VMfail
back to the guest.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c        |  9 +++++++--
 xen/arch/x86/hvm/vmx/vvmx.c        | 15 +++++++++++----
 xen/include/asm-x86/hvm/vmx/vmcs.h |  2 +-
 xen/include/asm-x86/hvm/vmx/vvmx.h |  4 ++--
 4 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 641e2ef..c1f09c6 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -943,11 +943,16 @@ u64 virtual_vmcs_vmread(const struct vcpu *v, u32 vmcs_encoding)
     return res;
 }
 
-void virtual_vmcs_vmwrite(const struct vcpu *v, u32 vmcs_encoding, u64 val)
+unsigned long virtual_vmcs_vmwrite(const struct vcpu *v, u32 vmcs_encoding,
+                                   u64 val)
 {
+    unsigned long ret;
+
     virtual_vmcs_enter(v);
-    __vmwrite(vmcs_encoding, val);
+    ret = __vmwrite_safe(vmcs_encoding, val);
     virtual_vmcs_exit(v);
+
+    return ret;
 }
 
 /*
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index e0cf0fb..c427a24 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -264,7 +264,7 @@ u64 get_vvmcs_real(const struct vcpu *v, u32 encoding)
     return virtual_vmcs_vmread(v, encoding);
 }
 
-void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val)
+unsigned long set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val)
 {
     union vmcs_encoding enc;
     u64 *content = (u64 *) vvmcs;
@@ -298,11 +298,13 @@ void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val)
     }
 
     content[offset] = res;
+
+    return 0;
 }
 
-void set_vvmcs_real(const struct vcpu *v, u32 encoding, u64 val)
+unsigned long set_vvmcs_real(const struct vcpu *v, u32 encoding, u64 val)
 {
-    virtual_vmcs_vmwrite(v, encoding, val);
+    return virtual_vmcs_vmwrite(v, encoding, val);
 }
 
 static unsigned long reg_read(struct cpu_user_regs *regs,
@@ -1740,13 +1742,18 @@ int nvmx_handle_vmwrite(struct cpu_user_regs *regs)
     unsigned long operand; 
     u64 vmcs_encoding;
     bool_t okay = 1;
+    unsigned long err;
 
     if ( decode_vmx_inst(regs, &decode, &operand, 0)
              != X86EMUL_OKAY )
         return X86EMUL_EXCEPTION;
 
     vmcs_encoding = reg_read(regs, decode.reg2);
-    set_vvmcs(v, vmcs_encoding, operand);
+    if ( (err = set_vvmcs(v, vmcs_encoding, operand)) )
+    {
+        vmfail(regs, err);
+        return X86EMUL_OKAY;
+    }
 
     switch ( vmcs_encoding & ~VMCS_HIGH(0) )
     {
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index cb02d53..b30e0f7 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -541,7 +541,7 @@ int vmx_check_msr_bitmap(unsigned long *msr_bitmap, u32 msr, int access_type);
 void virtual_vmcs_enter(const struct vcpu *);
 void virtual_vmcs_exit(const struct vcpu *);
 u64 virtual_vmcs_vmread(const struct vcpu *, u32 encoding);
-void virtual_vmcs_vmwrite(const struct vcpu *, u32 encoding, u64 val);
+unsigned long virtual_vmcs_vmwrite(const struct vcpu *, u32 encoding, u64 val);
 
 static inline int vmx_add_guest_msr(u32 msr)
 {
diff --git a/xen/include/asm-x86/hvm/vmx/vvmx.h b/xen/include/asm-x86/hvm/vmx/vvmx.h
index 242e524..d60e0bb 100644
--- a/xen/include/asm-x86/hvm/vmx/vvmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vvmx.h
@@ -181,8 +181,8 @@ enum vvmcs_encoding_type {
 
 u64 get_vvmcs_virtual(void *vvmcs, u32 encoding);
 u64 get_vvmcs_real(const struct vcpu *, u32 encoding);
-void set_vvmcs_virtual(void *vvmcs, u32 encoding, u64 val);
-void set_vvmcs_real(const struct vcpu *, u32 encoding, u64 val);
+unsigned long set_vvmcs_virtual(void *vvmcs, u32 encoding, u64 val);
+unsigned long set_vvmcs_real(const struct vcpu *, u32 encoding, u64 val);
 
 #define get_vvmcs(vcpu, encoding) \
   (cpu_has_vmx_vmcs_shadowing ? \
-- 
2.7.4


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

[PATCH 4/4] x86/vvmx: correctly emulate VMREAD

[Sergey Dyasli]

There is an issue with the original __vmread() in nested vmx mode:
emulation of a guest's VMREAD with invalid arguments leads to BUG().

Fix this by using __vmread_safe() and reporting any kind of VMfail
back to the guest.

A new safe versions of get_vvmcs() macro and related functions are
introduced because of new function signatures and lots of existing
users.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/vmx/vmcs.c        |  9 +++++----
 xen/arch/x86/hvm/vmx/vvmx.c        | 26 +++++++++++++++++++++++---
 xen/include/asm-x86/hvm/vmx/vmcs.h |  3 ++-
 xen/include/asm-x86/hvm/vmx/vvmx.h |  7 +++++++
 4 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index c1f09c6..8d49c89 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -932,15 +932,16 @@ void virtual_vmcs_exit(const struct vcpu *v)
         __vmptrld(cur);
 }
 
-u64 virtual_vmcs_vmread(const struct vcpu *v, u32 vmcs_encoding)
+unsigned long virtual_vmcs_vmread(const struct vcpu *v, u32 vmcs_encoding,
+                                  u64 *val)
 {
-    u64 res;
+    unsigned long ret;
 
     virtual_vmcs_enter(v);
-    __vmread(vmcs_encoding, &res);
+    ret = __vmread_safe(vmcs_encoding, val);
     virtual_vmcs_exit(v);
 
-    return res;
+    return ret;
 }
 
 unsigned long virtual_vmcs_vmwrite(const struct vcpu *v, u32 vmcs_encoding,
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index c427a24..e7f5010 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -261,7 +261,23 @@ u64 get_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding)
 
 u64 get_vvmcs_real(const struct vcpu *v, u32 encoding)
 {
-    return virtual_vmcs_vmread(v, encoding);
+    u64 val;
+
+    virtual_vmcs_vmread(v, encoding, &val);
+
+    return val;
+}
+
+unsigned long get_vvmcs_virtual_safe(void *vvmcs, u32 encoding, u64 *val)
+{
+    *val = get_vvmcs_virtual(vvmcs, encoding);
+
+    return 0;
+}
+
+unsigned long get_vvmcs_real_safe(const struct vcpu *v, u32 encoding, u64 *val)
+{
+    return virtual_vmcs_vmread(v, encoding, val);
 }
 
 unsigned long set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val)
@@ -1710,13 +1726,17 @@ int nvmx_handle_vmread(struct cpu_user_regs *regs)
     struct vmx_inst_decoded decode;
     pagefault_info_t pfinfo;
     u64 value = 0;
-    int rc;
+    unsigned long rc;
 
     rc = decode_vmx_inst(regs, &decode, NULL, 0);
     if ( rc != X86EMUL_OKAY )
         return rc;
 
-    value = get_vvmcs(v, reg_read(regs, decode.reg2));
+    if ( (rc = get_vvmcs_safe(v, reg_read(regs, decode.reg2), &value)) )
+    {
+        vmfail(regs, rc);
+        return X86EMUL_OKAY;
+    }
 
     switch ( decode.type ) {
     case VMX_INST_MEMREG_TYPE_MEMORY:
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index b30e0f7..7c57c00 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -540,7 +540,8 @@ void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector);
 int vmx_check_msr_bitmap(unsigned long *msr_bitmap, u32 msr, int access_type);
 void virtual_vmcs_enter(const struct vcpu *);
 void virtual_vmcs_exit(const struct vcpu *);
-u64 virtual_vmcs_vmread(const struct vcpu *, u32 encoding);
+unsigned long virtual_vmcs_vmread(const struct vcpu *v, u32 vmcs_encoding,
+                                  u64 *val);
 unsigned long virtual_vmcs_vmwrite(const struct vcpu *, u32 encoding, u64 val);
 
 static inline int vmx_add_guest_msr(u32 msr)
diff --git a/xen/include/asm-x86/hvm/vmx/vvmx.h b/xen/include/asm-x86/hvm/vmx/vvmx.h
index d60e0bb..28e2503 100644
--- a/xen/include/asm-x86/hvm/vmx/vvmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vvmx.h
@@ -181,6 +181,8 @@ enum vvmcs_encoding_type {
 
 u64 get_vvmcs_virtual(void *vvmcs, u32 encoding);
 u64 get_vvmcs_real(const struct vcpu *, u32 encoding);
+unsigned long get_vvmcs_virtual_safe(void *vvmcs, u32 encoding, u64 *val);
+unsigned long get_vvmcs_real_safe(const struct vcpu *, u32 encoding, u64 *val);
 unsigned long set_vvmcs_virtual(void *vvmcs, u32 encoding, u64 val);
 unsigned long set_vvmcs_real(const struct vcpu *, u32 encoding, u64 val);
 
@@ -194,6 +196,11 @@ unsigned long set_vvmcs_real(const struct vcpu *, u32 encoding, u64 val);
    set_vvmcs_real(vcpu, encoding, val) : \
    set_vvmcs_virtual(vcpu_nestedhvm(vcpu).nv_vvmcx, encoding, val))
 
+#define get_vvmcs_safe(vcpu, encoding, val) \
+  (cpu_has_vmx_vmcs_shadowing ? \
+   get_vvmcs_real_safe(vcpu, encoding, val) : \
+   get_vvmcs_virtual_safe(vcpu_nestedhvm(vcpu).nv_vvmcx, encoding, val))
+
 uint64_t get_shadow_eptp(struct vcpu *v);
 
 void nvmx_destroy_vmcs(struct vcpu *v);
-- 
2.7.4


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

Re: [PATCH 1/4] x86/vmx: introduce __vmwrite_safe()

[Jan Beulich]

>>> On 31.01.17 at 12:20, <sergey.dyasli@citrix.com> wrote:
> --- a/tools/tests/x86_emulator/x86_emulate.h
> +++ b/tools/tests/x86_emulator/x86_emulate.h
> @@ -46,6 +46,12 @@
>  #define MMAP_SZ 16384
>  bool emul_test_make_stack_executable(void);
>  
> +#ifdef __GCC_ASM_FLAG_OUTPUTS__
> +# define ASM_FLAG_OUT(yes, no) yes
> +#else
> +# define ASM_FLAG_OUT(yes, no) no
> +#endif

I ought to be sufficient to put this in
tools/tests/x86_emulator/x86_emulate.c - no need for other
consumers of this header to see it.

> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -526,6 +526,7 @@ enum vmx_insn_errno
>      VMX_INSN_VMPTRLD_INVALID_PHYADDR       = 9,
>      VMX_INSN_UNSUPPORTED_VMCS_COMPONENT    = 12,
>      VMX_INSN_VMXON_IN_VMX_ROOT             = 15,
> +    VMX_INSN_FAIL_INVALID                  = ~0UL,
>  };

Is the UL really need here? I'd think -1, ~0, or ~0U to suffice.

> @@ -423,6 +429,29 @@ static inline bool_t __vmread_safe(unsigned long field, unsigned long *value)
>      return okay;
>  }
>  
> +static always_inline unsigned long __vmwrite_safe(unsigned long field,
> +                                                  unsigned long value)

Can we please avoid adding more of these (even double) underscore
prefixed symbols? They're reserved to the compiler, so we should
limit their use to places where we absolutely can't think of better
alternatives.

> +{
> +    unsigned long ret = 0;
> +    bool fail_invalid, fail_valid;
> +
> +    asm volatile ( GAS_VMX_OP("vmwrite %[value], %[field]\n",
> +                              VMWRITE_OPCODE MODRM_EAX_ECX)
> +                   ASM_FLAG_OUT(, "setb %[fail_invalid]\n")

While they're synonyms, I'd prefer setc (and @ccc below) to be
used here, as this is not the result of a comparison you're looking
at.

> +                   ASM_FLAG_OUT(, "sete %[fail_valid]\n")

Similarly setz / @ccz here / below

Also I think for the constraint names (but not the variable ones) you
could omit the fail_ prefixes, to help readability.

Plus, strictly speaking it is wrong for instruction mnemonics to start
in column zero. Granted we have many violations of this, but please
add \t here to avoid introducing more slightly wrong code.

> +                   : ASM_FLAG_OUT("=@ccb", [fail_invalid] "=rm") (fail_invalid),
> +                     ASM_FLAG_OUT("=@cce", [fail_valid] "=rm") (fail_valid)
> +                   : [field] GAS_VMX_OP("r", "a") (field),
> +                     [value] GAS_VMX_OP("rm", "c") (value));
> +
> +    if ( fail_invalid )
> +        ret = VMX_INSN_FAIL_INVALID;
> +    else if ( fail_valid )
> +        __vmread(VM_INSTRUCTION_ERROR, &ret);

If already you don't fold this into the asm(), please at least add
unlikely().

> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -162,4 +162,10 @@ void init_constructors(void);
>  void *bsearch(const void *key, const void *base, size_t num, size_t size,
>                int (*cmp)(const void *key, const void *elt));
>  
> +#ifdef __GCC_ASM_FLAG_OUTPUTS__
> +# define ASM_FLAG_OUT(yes, no) yes
> +#else
> +# define ASM_FLAG_OUT(yes, no) no
> +#endif

Is this useful on other than x86?

Jan


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

Re: [PATCH 2/4] x86/vmx: improve __vmread_safe()

[Jan Beulich]

>>> On 31.01.17 at 12:20, <sergey.dyasli@citrix.com> wrote:
> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -401,32 +401,27 @@ static always_inline void __vmwrite(unsigned long field, unsigned long value)
>          );
>  }
>  
> -static inline bool_t __vmread_safe(unsigned long field, unsigned long *value)
> +static inline unsigned long __vmread_safe(unsigned long field,
> +                                          unsigned long *value)

Along the lines of the comments to patch 1, please take the
opportunity and drop the stray leading underscores. Other comments
made there apply here too.

Jan


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

Re: [PATCH 3/4] x86/vvmx: correctly emulate VMWRITE

[Jan Beulich]

>>> On 31.01.17 at 12:20, <sergey.dyasli@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -264,7 +264,7 @@ u64 get_vvmcs_real(const struct vcpu *v, u32 encoding)
>      return virtual_vmcs_vmread(v, encoding);
>  }
>  
> -void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val)
> +unsigned long set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val)
>  {
>      union vmcs_encoding enc;
>      u64 *content = (u64 *) vvmcs;
> @@ -298,11 +298,13 @@ void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val)
>      }
>  
>      content[offset] = res;
> +
> +    return 0;
>  }

Is it reasonable for this to never fail? Correct emulation would imo
require this to signal invalid fields too. But yes, this could be dealt
with in a second step.

Jan


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

Re: [PATCH 3/4] x86/vvmx: correctly emulate VMWRITE

[Andrew Cooper]

On 02/02/17 12:52, Jan Beulich wrote:
>>>> On 31.01.17 at 12:20, <sergey.dyasli@citrix.com> wrote:
>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>> @@ -264,7 +264,7 @@ u64 get_vvmcs_real(const struct vcpu *v, u32 encoding)
>>      return virtual_vmcs_vmread(v, encoding);
>>  }
>>  
>> -void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val)
>> +unsigned long set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val)
>>  {
>>      union vmcs_encoding enc;
>>      u64 *content = (u64 *) vvmcs;
>> @@ -298,11 +298,13 @@ void set_vvmcs_virtual(void *vvmcs, u32 vmcs_encoding, u64 val)
>>      }
>>  
>>      content[offset] = res;
>> +
>> +    return 0;
>>  }
> Is it reasonable for this to never fail?

No.  Fields and values need to be audited against the features offered
to the guest in the VT-x MSRs.

However, it is not worth trying to fix that now before the MSR levelling
work is started, at which point we will have a cpuid_policy-like object
to query.

~Andrew

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