[PATCH 0/6] HVM Emulation and trap injection fixes

[PATCH 0/6] HVM Emulation and trap injection fixes

[Andrew Cooper]

This series covers issues discovered during the analysis of XSAs 105 and 106.

Patch 1 makes fixes to the SYS{CALL,ENTER,EXIT} emulation
Patches 2-4 provide support to inject software events from the emulator
Patch 5 introduces HVM Forced Emulation Prefix support to aid testing
Patch 6 is misc cleanup to svm_inject_trap()

As part of developing the software event injection emulation (and reverse
engineering AMD's behaviour), I developed a unit test.

The unit test tests:
 * icebp (0xf1)
 * int $1 (0xcd 0x01)
 * int3 (0xcc)
 * int $3 (0xcd 0x03)

In the following setups:
 * Regular instruction
 * Regular instruction with a redundant addr32 (0x67) prefix
 * Forced emulation prefix
 * Forced eumation and redundant addr32 prefix

Under the following conditions:
 * Ring0, all perms ok
 * Ring0, descriptor not present
 * Ring3, all perms ok
 * Ring3, descriptor not present
 * Ring3, dpl=0

Verifying that the correct exception occurs with correct eip and error code
(when appropriate).

Given some TUITs I hope to upstream a framework for unit testing in this fashion.

Andrew Cooper (5):
  x86/emulate: Provide further information about software events
  x86/hvm: Don't discard the SW/HW event distinction from the emulator
  x86/emulate: Support for emulating software event injection
  x86/hvm: Forced Emulation Prefix for debug builds of Xen
  x86/svm: Misc cleanup

Jan Beulich (1):
  x86emul: fix SYSCALL/SYSENTER/SYSEXIT emulation

 docs/misc/xen-command-line.markdown    |   11 ++
 xen/arch/x86/hvm/emulate.c             |   50 ++++++--
 xen/arch/x86/hvm/hvm.c                 |    5 +
 xen/arch/x86/hvm/io.c                  |    2 +-
 xen/arch/x86/hvm/svm/svm.c             |   77 ++++++++++--
 xen/arch/x86/hvm/vmx/realmode.c        |   14 +--
 xen/arch/x86/hvm/vmx/vmx.c             |   18 ++-
 xen/arch/x86/mm.c                      |    2 +
 xen/arch/x86/mm/shadow/common.c        |    1 +
 xen/arch/x86/x86_emulate/x86_emulate.c |  203 +++++++++++++++++++++++---------
 xen/arch/x86/x86_emulate/x86_emulate.h |   19 +++
 xen/include/asm-x86/hvm/emulate.h      |    5 +-
 xen/include/asm-x86/hvm/hvm.h          |    5 +
 13 files changed, 326 insertions(+), 86 deletions(-)

-- 
1.7.10.4

[PATCH 1/6] x86emul: fix SYSCALL/SYSENTER/SYSEXIT emulation

[Andrew Cooper]

From: Jan Beulich <jbeulich@suse.com>

SYSCALL:
- make sure SS selector has RPL 0
- only use 32 bits of RIP to fill RCX when target execution mode is 32-bit
- don't shadow function wide variable 'rc'
- consolidate CS attribute setting into single statements
- drop pointless initializers and casts
- drop redundant MSR_STAR read (as suggested by Andrew Cooper)

SYSENTER/SYSEXIT:
- #GP condition doesn't depend on guest mode
- only use 32 bits for setting RIP/RSP when target execution mode is 32-bit
- don't shadow function wide variable 'rc'
- consolidate CS attribute setting into single statements
- drop pointless (and inconsistently used) casts

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/x86_emulate/x86_emulate.c |   77 ++++++++++----------------------
 1 file changed, 24 insertions(+), 53 deletions(-)

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 5fbe024..1e79d0f 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -3769,8 +3769,7 @@ x86_emulate(
 
     case 0x05: /* syscall */ {
         uint64_t msr_content;
-        struct segment_register cs = { 0 }, ss = { 0 };
-        int rc;
+        struct segment_register cs, ss;
 
         generate_exception_if(in_realmode(ctxt, ops), EXC_UD, -1);
         generate_exception_if(!in_protmode(ctxt, ops), EXC_UD, -1);
@@ -3784,13 +3783,11 @@ x86_emulate(
         if ( (rc = ops->read_msr(MSR_STAR, &msr_content, ctxt)) != 0 )
             goto done;
 
-        msr_content >>= 32;
-        cs.sel = (uint16_t)(msr_content & 0xfffc);
-        ss.sel = (uint16_t)(msr_content + 8);
+        cs.sel = (msr_content >> 32) & ~3; /* SELECTOR_RPL_MASK */
+        ss.sel = cs.sel + 8;
 
         cs.base = ss.base = 0; /* flat segment */
         cs.limit = ss.limit = ~0u;  /* 4GB limit */
-        cs.attr.bytes = 0xc9b; /* G+DB+P+S+Code */
         ss.attr.bytes = 0xc93; /* G+DB+P+S+Data */
 
 #ifdef __x86_64__
@@ -3799,8 +3796,7 @@ x86_emulate(
             goto cannot_emulate;
         if ( rc )
         {
-            cs.attr.fields.db = 0;
-            cs.attr.fields.l = 1;
+            cs.attr.bytes = 0xa9b; /* L+DB+P+S+Code */
 
             _regs.rcx = _regs.rip;
             _regs.r11 = _regs.eflags & ~EFLG_RF;
@@ -3817,10 +3813,9 @@ x86_emulate(
         else
 #endif
         {
-            if ( (rc = ops->read_msr(MSR_STAR, &msr_content, ctxt)) != 0 )
-                goto done;
+            cs.attr.bytes = 0xc9b; /* G+DB+P+S+Code */
 
-            _regs.ecx = _regs.eip;
+            _regs.ecx = (uint32_t)_regs.eip;
             _regs.eip = (uint32_t)msr_content;
             _regs.eflags &= ~(EFLG_VM | EFLG_IF | EFLG_RF);
         }
@@ -4012,7 +4007,7 @@ x86_emulate(
     case 0x34: /* sysenter */ {
         uint64_t msr_content;
         struct segment_register cs, ss;
-        int rc;
+        int lm;
 
         generate_exception_if(mode_ring0(), EXC_GP, 0);
         generate_exception_if(in_realmode(ctxt, ops), EXC_GP, 0);
@@ -4022,34 +4017,26 @@ x86_emulate(
         if ( (rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt)) != 0 )
             goto done;
 
-        if ( mode_64bit() )
-            generate_exception_if(msr_content == 0, EXC_GP, 0);
-        else
-            generate_exception_if((msr_content & 0xfffc) == 0, EXC_GP, 0);
+        generate_exception_if(!(msr_content & 0xfffc), EXC_GP, 0);
+        lm = in_longmode(ctxt, ops);
+        if ( lm < 0 )
+            goto cannot_emulate;
 
         _regs.eflags &= ~(EFLG_VM | EFLG_IF | EFLG_RF);
 
         fail_if(ops->read_segment == NULL);
         ops->read_segment(x86_seg_cs, &cs, ctxt);
-        cs.sel = (uint16_t)msr_content & ~3; /* SELECTOR_RPL_MASK */
+        cs.sel = msr_content & ~3; /* SELECTOR_RPL_MASK */
         cs.base = 0;   /* flat segment */
         cs.limit = ~0u;  /* 4GB limit */
-        cs.attr.bytes = 0xc9b; /* G+DB+P+S+Code */
+        cs.attr.bytes = lm ? 0xa9b  /* L+DB+P+S+Code */
+                           : 0xc9b; /* G+DB+P+S+Code */
 
         ss.sel = cs.sel + 8;
         ss.base = 0;   /* flat segment */
         ss.limit = ~0u;  /* 4GB limit */
         ss.attr.bytes = 0xc93; /* G+DB+P+S+Data */
 
-        rc = in_longmode(ctxt, ops);
-        if ( rc < 0 )
-            goto cannot_emulate;
-        if ( rc )
-        {
-            cs.attr.fields.db = 0;
-            cs.attr.fields.l = 1;
-        }
-
         fail_if(ops->write_segment == NULL);
         if ( (rc = ops->write_segment(x86_seg_cs, &cs, ctxt)) != 0 ||
              (rc = ops->write_segment(x86_seg_ss, &ss, ctxt)) != 0 )
@@ -4057,11 +4044,11 @@ x86_emulate(
 
         if ( (rc = ops->read_msr(MSR_SYSENTER_EIP, &msr_content, ctxt)) != 0 )
             goto done;
-        _regs.eip = msr_content;
+        _regs.eip = lm ? msr_content : (uint32_t)msr_content;
 
         if ( (rc = ops->read_msr(MSR_SYSENTER_ESP, &msr_content, ctxt)) != 0 )
             goto done;
-        _regs.esp = msr_content;
+        _regs.esp = lm ? msr_content : (uint32_t)msr_content;
 
         break;
     }
@@ -4070,7 +4057,6 @@ x86_emulate(
         uint64_t msr_content;
         struct segment_register cs, ss;
         bool_t user64 = !!(rex_prefix & REX_W);
-        int rc;
 
         generate_exception_if(!mode_ring0(), EXC_GP, 0);
         generate_exception_if(in_realmode(ctxt, ops), EXC_GP, 0);
@@ -4080,42 +4066,27 @@ x86_emulate(
         if ( (rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt)) != 0 )
             goto done;
 
-        if ( user64 )
-        {
-            cs.sel = (uint16_t)(msr_content + 32);
-            ss.sel = (cs.sel + 8);
-            generate_exception_if(msr_content == 0, EXC_GP, 0);
-        }
-        else
-        {
-            cs.sel = (uint16_t)(msr_content + 16);
-            ss.sel = (uint16_t)(msr_content + 24);
-            generate_exception_if((msr_content & 0xfffc) == 0, EXC_GP, 0);
-        }
+        generate_exception_if(!(msr_content & 0xfffc), EXC_GP, 0);
 
-        cs.sel |= 0x3;   /* SELECTOR_RPL_MASK */
+        cs.sel = (msr_content | 3) + /* SELECTOR_RPL_MASK */
+                 (user64 ? 32 : 16);
         cs.base = 0;   /* flat segment */
         cs.limit = ~0u;  /* 4GB limit */
-        cs.attr.bytes = 0xcfb; /* G+DB+P+DPL3+S+Code */
+        cs.attr.bytes = user64 ? 0xafb  /* L+DB+P+DPL3+S+Code */
+                               : 0xcfb; /* G+DB+P+DPL3+S+Code */
 
-        ss.sel |= 0x3;   /* SELECTOR_RPL_MASK */
+        ss.sel = cs.sel + 8;
         ss.base = 0;   /* flat segment */
         ss.limit = ~0u;  /* 4GB limit */
         ss.attr.bytes = 0xcf3; /* G+DB+P+DPL3+S+Data */
 
-        if ( user64 )
-        {
-            cs.attr.fields.db = 0;
-            cs.attr.fields.l = 1;
-        }
-
         fail_if(ops->write_segment == NULL);
         if ( (rc = ops->write_segment(x86_seg_cs, &cs, ctxt)) != 0 ||
              (rc = ops->write_segment(x86_seg_ss, &ss, ctxt)) != 0 )
             goto done;
 
-        _regs.eip = _regs.edx;
-        _regs.esp = _regs.ecx;
+        _regs.eip = user64 ? _regs.edx : (uint32_t)_regs.edx;
+        _regs.esp = user64 ? _regs.ecx : (uint32_t)_regs.ecx;
         break;
     }
 
-- 
1.7.10.4

[PATCH 2/6] x86/emulate: Provide further information about software events

[Andrew Cooper]

This is needed by subsequent patches to support correctly injecting sofware
events for HVM Guests.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/hvm/emulate.c             |    1 +
 xen/arch/x86/x86_emulate/x86_emulate.c |    8 +++++++-
 xen/arch/x86/x86_emulate/x86_emulate.h |    9 +++++++++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 6ab06e0..5d5d765 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1119,6 +1119,7 @@ static int hvmemul_inject_hw_exception(
 }
 
 static int hvmemul_inject_sw_interrupt(
+    enum x86_swint_type type,
     uint8_t vector,
     uint8_t insn_len,
     struct x86_emulate_ctxt *ctxt)
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 1e79d0f..e06aa60 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1333,6 +1333,7 @@ x86_emulate(
     bool_t lock_prefix = 0;
     int override_seg = -1, rc = X86EMUL_OKAY;
     struct operand src, dst;
+    enum x86_swint_type swint_type;
     DECLARE_ALIGNED(mmval_t, mmval);
     /*
      * Data operand effective address (usually computed from ModRM).
@@ -2629,14 +2630,17 @@ x86_emulate(
 
     case 0xcc: /* int3 */
         src.val = EXC_BP;
+        swint_type = x86_swint_int3;
         goto swint;
 
     case 0xcd: /* int imm8 */
         src.val = insn_fetch_type(uint8_t);
+        swint_type = x86_swint_int;
     swint:
         fail_if(!in_realmode(ctxt, ops)); /* XSA-106 */
         fail_if(ops->inject_sw_interrupt == NULL);
-        rc = ops->inject_sw_interrupt(src.val, _regs.eip - ctxt->regs->eip,
+        rc = ops->inject_sw_interrupt(swint_type, src.val,
+                                      _regs.eip - ctxt->regs->eip,
                                       ctxt) ? : X86EMUL_EXCEPTION;
         goto done;
 
@@ -2645,6 +2649,7 @@ x86_emulate(
         if ( !(_regs.eflags & EFLG_OF) )
             break;
         src.val = EXC_OF;
+        swint_type = x86_swint_into;
         goto swint;
 
     case 0xcf: /* iret */ {
@@ -3312,6 +3317,7 @@ x86_emulate(
 
     case 0xf1: /* int1 (icebp) */
         src.val = EXC_DB;
+        swint_type = x86_swint_icebp;
         goto swint;
 
     case 0xf4: /* hlt */
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
index 107addf..b336e17 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -51,6 +51,14 @@ enum x86_segment {
 
 #define is_x86_user_segment(seg) ((unsigned)(seg) <= x86_seg_gs)
 
+/* Classification of the types of software generated interrupts/exceptions. */
+enum x86_swint_type {
+    x86_swint_icebp, /* 0xf1 */
+    x86_swint_int3,  /* 0xcc */
+    x86_swint_into,  /* 0xce */
+    x86_swint_int,   /* 0xcd $n */
+};
+
 /* 
  * Attribute for segment selector. This is a copy of bit 40:47 & 52:55 of the
  * segment descriptor. It happens to match the format of an AMD SVM VMCB.
@@ -337,6 +345,7 @@ struct x86_emulate_ops
 
     /* inject_sw_interrupt */
     int (*inject_sw_interrupt)(
+        enum x86_swint_type type,
         uint8_t vector,
         uint8_t insn_len,
         struct x86_emulate_ctxt *ctxt);
-- 
1.7.10.4

[PATCH 3/6] x86/hvm: Don't discard the SW/HW event distinction from the emulator

[Andrew Cooper]

Injecting emulator software events as hardware exceptions results in a bypass
of DPL checks.  As the emulator doesn't perform DPL checks itself, guest
userspace is capable of bypassing DPL checks and injecting arbitrary events.

Propagating software event information from the emulator allows VMX to now
properly inject software events, including DPL and presence checks, as well
correct fault/trap frames.

Reported-by: Andrei LUTAS <vlutas@bitdefender.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Tested-by: Andrei LUTAS <vlutas@bitdefender.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Eddie Dong <eddie.dong@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/emulate.c        |   41 ++++++++++++++++++++++++++++---------
 xen/arch/x86/hvm/io.c             |    2 +-
 xen/arch/x86/hvm/svm/svm.c        |    2 +-
 xen/arch/x86/hvm/vmx/realmode.c   |   14 ++++++-------
 xen/arch/x86/hvm/vmx/vmx.c        |    2 +-
 xen/include/asm-x86/hvm/emulate.h |    5 ++---
 6 files changed, 43 insertions(+), 23 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 5d5d765..7ee146b 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -441,9 +441,10 @@ static int hvmemul_virtual_to_linear(
 
     /* This is a singleton operation: fail it with an exception. */
     hvmemul_ctxt->exn_pending = 1;
-    hvmemul_ctxt->exn_vector = TRAP_gp_fault;
-    hvmemul_ctxt->exn_error_code = 0;
-    hvmemul_ctxt->exn_insn_len = 0;
+    hvmemul_ctxt->trap.vector = TRAP_gp_fault;
+    hvmemul_ctxt->trap.type = X86_EVENTTYPE_HW_EXCEPTION;
+    hvmemul_ctxt->trap.error_code = 0;
+    hvmemul_ctxt->trap.insn_len = 0;
     return X86EMUL_EXCEPTION;
 }
 
@@ -1111,9 +1112,10 @@ static int hvmemul_inject_hw_exception(
         container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
 
     hvmemul_ctxt->exn_pending = 1;
-    hvmemul_ctxt->exn_vector = vector;
-    hvmemul_ctxt->exn_error_code = error_code;
-    hvmemul_ctxt->exn_insn_len = 0;
+    hvmemul_ctxt->trap.vector = vector;
+    hvmemul_ctxt->trap.type = X86_EVENTTYPE_HW_EXCEPTION;
+    hvmemul_ctxt->trap.error_code = error_code;
+    hvmemul_ctxt->trap.insn_len = 0;
 
     return X86EMUL_OKAY;
 }
@@ -1127,10 +1129,29 @@ static int hvmemul_inject_sw_interrupt(
     struct hvm_emulate_ctxt *hvmemul_ctxt =
         container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
 
+    switch ( type )
+    {
+    case x86_swint_icebp:
+        hvmemul_ctxt->trap.type = X86_EVENTTYPE_PRI_SW_EXCEPTION;
+        break;
+
+    case x86_swint_int3:
+    case x86_swint_into:
+        hvmemul_ctxt->trap.type = X86_EVENTTYPE_SW_EXCEPTION;
+        break;
+
+    case x86_swint_int:
+        hvmemul_ctxt->trap.type = X86_EVENTTYPE_SW_INTERRUPT;
+        break;
+
+    default:
+        return X86EMUL_UNHANDLEABLE;
+    }
+
     hvmemul_ctxt->exn_pending = 1;
-    hvmemul_ctxt->exn_vector = vector;
-    hvmemul_ctxt->exn_error_code = -1;
-    hvmemul_ctxt->exn_insn_len = insn_len;
+    hvmemul_ctxt->trap.vector = vector;
+    hvmemul_ctxt->trap.error_code = HVM_DELIVER_NO_ERROR_CODE;
+    hvmemul_ctxt->trap.insn_len = insn_len;
 
     return X86EMUL_OKAY;
 }
@@ -1404,7 +1425,7 @@ void hvm_mem_event_emulate_one(bool_t nowrite, unsigned int trapnr,
         break;
     case X86EMUL_EXCEPTION:
         if ( ctx.exn_pending )
-            hvm_inject_hw_exception(ctx.exn_vector, ctx.exn_error_code);
+            hvm_inject_trap(&ctx.trap);
         break;
     }
 
diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
index 9f565d6..e5d5e79 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -113,7 +113,7 @@ int handle_mmio(void)
         return 0;
     case X86EMUL_EXCEPTION:
         if ( ctxt.exn_pending )
-            hvm_inject_hw_exception(ctxt.exn_vector, ctxt.exn_error_code);
+            hvm_inject_trap(&ctxt.trap);
         break;
     default:
         break;
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 5d404ce..de982fd 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2080,7 +2080,7 @@ static void svm_vmexit_ud_intercept(struct cpu_user_regs *regs)
         break;
     case X86EMUL_EXCEPTION:
         if ( ctxt.exn_pending )
-            hvm_inject_hw_exception(ctxt.exn_vector, ctxt.exn_error_code);
+            hvm_inject_trap(&ctxt.trap);
         /* fall through */
     default:
         hvm_emulate_writeback(&ctxt);
diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c
index 45066b2..9a6de6c 100644
--- a/xen/arch/x86/hvm/vmx/realmode.c
+++ b/xen/arch/x86/hvm/vmx/realmode.c
@@ -129,27 +129,27 @@ static void realmode_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt)
                 gdprintk(XENLOG_ERR, "Exception pending but no info.\n");
                 goto fail;
             }
-            hvmemul_ctxt->exn_vector = (uint8_t)intr_info;
-            hvmemul_ctxt->exn_insn_len = 0;
+            hvmemul_ctxt->trap.vector = (uint8_t)intr_info;
+            hvmemul_ctxt->trap.insn_len = 0;
         }
 
         if ( unlikely(curr->domain->debugger_attached) &&
-             ((hvmemul_ctxt->exn_vector == TRAP_debug) ||
-              (hvmemul_ctxt->exn_vector == TRAP_int3)) )
+             ((hvmemul_ctxt->trap.vector == TRAP_debug) ||
+              (hvmemul_ctxt->trap.vector == TRAP_int3)) )
         {
             domain_pause_for_debugger();
         }
         else if ( curr->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE )
         {
             gdprintk(XENLOG_ERR, "Exception %02x in protected mode.\n",
-                     hvmemul_ctxt->exn_vector);
+                     hvmemul_ctxt->trap.vector);
             goto fail;
         }
         else
         {
             realmode_deliver_exception(
-                hvmemul_ctxt->exn_vector,
-                hvmemul_ctxt->exn_insn_len,
+                hvmemul_ctxt->trap.vector,
+                hvmemul_ctxt->trap.insn_len,
                 hvmemul_ctxt);
         }
     }
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 84119ed..addaa81 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2510,7 +2510,7 @@ static void vmx_vmexit_ud_intercept(struct cpu_user_regs *regs)
         break;
     case X86EMUL_EXCEPTION:
         if ( ctxt.exn_pending )
-            hvm_inject_hw_exception(ctxt.exn_vector, ctxt.exn_error_code);
+            hvm_inject_trap(&ctxt.trap);
         /* fall through */
     default:
         hvm_emulate_writeback(&ctxt);
diff --git a/xen/include/asm-x86/hvm/emulate.h b/xen/include/asm-x86/hvm/emulate.h
index efff97e..6cdc57b 100644
--- a/xen/include/asm-x86/hvm/emulate.h
+++ b/xen/include/asm-x86/hvm/emulate.h
@@ -13,6 +13,7 @@
 #define __ASM_X86_HVM_EMULATE_H__
 
 #include <xen/config.h>
+#include <asm/hvm/hvm.h>
 #include <asm/x86_emulate.h>
 
 struct hvm_emulate_ctxt {
@@ -28,9 +29,7 @@ struct hvm_emulate_ctxt {
     unsigned long seg_reg_dirty;
 
     bool_t exn_pending;
-    uint8_t exn_vector;
-    uint8_t exn_insn_len;
-    int32_t exn_error_code;
+    struct hvm_trap trap;
 
     uint32_t intr_shadow;
 };
-- 
1.7.10.4

[PATCH 4/6] x86/emulate: Support for emulating software event injection

[Andrew Cooper]

AMD SVM requires all software events to have their injection emulated if
hardware lacks NextRIP support.  In addition, `icebp` (opcode 0xf1) injection
requires emulation in all cases, even with hardware NextRIP support.

Emulating full control transfers is overkill for our needs.  All that matters
is that guest userspace can't bypass the descriptor DPL check.  Any guest OS
which would incur other faults as part of injection is going to end up with a
double fault instead, and won't be in a position to care that the faulting eip
is wrong.

Reported-by: Andrei LUTAS <vlutas@bitdefender.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
---
 xen/arch/x86/hvm/emulate.c             |    8 +++
 xen/arch/x86/hvm/svm/svm.c             |   57 +++++++++++++--
 xen/arch/x86/mm.c                      |    2 +
 xen/arch/x86/mm/shadow/common.c        |    1 +
 xen/arch/x86/x86_emulate/x86_emulate.c |  122 ++++++++++++++++++++++++++++++--
 xen/arch/x86/x86_emulate/x86_emulate.h |   10 +++
 6 files changed, 191 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 7ee146b..463ccfb 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -21,6 +21,7 @@
 #include <asm/hvm/hvm.h>
 #include <asm/hvm/trace.h>
 #include <asm/hvm/support.h>
+#include <asm/hvm/svm/svm.h>
 
 static void hvmtrace_io_assist(int is_mmio, ioreq_t *p)
 {
@@ -1328,6 +1329,13 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
     vio->mmio_retrying = vio->mmio_retry;
     vio->mmio_retry = 0;
 
+    if ( cpu_has_vmx )
+        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_none;
+    else if ( cpu_has_svm_nrips )
+        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_icebp;
+    else
+        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_all;
+
     rc = x86_emulate(&hvmemul_ctxt->ctxt, ops);
 
     if ( rc == X86EMUL_OKAY && vio->mmio_retry )
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index de982fd..b6beefc 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1177,11 +1177,12 @@ static void svm_inject_trap(struct hvm_trap *trap)
     struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
     eventinj_t event = vmcb->eventinj;
     struct hvm_trap _trap = *trap;
+    const struct cpu_user_regs *regs = guest_cpu_user_regs();
 
     switch ( _trap.vector )
     {
     case TRAP_debug:
-        if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
+        if ( regs->eflags & X86_EFLAGS_TF )
         {
             __restore_debug_registers(vmcb, curr);
             vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | 0x4000);
@@ -1209,10 +1210,58 @@ static void svm_inject_trap(struct hvm_trap *trap)
 
     event.bytes = 0;
     event.fields.v = 1;
-    event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
     event.fields.vector = _trap.vector;
-    event.fields.ev = (_trap.error_code != HVM_DELIVER_NO_ERROR_CODE);
-    event.fields.errorcode = _trap.error_code;
+
+    /* Refer to AMD Vol 2: System Programming, 15.20 Event Injection. */
+    switch ( _trap.type )
+    {
+    case X86_EVENTTYPE_SW_INTERRUPT: /* int $n */
+        /*
+         * Injection type 4 (software interrupt) is only supported with
+         * NextRIP support.  Without NextRIP, the emulator will have performed
+         * DPL and presence checks for us.
+         */
+        if ( cpu_has_svm_nrips )
+        {
+            vmcb->nextrip = regs->eip + _trap.insn_len;
+            event.fields.type = X86_EVENTTYPE_SW_INTERRUPT;
+        }
+        else
+            event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
+        break;
+
+    case X86_EVENTTYPE_PRI_SW_EXCEPTION: /* icebp */
+        /*
+         * icebp's injection must always be emulated.  Software injection help
+         * in x86_emulate has moved eip forward, but NextRIP (if used) still
+         * needs setting or execution will resume from 0.
+         */
+        if ( cpu_has_svm_nrips )
+            vmcb->nextrip = regs->eip;
+        event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
+        break;
+
+    case X86_EVENTTYPE_SW_EXCEPTION: /* int3, into */
+        /*
+         * The AMD manual states that .type=3 (HW exception), .vector=3 or 4,
+         * will perform DPL checks.  Experimentally, DPL and presence checks
+         * are indeed performed, even without NextRIP support.
+         *
+         * However without NextRIP support, the event injection still needs
+         * fully emulating to get the correct eip in the trap frame, yet get
+         * the correct faulting eip should a fault occur.
+         */
+        if ( cpu_has_svm_nrips )
+            vmcb->nextrip = regs->eip + _trap.insn_len;
+        event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
+        break;
+
+    default:
+        event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
+        event.fields.ev = (_trap.error_code != HVM_DELIVER_NO_ERROR_CODE);
+        event.fields.errorcode = _trap.error_code;
+        break;
+    }
 
     vmcb->eventinj = event;
 
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 5b3f06f..bfe9f05 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5096,6 +5096,7 @@ int ptwr_do_page_fault(struct vcpu *v, unsigned long addr,
     ptwr_ctxt.ctxt.force_writeback = 0;
     ptwr_ctxt.ctxt.addr_size = ptwr_ctxt.ctxt.sp_size =
         is_pv_32on64_domain(d) ? 32 : BITS_PER_LONG;
+    ptwr_ctxt.ctxt.swint_emulate = x86_swint_emulate_none;
     ptwr_ctxt.cr2 = addr;
     ptwr_ctxt.pte = pte;
 
@@ -5172,6 +5173,7 @@ int mmio_ro_do_page_fault(struct vcpu *v, unsigned long addr,
         .ctxt.regs = regs,
         .ctxt.addr_size = addr_size,
         .ctxt.sp_size = addr_size,
+        .ctxt.swint_emulate = x86_swint_emulate_none,
         .cr2 = addr
     };
     int rc;
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 9115a78..a5eed28 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -366,6 +366,7 @@ const struct x86_emulate_ops *shadow_init_emulation(
 
     sh_ctxt->ctxt.regs = regs;
     sh_ctxt->ctxt.force_writeback = 0;
+    sh_ctxt->ctxt.swint_emulate = x86_swint_emulate_none;
 
     if ( is_pv_vcpu(v) )
     {
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index e06aa60..ffca65a 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -403,6 +403,11 @@ typedef union {
 #define EXC_PF 14
 #define EXC_MF 16
 
+/* Segment selector error code bits. */
+#define ECODE_EXT (1 << 0)
+#define ECODE_IDT (1 << 1)
+#define ECODE_TI  (1 << 2)
+
 /*
  * Instruction emulation:
  * Most instructions are emulated directly via a fragment of inline assembly
@@ -1318,6 +1323,115 @@ decode_segment(uint8_t modrm_reg)
     return decode_segment_failed;
 }
 
+/* Inject a software interrupt/exception, emulating if needed. */
+static int inject_swint(enum x86_swint_type type,
+                        uint8_t vector, uint8_t insn_len,
+                        struct x86_emulate_ctxt *ctxt,
+                        const struct x86_emulate_ops *ops)
+{
+    int rc, error_code, fault_type = EXC_GP;
+
+    fail_if(ops->inject_sw_interrupt == NULL);
+    fail_if(ops->inject_hw_exception == NULL);
+
+    /*
+     * Without hardware support, injecting software interrupts/exceptions is
+     * problematic.
+     *
+     * All software methods of generating exceptions (other than BOUND) yield
+     * traps, so eip in the exception frame needs to point after the
+     * instruction, not at it.
+     *
+     * However, if injecting it as a hardware exception causes a fault during
+     * delivery, our adjustment of eip will cause the fault to be reported
+     * after the faulting instruction, not pointing to it.
+     *
+     * Therefore, eip can only safely be wound forwards if we are certain that
+     * injecting an equivalent hardware exception won't fault, which means
+     * emulating everything the processor would do on a control transfer.
+     *
+     * However, emulation of complete control transfers is very complicated.
+     * All we care about is that guest userspace cannot avoid the descriptor
+     * DPL check by using the Xen emulator, and successfully invoke DPL=0
+     * descriptors.
+     *
+     * Any OS which would further fault during injection is going to receive a
+     * double fault anyway, and won't be in a position to care that the
+     * faulting eip is incorrect.
+     */
+
+    if ( (ctxt->swint_emulate == x86_swint_emulate_all) ||
+         ((ctxt->swint_emulate == x86_swint_emulate_icebp) &&
+          (type == x86_swint_icebp)) )
+    {
+        if ( !in_realmode(ctxt, ops) )
+        {
+            unsigned int idte_size = (ctxt->addr_size == 64) ? 16 : 8;
+            unsigned int idte_offset = vector * idte_size;
+            struct segment_register idtr;
+            uint32_t idte_ctl;
+
+            /* icebp sets the External Event bit despite being an instruction. */
+            error_code = (vector << 3) | ECODE_IDT |
+                (type == x86_swint_icebp ? ECODE_EXT : 0);
+
+            /*
+             * TODO - this does not cover the v8086 mode with CR4.VME case
+             * correctly, but falls on the safe side from the point of view of
+             * a 32bit OS.  Someone with many TUITs can see about reading the
+             * TSS Software Interrupt Redirection bitmap.
+             */
+            if ( (ctxt->regs->eflags & EFLG_VM) &&
+                 ((ctxt->regs->eflags & EFLG_IOPL) != EFLG_IOPL) )
+                goto raise_exn;
+
+            fail_if(ops->read_segment == NULL);
+            fail_if(ops->read == NULL);
+            if ( (rc = ops->read_segment(x86_seg_idtr, &idtr, ctxt)) )
+                goto done;
+
+            if ( (idte_offset + idte_size - 1) > idtr.limit )
+                goto raise_exn;
+
+            /*
+             * Should strictly speaking read all 8/16 bytes of an entry,
+             * but we currently only care about the dpl and present bits.
+             */
+            ops->read(x86_seg_none, idtr.base + idte_offset + 4,
+                      &idte_ctl, sizeof(idte_ctl), ctxt);
+
+            /* Is this entry present? */
+            if ( !(idte_ctl & (1u << 15)) )
+            {
+                fault_type = EXC_NP;
+                goto raise_exn;
+            }
+
+            /* icebp counts as a hardware event, and bypasses the dpl check. */
+            if ( type != x86_swint_icebp )
+            {
+                struct segment_register ss;
+
+                if ( (rc = ops->read_segment(x86_seg_ss, &ss, ctxt)) )
+                    goto done;
+
+                if ( ss.attr.fields.dpl > ((idte_ctl >> 13) & 3) )
+                    goto raise_exn;
+            }
+        }
+
+        ctxt->regs->eip += insn_len;
+    }
+
+    rc = ops->inject_sw_interrupt(type, vector, insn_len, ctxt);
+
+ done:
+    return rc;
+
+ raise_exn:
+    return ops->inject_hw_exception(fault_type, error_code, ctxt);
+}
+
 int
 x86_emulate(
     struct x86_emulate_ctxt *ctxt,
@@ -2637,11 +2751,9 @@ x86_emulate(
         src.val = insn_fetch_type(uint8_t);
         swint_type = x86_swint_int;
     swint:
-        fail_if(!in_realmode(ctxt, ops)); /* XSA-106 */
-        fail_if(ops->inject_sw_interrupt == NULL);
-        rc = ops->inject_sw_interrupt(swint_type, src.val,
-                                      _regs.eip - ctxt->regs->eip,
-                                      ctxt) ? : X86EMUL_EXCEPTION;
+        rc = inject_swint(swint_type, src.val,
+                          _regs.eip - ctxt->regs->eip,
+                          ctxt, ops) ? : X86EMUL_EXCEPTION;
         goto done;
 
     case 0xce: /* into */
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
index b336e17..b059341 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -59,6 +59,13 @@ enum x86_swint_type {
     x86_swint_int,   /* 0xcd $n */
 };
 
+/* How much help is required with software event injection? */
+enum x86_swint_emulation {
+    x86_swint_emulate_none, /* Hardware supports all software injection properly */
+    x86_swint_emulate_icebp,/* Help needed with `icebp` (0xf1) */
+    x86_swint_emulate_all,  /* Help needed with all software events */
+};
+
 /* 
  * Attribute for segment selector. This is a copy of bit 40:47 & 52:55 of the
  * segment descriptor. It happens to match the format of an AMD SVM VMCB.
@@ -388,6 +395,9 @@ struct x86_emulate_ctxt
     /* Set this if writes may have side effects. */
     uint8_t force_writeback;
 
+    /* Software event injection support. */
+    enum x86_swint_emulation swint_emulate;
+
     /* Retirement state, set by the emulator (valid only on X86EMUL_OKAY). */
     union {
         struct {
-- 
1.7.10.4

[PATCH 5/6] x86/hvm: Forced Emulation Prefix for debug builds of Xen

[Andrew Cooper]

Analysis of XSAs 105 and 106 show that is possible to force a race condition
which causes any arbitrary instruction to be emulated.

To aid testing, explicitly introduce the Forced Emulation Prefix for debug
builds alone.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Eddie Dong <eddie.dong@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 docs/misc/xen-command-line.markdown |   11 +++++++++++
 xen/arch/x86/hvm/hvm.c              |    5 +++++
 xen/arch/x86/hvm/svm/svm.c          |   16 ++++++++++++++++
 xen/arch/x86/hvm/vmx/vmx.c          |   16 ++++++++++++++++
 xen/include/asm-x86/hvm/hvm.h       |    5 +++++
 5 files changed, 53 insertions(+)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index af93e17..389701a 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -682,6 +682,17 @@ Bit 11 - MSR operation logging
 
 Recognized in debug builds of the hypervisor only.
 
+### hvm\_fep
+> `= <boolean>`
+
+> Default: `false`
+
+Allow use of the Forced Emulation Prefix in HVM guests, to allow emulation of
+arbitrary instructions.
+
+This option is intended for development purposes, and is only available in
+debug builds of the hypervisor.
+
 ### hvm\_port80
 > `= <boolean>`
 
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index bb45593..a9b0f9e 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -86,6 +86,11 @@ unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
 static bool_t __initdata opt_hap_enabled = 1;
 boolean_param("hap", opt_hap_enabled);
 
+#ifndef NDEBUG
+bool_t opt_hvm_fep;
+boolean_param("hvm_fep", opt_hvm_fep);
+#endif
+
 static int cpu_callback(
     struct notifier_block *nfb, unsigned long action, void *hcpu)
 {
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index b6beefc..4823c80 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -17,6 +17,7 @@
  * Place - Suite 330, Boston, MA 02111-1307 USA.
  */
 
+#include <xen/guest_access.h>
 #include <xen/config.h>
 #include <xen/init.h>
 #include <xen/lib.h>
@@ -2118,6 +2119,21 @@ static void svm_vmexit_ud_intercept(struct cpu_user_regs *regs)
     struct hvm_emulate_ctxt ctxt;
     int rc;
 
+#ifndef NDEBUG
+    if ( opt_hvm_fep )
+    {
+        XEN_GUEST_HANDLE(char) guest_rip = { (char*)regs->eip };
+        char sig[5]; /* ud2; .ascii "xen" */
+
+        if ( (copy_from_guest(sig, guest_rip, sizeof(sig)) == 0) &&
+             (memcmp(sig, "\xf\xbxen", sizeof(sig)) == 0) )
+        {
+            regs->eip += sizeof(sig);
+            regs->eflags &= ~X86_EFLAGS_RF;
+        }
+    }
+#endif
+
     hvm_emulate_prepare(&ctxt, regs);
 
     rc = hvm_emulate_one(&ctxt);
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index addaa81..fdd05c3 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -16,6 +16,7 @@
  * Place - Suite 330, Boston, MA 02111-1307 USA.
  */
 
+#include <xen/guest_access.h>
 #include <xen/config.h>
 #include <xen/init.h>
 #include <xen/lib.h>
@@ -2499,6 +2500,21 @@ static void vmx_vmexit_ud_intercept(struct cpu_user_regs *regs)
     struct hvm_emulate_ctxt ctxt;
     int rc;
 
+#ifndef NDEBUG
+    if ( opt_hvm_fep )
+    {
+        XEN_GUEST_HANDLE(char) guest_rip = { (char*)regs->eip };
+        char sig[5]; /* ud2; .ascii "xen" */
+
+        if ( (copy_from_guest(sig, guest_rip, sizeof(sig)) == 0) &&
+             (memcmp(sig, "\xf\xbxen", sizeof(sig)) == 0) )
+        {
+            regs->eip += sizeof(sig);
+            regs->eflags &= ~X86_EFLAGS_RF;
+        }
+    }
+#endif
+
     hvm_emulate_prepare(&ctxt, regs);
 
     rc = hvm_emulate_one(&ctxt);
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 121d053..f9fd663 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -514,6 +514,11 @@ bool_t nhvm_vmcx_hap_enabled(struct vcpu *v);
 /* interrupt */
 enum hvm_intblk nhvm_interrupt_blocked(struct vcpu *v);
 
+#ifndef NDEBUG
+/* Permit use of the Forced Emulation Prefix in HVM guests */
+extern bool_t opt_hvm_fep;
+#endif
+
 #endif /* __ASM_X86_HVM_HVM_H__ */
 
 /*
-- 
1.7.10.4

[PATCH 6/6] x86/svm: Misc cleanup

[Andrew Cooper]

cpu_has_monitor_trap_flag is a VMX control, and has nothing to do with SVM.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
---
 xen/arch/x86/hvm/svm/svm.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 4823c80..73f3108 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1188,8 +1188,6 @@ static void svm_inject_trap(struct hvm_trap *trap)
             __restore_debug_registers(vmcb, curr);
             vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | 0x4000);
         }
-        if ( cpu_has_monitor_trap_flag )
-            break;
         /* fall through */
     case TRAP_int3:
         if ( curr->domain->debugger_attached )
-- 
1.7.10.4

Re: [PATCH 0/6] HVM Emulation and trap injection fixes

[Jan Beulich]

>>> On 23.09.14 at 17:03, <andrew.cooper3@citrix.com> wrote:
> This series covers issues discovered during the analysis of XSAs 105 and 106.
> 
> Patch 1 makes fixes to the SYS{CALL,ENTER,EXIT} emulation
> Patches 2-4 provide support to inject software events from the emulator
> Patch 5 introduces HVM Forced Emulation Prefix support to aid testing
> Patch 6 is misc cleanup to svm_inject_trap()
> 
> As part of developing the software event injection emulation (and reverse
> engineering AMD's behaviour), I developed a unit test.

And given that we didn't hear back from you on this while the
embargo was still in place, we'd very much appreciate a timely
response on the relevant parts of this series to make sure there
aren't further cases to consider.

Jan

Re: [PATCH 5/6] x86/hvm: Forced Emulation Prefix for debug builds of Xen

[Jan Beulich]

>>> On 23.09.14 at 17:03, <andrew.cooper3@citrix.com> wrote:

[re-ordering patch contents for a better response context]

> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -514,6 +514,11 @@ bool_t nhvm_vmcx_hap_enabled(struct vcpu *v);
>  /* interrupt */
>  enum hvm_intblk nhvm_interrupt_blocked(struct vcpu *v);
>  
> +#ifndef NDEBUG
> +/* Permit use of the Forced Emulation Prefix in HVM guests */
> +extern bool_t opt_hvm_fep;

#else
#define opt_hvm_fep 0

> +#endif

allowing this to be the single place of dependency on NDEBUG (in
case someone wants to change the condition to some thing else)
by ...

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -86,6 +86,11 @@ unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
>  static bool_t __initdata opt_hap_enabled = 1;
>  boolean_param("hap", opt_hap_enabled);
>  
> +#ifndef NDEBUG
> +bool_t opt_hvm_fep;
> +boolean_param("hvm_fep", opt_hvm_fep);
> +#endif

... making this

#ifndef opt_hvm_fep

and ...

> +#ifndef NDEBUG
> +    if ( opt_hvm_fep )
> +    {
> +        XEN_GUEST_HANDLE(char) guest_rip = { (char*)regs->eip };
> +        char sig[5]; /* ud2; .ascii "xen" */
> +
> +        if ( (copy_from_guest(sig, guest_rip, sizeof(sig)) == 0) &&
> +             (memcmp(sig, "\xf\xbxen", sizeof(sig)) == 0) )
> +        {
> +            regs->eip += sizeof(sig);
> +            regs->eflags &= ~X86_EFLAGS_RF;
> +        }
> +    }
> +#endif

... dropping the preprocessor conditionals both here and in the
similar VMX code chunk.

Apart from that I'd really like you to consider using
copy_from_user_hvm() or hvm_fetch_from_guest_virt_nofault()
- there's no visible benefit from using the PV-aware
copy_from_guest() here.

Jan

[PATCH v2 5/6] x86/hvm: Forced Emulation Prefix for debug builds of Xen

[Andrew Cooper]

Analysis of XSAs 105 and 106 show that is possible to force a race condition
which causes any arbitrary instruction to be emulated.

To aid testing, explicitly introduce the Forced Emulation Prefix for debug
builds alone.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Eddie Dong <eddie.dong@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>

---
v2: (all suggested by Jan)
 * Use hvm_fetch_from_guest_virt_nofault() in preference to copy_from_guest()
 * Vastly reduce use of #ifndef NDEBUG
---
 docs/misc/xen-command-line.markdown |   11 +++++++++++
 xen/arch/x86/hvm/hvm.c              |    5 +++++
 xen/arch/x86/hvm/svm/svm.c          |   13 +++++++++++++
 xen/arch/x86/hvm/vmx/vmx.c          |   13 +++++++++++++
 xen/include/asm-x86/hvm/hvm.h       |    7 +++++++
 5 files changed, 49 insertions(+)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index af93e17..389701a 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -682,6 +682,17 @@ Bit 11 - MSR operation logging
 
 Recognized in debug builds of the hypervisor only.
 
+### hvm\_fep
+> `= <boolean>`
+
+> Default: `false`
+
+Allow use of the Forced Emulation Prefix in HVM guests, to allow emulation of
+arbitrary instructions.
+
+This option is intended for development purposes, and is only available in
+debug builds of the hypervisor.
+
 ### hvm\_port80
 > `= <boolean>`
 
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 5c7e0a4..34f28d0 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -86,6 +86,11 @@ unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
 static bool_t __initdata opt_hap_enabled = 1;
 boolean_param("hap", opt_hap_enabled);
 
+#ifndef opt_hvm_fep
+bool_t opt_hvm_fep;
+boolean_param("hvm_fep", opt_hvm_fep);
+#endif
+
 static int cpu_callback(
     struct notifier_block *nfb, unsigned long action, void *hcpu)
 {
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index b6beefc..cda968b 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2118,6 +2118,19 @@ static void svm_vmexit_ud_intercept(struct cpu_user_regs *regs)
     struct hvm_emulate_ctxt ctxt;
     int rc;
 
+    if ( opt_hvm_fep )
+    {
+        char sig[5]; /* ud2; .ascii "xen" */
+
+        if ( (hvm_fetch_from_guest_virt_nofault(
+                  sig, regs->eip, sizeof(sig), 0) == HVMCOPY_okay) &&
+             (memcmp(sig, "\xf\xbxen", sizeof(sig)) == 0) )
+        {
+            regs->eip += sizeof(sig);
+            regs->eflags &= ~X86_EFLAGS_RF;
+        }
+    }
+
     hvm_emulate_prepare(&ctxt, regs);
 
     rc = hvm_emulate_one(&ctxt);
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index addaa81..7f02ba2 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2499,6 +2499,19 @@ static void vmx_vmexit_ud_intercept(struct cpu_user_regs *regs)
     struct hvm_emulate_ctxt ctxt;
     int rc;
 
+    if ( opt_hvm_fep )
+    {
+        char sig[5]; /* ud2; .ascii "xen" */
+
+        if ( (hvm_fetch_from_guest_virt_nofault(
+                  sig, regs->eip, sizeof(sig), 0) == HVMCOPY_okay) &&
+             (memcmp(sig, "\xf\xbxen", sizeof(sig)) == 0) )
+        {
+            regs->eip += sizeof(sig);
+            regs->eflags &= ~X86_EFLAGS_RF;
+        }
+    }
+
     hvm_emulate_prepare(&ctxt, regs);
 
     rc = hvm_emulate_one(&ctxt);
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 3e66276..c0fbc8b 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -517,6 +517,13 @@ bool_t nhvm_vmcx_hap_enabled(struct vcpu *v);
 /* interrupt */
 enum hvm_intblk nhvm_interrupt_blocked(struct vcpu *v);
 
+#ifndef NDEBUG
+/* Permit use of the Forced Emulation Prefix in HVM guests */
+extern bool_t opt_hvm_fep;
+#else
+#define opt_hvm_fep 0
+#endif
+
 #endif /* __ASM_X86_HVM_HVM_H__ */
 
 /*
-- 
1.7.10.4

Re: [PATCH v2 5/6] x86/hvm: Forced Emulation Prefix for debug builds of Xen

[Jan Beulich]

>>> On 23.09.14 at 18:09, <andrew.cooper3@citrix.com> wrote:
> Analysis of XSAs 105 and 106 show that is possible to force a race condition
> which causes any arbitrary instruction to be emulated.
> 
> To aid testing, explicitly introduce the Forced Emulation Prefix for debug
> builds alone.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v2 5/6] x86/hvm: Forced Emulation Prefix for debug builds of Xen

[Boris Ostrovsky]

On 09/23/2014 12:09 PM, Andrew Cooper wrote:
> Analysis of XSAs 105 and 106 show that is possible to force a race condition
> which causes any arbitrary instruction to be emulated.
>
> To aid testing, explicitly introduce the Forced Emulation Prefix for debug
> builds alone.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Eddie Dong <eddie.dong@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
>
> ---
> v2: (all suggested by Jan)
>   * Use hvm_fetch_from_guest_virt_nofault() in preference to copy_from_guest()
>   * Vastly reduce use of #ifndef NDEBUG
> ---
>   docs/misc/xen-command-line.markdown |   11 +++++++++++
>   xen/arch/x86/hvm/hvm.c              |    5 +++++
>   xen/arch/x86/hvm/svm/svm.c          |   13 +++++++++++++
>   xen/arch/x86/hvm/vmx/vmx.c          |   13 +++++++++++++
>   xen/include/asm-x86/hvm/hvm.h       |    7 +++++++
>   5 files changed, 49 insertions(+)
>
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index af93e17..389701a 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -682,6 +682,17 @@ Bit 11 - MSR operation logging
>   
>   Recognized in debug builds of the hypervisor only.
>   
> +### hvm\_fep
> +> `= <boolean>`
> +
> +> Default: `false`
> +
> +Allow use of the Forced Emulation Prefix in HVM guests, to allow emulation of
> +arbitrary instructions.
> +
> +This option is intended for development purposes, and is only available in
> +debug builds of the hypervisor.
> +
>   ### hvm\_port80
>   > `= <boolean>`
>   
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 5c7e0a4..34f28d0 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -86,6 +86,11 @@ unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
>   static bool_t __initdata opt_hap_enabled = 1;
>   boolean_param("hap", opt_hap_enabled);
>   
> +#ifndef opt_hvm_fep
> +bool_t opt_hvm_fep;
> +boolean_param("hvm_fep", opt_hvm_fep);
> +#endif
> +
>   static int cpu_callback(
>       struct notifier_block *nfb, unsigned long action, void *hcpu)
>   {
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index b6beefc..cda968b 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2118,6 +2118,19 @@ static void svm_vmexit_ud_intercept(struct cpu_user_regs *regs)
>       struct hvm_emulate_ctxt ctxt;
>       int rc;
>   
> +    if ( opt_hvm_fep )
> +    {
> +        char sig[5]; /* ud2; .ascii "xen" */
> +
> +        if ( (hvm_fetch_from_guest_virt_nofault(
> +                  sig, regs->eip, sizeof(sig), 0) == HVMCOPY_okay) &&
> +             (memcmp(sig, "\xf\xbxen", sizeof(sig)) == 0) )
> +        {
> +            regs->eip += sizeof(sig);
> +            regs->eflags &= ~X86_EFLAGS_RF;
> +        }
> +    }

This code is exactly the same for SVM and VMX. Can it be factored out?

-boris

> +
>       hvm_emulate_prepare(&ctxt, regs);
>   
>       rc = hvm_emulate_one(&ctxt);
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index addaa81..7f02ba2 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2499,6 +2499,19 @@ static void vmx_vmexit_ud_intercept(struct cpu_user_regs *regs)
>       struct hvm_emulate_ctxt ctxt;
>       int rc;
>   
> +    if ( opt_hvm_fep )
> +    {
> +        char sig[5]; /* ud2; .ascii "xen" */
> +
> +        if ( (hvm_fetch_from_guest_virt_nofault(
> +                  sig, regs->eip, sizeof(sig), 0) == HVMCOPY_okay) &&
> +             (memcmp(sig, "\xf\xbxen", sizeof(sig)) == 0) )
> +        {
> +            regs->eip += sizeof(sig);
> +            regs->eflags &= ~X86_EFLAGS_RF;
> +        }
> +    }
> +
>       hvm_emulate_prepare(&ctxt, regs);
>   
>       rc = hvm_emulate_one(&ctxt);
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index 3e66276..c0fbc8b 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -517,6 +517,13 @@ bool_t nhvm_vmcx_hap_enabled(struct vcpu *v);
>   /* interrupt */
>   enum hvm_intblk nhvm_interrupt_blocked(struct vcpu *v);
>   
> +#ifndef NDEBUG
> +/* Permit use of the Forced Emulation Prefix in HVM guests */
> +extern bool_t opt_hvm_fep;
> +#else
> +#define opt_hvm_fep 0
> +#endif
> +
>   #endif /* __ASM_X86_HVM_HVM_H__ */
>   
>   /*

Re: [PATCH v2 5/6] x86/hvm: Forced Emulation Prefix for debug builds of Xen

[Andrew Cooper]

On 23/09/14 19:20, Boris Ostrovsky wrote:
> On 09/23/2014 12:09 PM, Andrew Cooper wrote:
>> Analysis of XSAs 105 and 106 show that is possible to force a race
>> condition
>> which causes any arbitrary instruction to be emulated.
>>
>> To aid testing, explicitly introduce the Forced Emulation Prefix for
>> debug
>> builds alone.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>> CC: Jun Nakajima <jun.nakajima@intel.com>
>> CC: Eddie Dong <eddie.dong@intel.com>
>> CC: Kevin Tian <kevin.tian@intel.com>
>>
>> ---
>> v2: (all suggested by Jan)
>>   * Use hvm_fetch_from_guest_virt_nofault() in preference to
>> copy_from_guest()
>>   * Vastly reduce use of #ifndef NDEBUG
>> ---
>>   docs/misc/xen-command-line.markdown |   11 +++++++++++
>>   xen/arch/x86/hvm/hvm.c              |    5 +++++
>>   xen/arch/x86/hvm/svm/svm.c          |   13 +++++++++++++
>>   xen/arch/x86/hvm/vmx/vmx.c          |   13 +++++++++++++
>>   xen/include/asm-x86/hvm/hvm.h       |    7 +++++++
>>   5 files changed, 49 insertions(+)
>>
>> diff --git a/docs/misc/xen-command-line.markdown
>> b/docs/misc/xen-command-line.markdown
>> index af93e17..389701a 100644
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -682,6 +682,17 @@ Bit 11 - MSR operation logging
>>     Recognized in debug builds of the hypervisor only.
>>   +### hvm\_fep
>> +> `= <boolean>`
>> +
>> +> Default: `false`
>> +
>> +Allow use of the Forced Emulation Prefix in HVM guests, to allow
>> emulation of
>> +arbitrary instructions.
>> +
>> +This option is intended for development purposes, and is only
>> available in
>> +debug builds of the hypervisor.
>> +
>>   ### hvm\_port80
>>   > `= <boolean>`
>>   diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 5c7e0a4..34f28d0 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -86,6 +86,11 @@ unsigned long __attribute__ ((__section__
>> (".bss.page_aligned")))
>>   static bool_t __initdata opt_hap_enabled = 1;
>>   boolean_param("hap", opt_hap_enabled);
>>   +#ifndef opt_hvm_fep
>> +bool_t opt_hvm_fep;
>> +boolean_param("hvm_fep", opt_hvm_fep);
>> +#endif
>> +
>>   static int cpu_callback(
>>       struct notifier_block *nfb, unsigned long action, void *hcpu)
>>   {
>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>> index b6beefc..cda968b 100644
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -2118,6 +2118,19 @@ static void svm_vmexit_ud_intercept(struct
>> cpu_user_regs *regs)
>>       struct hvm_emulate_ctxt ctxt;
>>       int rc;
>>   +    if ( opt_hvm_fep )
>> +    {
>> +        char sig[5]; /* ud2; .ascii "xen" */
>> +
>> +        if ( (hvm_fetch_from_guest_virt_nofault(
>> +                  sig, regs->eip, sizeof(sig), 0) == HVMCOPY_okay) &&
>> +             (memcmp(sig, "\xf\xbxen", sizeof(sig)) == 0) )
>> +        {
>> +            regs->eip += sizeof(sig);
>> +            regs->eflags &= ~X86_EFLAGS_RF;
>> +        }
>> +    }
>
> This code is exactly the same for SVM and VMX. Can it be factored out?
>
> -boris

It can, and I considered that, but it would prevent optimising to
nothing for non-debug builds.  Given that it was a single simple if()
statement, I chose not to.

~Andrew

Re: [PATCH v2 5/6] x86/hvm: Forced Emulation Prefix for debug builds of Xen

[Boris Ostrovsky]

On 09/23/2014 02:23 PM, Andrew Cooper wrote:
> On 23/09/14 19:20, Boris Ostrovsky wrote:
>> On 09/23/2014 12:09 PM, Andrew Cooper wrote:
>>> Analysis of XSAs 105 and 106 show that is possible to force a race
>>> condition
>>> which causes any arbitrary instruction to be emulated.
>>>
>>> To aid testing, explicitly introduce the Forced Emulation Prefix for
>>> debug
>>> builds alone.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> CC: Keir Fraser <keir@xen.org>
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>>> CC: Jun Nakajima <jun.nakajima@intel.com>
>>> CC: Eddie Dong <eddie.dong@intel.com>
>>> CC: Kevin Tian <kevin.tian@intel.com>
>>>
>>> ---
>>> v2: (all suggested by Jan)
>>>    * Use hvm_fetch_from_guest_virt_nofault() in preference to
>>> copy_from_guest()
>>>    * Vastly reduce use of #ifndef NDEBUG
>>> ---
>>>    docs/misc/xen-command-line.markdown |   11 +++++++++++
>>>    xen/arch/x86/hvm/hvm.c              |    5 +++++
>>>    xen/arch/x86/hvm/svm/svm.c          |   13 +++++++++++++
>>>    xen/arch/x86/hvm/vmx/vmx.c          |   13 +++++++++++++
>>>    xen/include/asm-x86/hvm/hvm.h       |    7 +++++++
>>>    5 files changed, 49 insertions(+)
>>>
>>> diff --git a/docs/misc/xen-command-line.markdown
>>> b/docs/misc/xen-command-line.markdown
>>> index af93e17..389701a 100644
>>> --- a/docs/misc/xen-command-line.markdown
>>> +++ b/docs/misc/xen-command-line.markdown
>>> @@ -682,6 +682,17 @@ Bit 11 - MSR operation logging
>>>      Recognized in debug builds of the hypervisor only.
>>>    +### hvm\_fep
>>> +> `= <boolean>`
>>> +
>>> +> Default: `false`
>>> +
>>> +Allow use of the Forced Emulation Prefix in HVM guests, to allow
>>> emulation of
>>> +arbitrary instructions.
>>> +
>>> +This option is intended for development purposes, and is only
>>> available in
>>> +debug builds of the hypervisor.
>>> +
>>>    ### hvm\_port80
>>>    > `= <boolean>`
>>>    diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>> index 5c7e0a4..34f28d0 100644
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -86,6 +86,11 @@ unsigned long __attribute__ ((__section__
>>> (".bss.page_aligned")))
>>>    static bool_t __initdata opt_hap_enabled = 1;
>>>    boolean_param("hap", opt_hap_enabled);
>>>    +#ifndef opt_hvm_fep
>>> +bool_t opt_hvm_fep;
>>> +boolean_param("hvm_fep", opt_hvm_fep);
>>> +#endif
>>> +
>>>    static int cpu_callback(
>>>        struct notifier_block *nfb, unsigned long action, void *hcpu)
>>>    {
>>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>>> index b6beefc..cda968b 100644
>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>> @@ -2118,6 +2118,19 @@ static void svm_vmexit_ud_intercept(struct
>>> cpu_user_regs *regs)
>>>        struct hvm_emulate_ctxt ctxt;
>>>        int rc;
>>>    +    if ( opt_hvm_fep )
>>> +    {
>>> +        char sig[5]; /* ud2; .ascii "xen" */
>>> +
>>> +        if ( (hvm_fetch_from_guest_virt_nofault(
>>> +                  sig, regs->eip, sizeof(sig), 0) == HVMCOPY_okay) &&
>>> +             (memcmp(sig, "\xf\xbxen", sizeof(sig)) == 0) )
>>> +        {
>>> +            regs->eip += sizeof(sig);
>>> +            regs->eflags &= ~X86_EFLAGS_RF;
>>> +        }
>>> +    }
>> This code is exactly the same for SVM and VMX. Can it be factored out?
>>
>> -boris
> It can, and I considered that, but it would prevent optimising to
> nothing for non-debug builds.  Given that it was a single simple if()
> statement, I chose not to.

What about an inline (or a macro)? It won't help with code size but is a 
good thing from code maintainability point of vew.

-boris

Re: [PATCH 4/6] x86/emulate: Support for emulating software event injection

[Aravind Gopalakrishnan]

On 9/23/2014 10:03 AM, Andrew Cooper wrote:
> AMD SVM requires all software events to have their injection emulated if
> hardware lacks NextRIP support.  In addition, `icebp` (opcode 0xf1) injection
> requires emulation in all cases, even with hardware NextRIP support.
>
> Emulating full control transfers is overkill for our needs.  All that matters
> is that guest userspace can't bypass the descriptor DPL check.  Any guest OS
> which would incur other faults as part of injection is going to end up with a
> double fault instead, and won't be in a position to care that the faulting eip
> is wrong.
>
> Reported-by: Andrei LUTAS <vlutas@bitdefender.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> ---
>   xen/arch/x86/hvm/emulate.c             |    8 +++
>   xen/arch/x86/hvm/svm/svm.c             |   57 +++++++++++++--
>   xen/arch/x86/mm.c                      |    2 +
>   xen/arch/x86/mm/shadow/common.c        |    1 +
>   xen/arch/x86/x86_emulate/x86_emulate.c |  122 ++++++++++++++++++++++++++++++--
>   xen/arch/x86/x86_emulate/x86_emulate.h |   10 +++
>   6 files changed, 191 insertions(+), 9 deletions(-)
>
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index de982fd..b6beefc 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1177,11 +1177,12 @@ static void svm_inject_trap(struct hvm_trap *trap)
>       struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
>       eventinj_t event = vmcb->eventinj;
>       struct hvm_trap _trap = *trap;
> +    const struct cpu_user_regs *regs = guest_cpu_user_regs();
>   
>       switch ( _trap.vector )
>       {
>       case TRAP_debug:
> -        if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
> +        if ( regs->eflags & X86_EFLAGS_TF )
>           {
>               __restore_debug_registers(vmcb, curr);
>               vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | 0x4000);
> @@ -1209,10 +1210,58 @@ static void svm_inject_trap(struct hvm_trap *trap)
>   
>       event.bytes = 0;
>       event.fields.v = 1;
> -    event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;

Why remove the above line?
It's common for all cases below.
We can leave it as it is and set it selectively to 
X86_EVENTTYPE_SW_INTERRUPT as
you do in 'case X86_EVENTTYPE_SW_INTERRUPT ' right?

-Aravind.

>       event.fields.vector = _trap.vector;
> -    event.fields.ev = (_trap.error_code != HVM_DELIVER_NO_ERROR_CODE);
> -    event.fields.errorcode = _trap.error_code;
> +
> +    /* Refer to AMD Vol 2: System Programming, 15.20 Event Injection. */
> +    switch ( _trap.type )
> +    {
> +    case X86_EVENTTYPE_SW_INTERRUPT: /* int $n */
> +        /*
> +         * Injection type 4 (software interrupt) is only supported with
> +         * NextRIP support.  Without NextRIP, the emulator will have performed
> +         * DPL and presence checks for us.
> +         */
> +        if ( cpu_has_svm_nrips )
> +        {
> +            vmcb->nextrip = regs->eip + _trap.insn_len;
> +            event.fields.type = X86_EVENTTYPE_SW_INTERRUPT;
> +        }
> +        else
> +            event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
> +        break;
> +
> +    case X86_EVENTTYPE_PRI_SW_EXCEPTION: /* icebp */
> +        /*
> +         * icebp's injection must always be emulated.  Software injection help
> +         * in x86_emulate has moved eip forward, but NextRIP (if used) still
> +         * needs setting or execution will resume from 0.
> +         */
> +        if ( cpu_has_svm_nrips )
> +            vmcb->nextrip = regs->eip;
> +        event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
> +        break;
> +
> +    case X86_EVENTTYPE_SW_EXCEPTION: /* int3, into */
> +        /*
> +         * The AMD manual states that .type=3 (HW exception), .vector=3 or 4,
> +         * will perform DPL checks.  Experimentally, DPL and presence checks
> +         * are indeed performed, even without NextRIP support.
> +         *
> +         * However without NextRIP support, the event injection still needs
> +         * fully emulating to get the correct eip in the trap frame, yet get
> +         * the correct faulting eip should a fault occur.
> +         */
> +        if ( cpu_has_svm_nrips )
> +            vmcb->nextrip = regs->eip + _trap.insn_len;
> +        event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
> +        break;
> +
> +    default:
> +        event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
> +        event.fields.ev = (_trap.error_code != HVM_DELIVER_NO_ERROR_CODE);
> +        event.fields.errorcode = _trap.error_code;
> +        break;
> +    }
>   
>

Re: [PATCH 4/6] x86/emulate: Support for emulating software event injection

[Andrew Cooper]

On 23/09/14 23:24, Aravind Gopalakrishnan wrote:
> On 9/23/2014 10:03 AM, Andrew Cooper wrote:
>> AMD SVM requires all software events to have their injection emulated if
>> hardware lacks NextRIP support.  In addition, `icebp` (opcode 0xf1)
>> injection
>> requires emulation in all cases, even with hardware NextRIP support.
>>
>> Emulating full control transfers is overkill for our needs.  All that
>> matters
>> is that guest userspace can't bypass the descriptor DPL check.  Any
>> guest OS
>> which would incur other faults as part of injection is going to end
>> up with a
>> double fault instead, and won't be in a position to care that the
>> faulting eip
>> is wrong.
>>
>> Reported-by: Andrei LUTAS <vlutas@bitdefender.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>> ---
>>   xen/arch/x86/hvm/emulate.c             |    8 +++
>>   xen/arch/x86/hvm/svm/svm.c             |   57 +++++++++++++--
>>   xen/arch/x86/mm.c                      |    2 +
>>   xen/arch/x86/mm/shadow/common.c        |    1 +
>>   xen/arch/x86/x86_emulate/x86_emulate.c |  122
>> ++++++++++++++++++++++++++++++--
>>   xen/arch/x86/x86_emulate/x86_emulate.h |   10 +++
>>   6 files changed, 191 insertions(+), 9 deletions(-)
>>
>>
>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>> index de982fd..b6beefc 100644
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -1177,11 +1177,12 @@ static void svm_inject_trap(struct hvm_trap
>> *trap)
>>       struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
>>       eventinj_t event = vmcb->eventinj;
>>       struct hvm_trap _trap = *trap;
>> +    const struct cpu_user_regs *regs = guest_cpu_user_regs();
>>         switch ( _trap.vector )
>>       {
>>       case TRAP_debug:
>> -        if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
>> +        if ( regs->eflags & X86_EFLAGS_TF )
>>           {
>>               __restore_debug_registers(vmcb, curr);
>>               vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | 0x4000);
>> @@ -1209,10 +1210,58 @@ static void svm_inject_trap(struct hvm_trap
>> *trap)
>>         event.bytes = 0;
>>       event.fields.v = 1;
>> -    event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
>
> Why remove the above line?
> It's common for all cases below.
> We can leave it as it is and set it selectively to
> X86_EVENTTYPE_SW_INTERRUPT as
> you do in 'case X86_EVENTTYPE_SW_INTERRUPT ' right?
>
> -Aravind.

Mainly because that's how the diff ended up between the current code and
final implementation (which several different implementations inbetween).

Having said that, setting type to 0, then to HW, then possibly to SW is
a little inefficient.

~Andrew

>
>>       event.fields.vector = _trap.vector;
>> -    event.fields.ev = (_trap.error_code != HVM_DELIVER_NO_ERROR_CODE);
>> -    event.fields.errorcode = _trap.error_code;
>> +
>> +    /* Refer to AMD Vol 2: System Programming, 15.20 Event
>> Injection. */
>> +    switch ( _trap.type )
>> +    {
>> +    case X86_EVENTTYPE_SW_INTERRUPT: /* int $n */
>> +        /*
>> +         * Injection type 4 (software interrupt) is only supported with
>> +         * NextRIP support.  Without NextRIP, the emulator will have
>> performed
>> +         * DPL and presence checks for us.
>> +         */
>> +        if ( cpu_has_svm_nrips )
>> +        {
>> +            vmcb->nextrip = regs->eip + _trap.insn_len;
>> +            event.fields.type = X86_EVENTTYPE_SW_INTERRUPT;
>> +        }
>> +        else
>> +            event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
>> +        break;
>> +
>> +    case X86_EVENTTYPE_PRI_SW_EXCEPTION: /* icebp */
>> +        /*
>> +         * icebp's injection must always be emulated.  Software
>> injection help
>> +         * in x86_emulate has moved eip forward, but NextRIP (if
>> used) still
>> +         * needs setting or execution will resume from 0.
>> +         */
>> +        if ( cpu_has_svm_nrips )
>> +            vmcb->nextrip = regs->eip;
>> +        event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
>> +        break;
>> +
>> +    case X86_EVENTTYPE_SW_EXCEPTION: /* int3, into */
>> +        /*
>> +         * The AMD manual states that .type=3 (HW exception),
>> .vector=3 or 4,
>> +         * will perform DPL checks.  Experimentally, DPL and
>> presence checks
>> +         * are indeed performed, even without NextRIP support.
>> +         *
>> +         * However without NextRIP support, the event injection
>> still needs
>> +         * fully emulating to get the correct eip in the trap frame,
>> yet get
>> +         * the correct faulting eip should a fault occur.
>> +         */
>> +        if ( cpu_has_svm_nrips )
>> +            vmcb->nextrip = regs->eip + _trap.insn_len;
>> +        event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
>> +        break;
>> +
>> +    default:
>> +        event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
>> +        event.fields.ev = (_trap.error_code !=
>> HVM_DELIVER_NO_ERROR_CODE);
>> +        event.fields.errorcode = _trap.error_code;
>> +        break;
>> +    }
>>  
>

Re: [PATCH v2 5/6] x86/hvm: Forced Emulation Prefix for debug builds of Xen

[Andrew Cooper]

On 23/09/14 21:17, Boris Ostrovsky wrote:
> On 09/23/2014 02:23 PM, Andrew Cooper wrote:
>> On 23/09/14 19:20, Boris Ostrovsky wrote:
>>> On 09/23/2014 12:09 PM, Andrew Cooper wrote:
>>>> Analysis of XSAs 105 and 106 show that is possible to force a race
>>>> condition
>>>> which causes any arbitrary instruction to be emulated.
>>>>
>>>> To aid testing, explicitly introduce the Forced Emulation Prefix for
>>>> debug
>>>> builds alone.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> CC: Keir Fraser <keir@xen.org>
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>>> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>>>> CC: Jun Nakajima <jun.nakajima@intel.com>
>>>> CC: Eddie Dong <eddie.dong@intel.com>
>>>> CC: Kevin Tian <kevin.tian@intel.com>
>>>>
>>>> ---
>>>> v2: (all suggested by Jan)
>>>>    * Use hvm_fetch_from_guest_virt_nofault() in preference to
>>>> copy_from_guest()
>>>>    * Vastly reduce use of #ifndef NDEBUG
>>>> ---
>>>>    docs/misc/xen-command-line.markdown |   11 +++++++++++
>>>>    xen/arch/x86/hvm/hvm.c              |    5 +++++
>>>>    xen/arch/x86/hvm/svm/svm.c          |   13 +++++++++++++
>>>>    xen/arch/x86/hvm/vmx/vmx.c          |   13 +++++++++++++
>>>>    xen/include/asm-x86/hvm/hvm.h       |    7 +++++++
>>>>    5 files changed, 49 insertions(+)
>>>>
>>>> diff --git a/docs/misc/xen-command-line.markdown
>>>> b/docs/misc/xen-command-line.markdown
>>>> index af93e17..389701a 100644
>>>> --- a/docs/misc/xen-command-line.markdown
>>>> +++ b/docs/misc/xen-command-line.markdown
>>>> @@ -682,6 +682,17 @@ Bit 11 - MSR operation logging
>>>>      Recognized in debug builds of the hypervisor only.
>>>>    +### hvm\_fep
>>>> +> `= <boolean>`
>>>> +
>>>> +> Default: `false`
>>>> +
>>>> +Allow use of the Forced Emulation Prefix in HVM guests, to allow
>>>> emulation of
>>>> +arbitrary instructions.
>>>> +
>>>> +This option is intended for development purposes, and is only
>>>> available in
>>>> +debug builds of the hypervisor.
>>>> +
>>>>    ### hvm\_port80
>>>>    > `= <boolean>`
>>>>    diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>>> index 5c7e0a4..34f28d0 100644
>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>> @@ -86,6 +86,11 @@ unsigned long __attribute__ ((__section__
>>>> (".bss.page_aligned")))
>>>>    static bool_t __initdata opt_hap_enabled = 1;
>>>>    boolean_param("hap", opt_hap_enabled);
>>>>    +#ifndef opt_hvm_fep
>>>> +bool_t opt_hvm_fep;
>>>> +boolean_param("hvm_fep", opt_hvm_fep);
>>>> +#endif
>>>> +
>>>>    static int cpu_callback(
>>>>        struct notifier_block *nfb, unsigned long action, void *hcpu)
>>>>    {
>>>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>>>> index b6beefc..cda968b 100644
>>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>>> @@ -2118,6 +2118,19 @@ static void svm_vmexit_ud_intercept(struct
>>>> cpu_user_regs *regs)
>>>>        struct hvm_emulate_ctxt ctxt;
>>>>        int rc;
>>>>    +    if ( opt_hvm_fep )
>>>> +    {
>>>> +        char sig[5]; /* ud2; .ascii "xen" */
>>>> +
>>>> +        if ( (hvm_fetch_from_guest_virt_nofault(
>>>> +                  sig, regs->eip, sizeof(sig), 0) == HVMCOPY_okay) &&
>>>> +             (memcmp(sig, "\xf\xbxen", sizeof(sig)) == 0) )
>>>> +        {
>>>> +            regs->eip += sizeof(sig);
>>>> +            regs->eflags &= ~X86_EFLAGS_RF;
>>>> +        }
>>>> +    }
>>> This code is exactly the same for SVM and VMX. Can it be factored out?
>>>
>>> -boris
>> It can, and I considered that, but it would prevent optimising to
>> nothing for non-debug builds.  Given that it was a single simple if()
>> statement, I chose not to.
>
> What about an inline (or a macro)? It won't help with code size but is
> a good thing from code maintainability point of vew.
>
> -boris
>

I have vehement dislike of macros for things like this, where a static
inline should be used.

When attempting to make a static inline, the use of <asm/hvm/support.h>
in include/asm-x86/hvm.h (which is the only logical place for it IMO)
causes a failure to compile due to include dependency issues, and an
incomplete type definitions of struct domain and vcpu.

I currently lack enough TUITs to investigate.

~Andrew

Re: [PATCH 4/6] x86/emulate: Support for emulating software event injection

[Boris Ostrovsky]

On 09/23/2014 11:03 AM, Andrew Cooper wrote:
> AMD SVM requires all software events to have their injection emulated if
> hardware lacks NextRIP support.  In addition, `icebp` (opcode 0xf1) injection
> requires emulation in all cases, even with hardware NextRIP support.
>
> Emulating full control transfers is overkill for our needs.  All that matters
> is that guest userspace can't bypass the descriptor DPL check.  Any guest OS
> which would incur other faults as part of injection is going to end up with a
> double fault instead, and won't be in a position to care that the faulting eip
> is wrong.
>
> Reported-by: Andrei LUTAS <vlutas@bitdefender.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> ---
>   xen/arch/x86/hvm/emulate.c             |    8 +++
>   xen/arch/x86/hvm/svm/svm.c             |   57 +++++++++++++--
>   xen/arch/x86/mm.c                      |    2 +
>   xen/arch/x86/mm/shadow/common.c        |    1 +
>   xen/arch/x86/x86_emulate/x86_emulate.c |  122 ++++++++++++++++++++++++++++++--
>   xen/arch/x86/x86_emulate/x86_emulate.h |   10 +++
>   6 files changed, 191 insertions(+), 9 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 7ee146b..463ccfb 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -21,6 +21,7 @@
>   #include <asm/hvm/hvm.h>
>   #include <asm/hvm/trace.h>
>   #include <asm/hvm/support.h>
> +#include <asm/hvm/svm/svm.h>
>   
>   static void hvmtrace_io_assist(int is_mmio, ioreq_t *p)
>   {
> @@ -1328,6 +1329,13 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
>       vio->mmio_retrying = vio->mmio_retry;
>       vio->mmio_retry = 0;
>   
> +    if ( cpu_has_vmx )
> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_none;
> +    else if ( cpu_has_svm_nrips )
> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_icebp;
> +    else
> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_all;
> +
>       rc = x86_emulate(&hvmemul_ctxt->ctxt, ops);
>   
>       if ( rc == X86EMUL_OKAY && vio->mmio_retry )
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index de982fd..b6beefc 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1177,11 +1177,12 @@ static void svm_inject_trap(struct hvm_trap *trap)
>       struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
>       eventinj_t event = vmcb->eventinj;
>       struct hvm_trap _trap = *trap;
> +    const struct cpu_user_regs *regs = guest_cpu_user_regs();
>   
>       switch ( _trap.vector )
>       {
>       case TRAP_debug:
> -        if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
> +        if ( regs->eflags & X86_EFLAGS_TF )
>           {
>               __restore_debug_registers(vmcb, curr);
>               vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | 0x4000);
> @@ -1209,10 +1210,58 @@ static void svm_inject_trap(struct hvm_trap *trap)
>   
>       event.bytes = 0;
>       event.fields.v = 1;
> -    event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
>       event.fields.vector = _trap.vector;
> -    event.fields.ev = (_trap.error_code != HVM_DELIVER_NO_ERROR_CODE);
> -    event.fields.errorcode = _trap.error_code;
> +
> +    /* Refer to AMD Vol 2: System Programming, 15.20 Event Injection. */
> +    switch ( _trap.type )
> +    {
> +    case X86_EVENTTYPE_SW_INTERRUPT: /* int $n */
> +        /*
> +         * Injection type 4 (software interrupt) is only supported with
> +         * NextRIP support.  Without NextRIP, the emulator will have performed
> +         * DPL and presence checks for us.
> +         */
> +        if ( cpu_has_svm_nrips )
> +        {
> +            vmcb->nextrip = regs->eip + _trap.insn_len;
> +            event.fields.type = X86_EVENTTYPE_SW_INTERRUPT;
> +        }
> +        else
> +            event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
> +        break;
> +
> +    case X86_EVENTTYPE_PRI_SW_EXCEPTION: /* icebp */
> +        /*
> +         * icebp's injection must always be emulated.  Software injection help
> +         * in x86_emulate has moved eip forward, but NextRIP (if used) still
> +         * needs setting or execution will resume from 0.
> +         */


Can you tell me where eip is updated? I don't see any difference between 
how, for example, int3 is emulated differently from icebp.

-boris


> +        if ( cpu_has_svm_nrips )
> +            vmcb->nextrip = regs->eip;
> +        event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
> +        break;
> +
> +    case X86_EVENTTYPE_SW_EXCEPTION: /* int3, into */
> +        /*
> +         * The AMD manual states that .type=3 (HW exception), .vector=3 or 4,
> +         * will perform DPL checks.  Experimentally, DPL and presence checks
> +         * are indeed performed, even without NextRIP support.
> +         *
> +         * However without NextRIP support, the event injection still needs
> +         * fully emulating to get the correct eip in the trap frame, yet get
> +         * the correct faulting eip should a fault occur.
> +         */
> +        if ( cpu_has_svm_nrips )
> +            vmcb->nextrip = regs->eip + _trap.insn_len;
> +        event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
> +        break;
> +
> +    default:
> +        event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
> +        event.fields.ev = (_trap.error_code != HVM_DELIVER_NO_ERROR_CODE);
> +        event.fields.errorcode = _trap.error_code;
> +        break;
> +    }
>   
>       vmcb->eventinj = event;
>   
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 5b3f06f..bfe9f05 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5096,6 +5096,7 @@ int ptwr_do_page_fault(struct vcpu *v, unsigned long addr,
>       ptwr_ctxt.ctxt.force_writeback = 0;
>       ptwr_ctxt.ctxt.addr_size = ptwr_ctxt.ctxt.sp_size =
>           is_pv_32on64_domain(d) ? 32 : BITS_PER_LONG;
> +    ptwr_ctxt.ctxt.swint_emulate = x86_swint_emulate_none;
>       ptwr_ctxt.cr2 = addr;
>       ptwr_ctxt.pte = pte;
>   
> @@ -5172,6 +5173,7 @@ int mmio_ro_do_page_fault(struct vcpu *v, unsigned long addr,
>           .ctxt.regs = regs,
>           .ctxt.addr_size = addr_size,
>           .ctxt.sp_size = addr_size,
> +        .ctxt.swint_emulate = x86_swint_emulate_none,
>           .cr2 = addr
>       };
>       int rc;
> diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
> index 9115a78..a5eed28 100644
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -366,6 +366,7 @@ const struct x86_emulate_ops *shadow_init_emulation(
>   
>       sh_ctxt->ctxt.regs = regs;
>       sh_ctxt->ctxt.force_writeback = 0;
> +    sh_ctxt->ctxt.swint_emulate = x86_swint_emulate_none;
>   
>       if ( is_pv_vcpu(v) )
>       {
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
> index e06aa60..ffca65a 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -403,6 +403,11 @@ typedef union {
>   #define EXC_PF 14
>   #define EXC_MF 16
>   
> +/* Segment selector error code bits. */
> +#define ECODE_EXT (1 << 0)
> +#define ECODE_IDT (1 << 1)
> +#define ECODE_TI  (1 << 2)
> +
>   /*
>    * Instruction emulation:
>    * Most instructions are emulated directly via a fragment of inline assembly
> @@ -1318,6 +1323,115 @@ decode_segment(uint8_t modrm_reg)
>       return decode_segment_failed;
>   }
>   
> +/* Inject a software interrupt/exception, emulating if needed. */
> +static int inject_swint(enum x86_swint_type type,
> +                        uint8_t vector, uint8_t insn_len,
> +                        struct x86_emulate_ctxt *ctxt,
> +                        const struct x86_emulate_ops *ops)
> +{
> +    int rc, error_code, fault_type = EXC_GP;
> +
> +    fail_if(ops->inject_sw_interrupt == NULL);
> +    fail_if(ops->inject_hw_exception == NULL);
> +
> +    /*
> +     * Without hardware support, injecting software interrupts/exceptions is
> +     * problematic.
> +     *
> +     * All software methods of generating exceptions (other than BOUND) yield
> +     * traps, so eip in the exception frame needs to point after the
> +     * instruction, not at it.
> +     *
> +     * However, if injecting it as a hardware exception causes a fault during
> +     * delivery, our adjustment of eip will cause the fault to be reported
> +     * after the faulting instruction, not pointing to it.
> +     *
> +     * Therefore, eip can only safely be wound forwards if we are certain that
> +     * injecting an equivalent hardware exception won't fault, which means
> +     * emulating everything the processor would do on a control transfer.
> +     *
> +     * However, emulation of complete control transfers is very complicated.
> +     * All we care about is that guest userspace cannot avoid the descriptor
> +     * DPL check by using the Xen emulator, and successfully invoke DPL=0
> +     * descriptors.
> +     *
> +     * Any OS which would further fault during injection is going to receive a
> +     * double fault anyway, and won't be in a position to care that the
> +     * faulting eip is incorrect.
> +     */
> +
> +    if ( (ctxt->swint_emulate == x86_swint_emulate_all) ||
> +         ((ctxt->swint_emulate == x86_swint_emulate_icebp) &&
> +          (type == x86_swint_icebp)) )
> +    {
> +        if ( !in_realmode(ctxt, ops) )
> +        {
> +            unsigned int idte_size = (ctxt->addr_size == 64) ? 16 : 8;
> +            unsigned int idte_offset = vector * idte_size;
> +            struct segment_register idtr;
> +            uint32_t idte_ctl;
> +
> +            /* icebp sets the External Event bit despite being an instruction. */
> +            error_code = (vector << 3) | ECODE_IDT |
> +                (type == x86_swint_icebp ? ECODE_EXT : 0);
> +
> +            /*
> +             * TODO - this does not cover the v8086 mode with CR4.VME case
> +             * correctly, but falls on the safe side from the point of view of
> +             * a 32bit OS.  Someone with many TUITs can see about reading the
> +             * TSS Software Interrupt Redirection bitmap.
> +             */
> +            if ( (ctxt->regs->eflags & EFLG_VM) &&
> +                 ((ctxt->regs->eflags & EFLG_IOPL) != EFLG_IOPL) )
> +                goto raise_exn;
> +
> +            fail_if(ops->read_segment == NULL);
> +            fail_if(ops->read == NULL);
> +            if ( (rc = ops->read_segment(x86_seg_idtr, &idtr, ctxt)) )
> +                goto done;
> +
> +            if ( (idte_offset + idte_size - 1) > idtr.limit )
> +                goto raise_exn;
> +
> +            /*
> +             * Should strictly speaking read all 8/16 bytes of an entry,
> +             * but we currently only care about the dpl and present bits.
> +             */
> +            ops->read(x86_seg_none, idtr.base + idte_offset + 4,
> +                      &idte_ctl, sizeof(idte_ctl), ctxt);
> +
> +            /* Is this entry present? */
> +            if ( !(idte_ctl & (1u << 15)) )
> +            {
> +                fault_type = EXC_NP;
> +                goto raise_exn;
> +            }
> +
> +            /* icebp counts as a hardware event, and bypasses the dpl check. */
> +            if ( type != x86_swint_icebp )
> +            {
> +                struct segment_register ss;
> +
> +                if ( (rc = ops->read_segment(x86_seg_ss, &ss, ctxt)) )
> +                    goto done;
> +
> +                if ( ss.attr.fields.dpl > ((idte_ctl >> 13) & 3) )
> +                    goto raise_exn;
> +            }
> +        }
> +
> +        ctxt->regs->eip += insn_len;
> +    }
> +
> +    rc = ops->inject_sw_interrupt(type, vector, insn_len, ctxt);
> +
> + done:
> +    return rc;
> +
> + raise_exn:
> +    return ops->inject_hw_exception(fault_type, error_code, ctxt);
> +}
> +
>   int
>   x86_emulate(
>       struct x86_emulate_ctxt *ctxt,
> @@ -2637,11 +2751,9 @@ x86_emulate(
>           src.val = insn_fetch_type(uint8_t);
>           swint_type = x86_swint_int;
>       swint:
> -        fail_if(!in_realmode(ctxt, ops)); /* XSA-106 */
> -        fail_if(ops->inject_sw_interrupt == NULL);
> -        rc = ops->inject_sw_interrupt(swint_type, src.val,
> -                                      _regs.eip - ctxt->regs->eip,
> -                                      ctxt) ? : X86EMUL_EXCEPTION;
> +        rc = inject_swint(swint_type, src.val,
> +                          _regs.eip - ctxt->regs->eip,
> +                          ctxt, ops) ? : X86EMUL_EXCEPTION;
>           goto done;
>   
>       case 0xce: /* into */
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
> index b336e17..b059341 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -59,6 +59,13 @@ enum x86_swint_type {
>       x86_swint_int,   /* 0xcd $n */
>   };
>   
> +/* How much help is required with software event injection? */
> +enum x86_swint_emulation {
> +    x86_swint_emulate_none, /* Hardware supports all software injection properly */
> +    x86_swint_emulate_icebp,/* Help needed with `icebp` (0xf1) */
> +    x86_swint_emulate_all,  /* Help needed with all software events */
> +};
> +
>   /*
>    * Attribute for segment selector. This is a copy of bit 40:47 & 52:55 of the
>    * segment descriptor. It happens to match the format of an AMD SVM VMCB.
> @@ -388,6 +395,9 @@ struct x86_emulate_ctxt
>       /* Set this if writes may have side effects. */
>       uint8_t force_writeback;
>   
> +    /* Software event injection support. */
> +    enum x86_swint_emulation swint_emulate;
> +
>       /* Retirement state, set by the emulator (valid only on X86EMUL_OKAY). */
>       union {
>           struct {

Re: [PATCH 4/6] x86/emulate: Support for emulating software event injection

[Andrew Cooper]

On 24/09/14 14:01, Boris Ostrovsky wrote:
> On 09/23/2014 11:03 AM, Andrew Cooper wrote:
>> AMD SVM requires all software events to have their injection emulated if
>> hardware lacks NextRIP support.  In addition, `icebp` (opcode 0xf1)
>> injection
>> requires emulation in all cases, even with hardware NextRIP support.
>>
>> Emulating full control transfers is overkill for our needs.  All that
>> matters
>> is that guest userspace can't bypass the descriptor DPL check.  Any
>> guest OS
>> which would incur other faults as part of injection is going to end
>> up with a
>> double fault instead, and won't be in a position to care that the
>> faulting eip
>> is wrong.
>>
>> Reported-by: Andrei LUTAS <vlutas@bitdefender.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>> ---
>>   xen/arch/x86/hvm/emulate.c             |    8 +++
>>   xen/arch/x86/hvm/svm/svm.c             |   57 +++++++++++++--
>>   xen/arch/x86/mm.c                      |    2 +
>>   xen/arch/x86/mm/shadow/common.c        |    1 +
>>   xen/arch/x86/x86_emulate/x86_emulate.c |  122
>> ++++++++++++++++++++++++++++++--
>>   xen/arch/x86/x86_emulate/x86_emulate.h |   10 +++
>>   6 files changed, 191 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>> index 7ee146b..463ccfb 100644
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -21,6 +21,7 @@
>>   #include <asm/hvm/hvm.h>
>>   #include <asm/hvm/trace.h>
>>   #include <asm/hvm/support.h>
>> +#include <asm/hvm/svm/svm.h>
>>     static void hvmtrace_io_assist(int is_mmio, ioreq_t *p)
>>   {
>> @@ -1328,6 +1329,13 @@ static int _hvm_emulate_one(struct
>> hvm_emulate_ctxt *hvmemul_ctxt,
>>       vio->mmio_retrying = vio->mmio_retry;
>>       vio->mmio_retry = 0;
>>   +    if ( cpu_has_vmx )
>> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_none;
>> +    else if ( cpu_has_svm_nrips )
>> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_icebp;
>> +    else
>> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_all;
>> +
>>       rc = x86_emulate(&hvmemul_ctxt->ctxt, ops);
>>         if ( rc == X86EMUL_OKAY && vio->mmio_retry )
>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>> index de982fd..b6beefc 100644
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -1177,11 +1177,12 @@ static void svm_inject_trap(struct hvm_trap
>> *trap)
>>       struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
>>       eventinj_t event = vmcb->eventinj;
>>       struct hvm_trap _trap = *trap;
>> +    const struct cpu_user_regs *regs = guest_cpu_user_regs();
>>         switch ( _trap.vector )
>>       {
>>       case TRAP_debug:
>> -        if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
>> +        if ( regs->eflags & X86_EFLAGS_TF )
>>           {
>>               __restore_debug_registers(vmcb, curr);
>>               vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | 0x4000);
>> @@ -1209,10 +1210,58 @@ static void svm_inject_trap(struct hvm_trap
>> *trap)
>>         event.bytes = 0;
>>       event.fields.v = 1;
>> -    event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
>>       event.fields.vector = _trap.vector;
>> -    event.fields.ev = (_trap.error_code != HVM_DELIVER_NO_ERROR_CODE);
>> -    event.fields.errorcode = _trap.error_code;
>> +
>> +    /* Refer to AMD Vol 2: System Programming, 15.20 Event
>> Injection. */
>> +    switch ( _trap.type )
>> +    {
>> +    case X86_EVENTTYPE_SW_INTERRUPT: /* int $n */
>> +        /*
>> +         * Injection type 4 (software interrupt) is only supported with
>> +         * NextRIP support.  Without NextRIP, the emulator will have
>> performed
>> +         * DPL and presence checks for us.
>> +         */
>> +        if ( cpu_has_svm_nrips )
>> +        {
>> +            vmcb->nextrip = regs->eip + _trap.insn_len;
>> +            event.fields.type = X86_EVENTTYPE_SW_INTERRUPT;
>> +        }
>> +        else
>> +            event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
>> +        break;
>> +
>> +    case X86_EVENTTYPE_PRI_SW_EXCEPTION: /* icebp */
>> +        /*
>> +         * icebp's injection must always be emulated.  Software
>> injection help
>> +         * in x86_emulate has moved eip forward, but NextRIP (if
>> used) still
>> +         * needs setting or execution will resume from 0.
>> +         */
>
>
> Can you tell me where eip is updated? I don't see any difference
> between how, for example, int3 is emulated differently from icebp.
>
> -boris

The second hunk in this series, chooses between x86_swint_emulate_icebp
and x86_swint_emulate_all based on NRIP support.

This cause inject_swint() to either emulate the injection or not.

~Andrew

Re: [PATCH 4/6] x86/emulate: Support for emulating software event injection

[Boris Ostrovsky]

On 09/24/2014 09:04 AM, Andrew Cooper wrote:
> On 24/09/14 14:01, Boris Ostrovsky wrote:
>> On 09/23/2014 11:03 AM, Andrew Cooper wrote:
>>> AMD SVM requires all software events to have their injection emulated if
>>> hardware lacks NextRIP support.  In addition, `icebp` (opcode 0xf1)
>>> injection
>>> requires emulation in all cases, even with hardware NextRIP support.
>>>
>>> Emulating full control transfers is overkill for our needs.  All that
>>> matters
>>> is that guest userspace can't bypass the descriptor DPL check.  Any
>>> guest OS
>>> which would incur other faults as part of injection is going to end
>>> up with a
>>> double fault instead, and won't be in a position to care that the
>>> faulting eip
>>> is wrong.
>>>
>>> Reported-by: Andrei LUTAS <vlutas@bitdefender.com>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>>> ---
>>>    xen/arch/x86/hvm/emulate.c             |    8 +++
>>>    xen/arch/x86/hvm/svm/svm.c             |   57 +++++++++++++--
>>>    xen/arch/x86/mm.c                      |    2 +
>>>    xen/arch/x86/mm/shadow/common.c        |    1 +
>>>    xen/arch/x86/x86_emulate/x86_emulate.c |  122
>>> ++++++++++++++++++++++++++++++--
>>>    xen/arch/x86/x86_emulate/x86_emulate.h |   10 +++
>>>    6 files changed, 191 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>>> index 7ee146b..463ccfb 100644
>>> --- a/xen/arch/x86/hvm/emulate.c
>>> +++ b/xen/arch/x86/hvm/emulate.c
>>> @@ -21,6 +21,7 @@
>>>    #include <asm/hvm/hvm.h>
>>>    #include <asm/hvm/trace.h>
>>>    #include <asm/hvm/support.h>
>>> +#include <asm/hvm/svm/svm.h>
>>>      static void hvmtrace_io_assist(int is_mmio, ioreq_t *p)
>>>    {
>>> @@ -1328,6 +1329,13 @@ static int _hvm_emulate_one(struct
>>> hvm_emulate_ctxt *hvmemul_ctxt,
>>>        vio->mmio_retrying = vio->mmio_retry;
>>>        vio->mmio_retry = 0;
>>>    +    if ( cpu_has_vmx )
>>> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_none;
>>> +    else if ( cpu_has_svm_nrips )
>>> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_icebp;
>>> +    else
>>> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_all;
>>> +
>>>        rc = x86_emulate(&hvmemul_ctxt->ctxt, ops);
>>>          if ( rc == X86EMUL_OKAY && vio->mmio_retry )
>>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>>> index de982fd..b6beefc 100644
>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>> @@ -1177,11 +1177,12 @@ static void svm_inject_trap(struct hvm_trap
>>> *trap)
>>>        struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
>>>        eventinj_t event = vmcb->eventinj;
>>>        struct hvm_trap _trap = *trap;
>>> +    const struct cpu_user_regs *regs = guest_cpu_user_regs();
>>>          switch ( _trap.vector )
>>>        {
>>>        case TRAP_debug:
>>> -        if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
>>> +        if ( regs->eflags & X86_EFLAGS_TF )
>>>            {
>>>                __restore_debug_registers(vmcb, curr);
>>>                vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | 0x4000);
>>> @@ -1209,10 +1210,58 @@ static void svm_inject_trap(struct hvm_trap
>>> *trap)
>>>          event.bytes = 0;
>>>        event.fields.v = 1;
>>> -    event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
>>>        event.fields.vector = _trap.vector;
>>> -    event.fields.ev = (_trap.error_code != HVM_DELIVER_NO_ERROR_CODE);
>>> -    event.fields.errorcode = _trap.error_code;
>>> +
>>> +    /* Refer to AMD Vol 2: System Programming, 15.20 Event
>>> Injection. */
>>> +    switch ( _trap.type )
>>> +    {
>>> +    case X86_EVENTTYPE_SW_INTERRUPT: /* int $n */
>>> +        /*
>>> +         * Injection type 4 (software interrupt) is only supported with
>>> +         * NextRIP support.  Without NextRIP, the emulator will have
>>> performed
>>> +         * DPL and presence checks for us.
>>> +         */
>>> +        if ( cpu_has_svm_nrips )
>>> +        {
>>> +            vmcb->nextrip = regs->eip + _trap.insn_len;
>>> +            event.fields.type = X86_EVENTTYPE_SW_INTERRUPT;
>>> +        }
>>> +        else
>>> +            event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
>>> +        break;
>>> +
>>> +    case X86_EVENTTYPE_PRI_SW_EXCEPTION: /* icebp */
>>> +        /*
>>> +         * icebp's injection must always be emulated.  Software
>>> injection help
>>> +         * in x86_emulate has moved eip forward, but NextRIP (if
>>> used) still
>>> +         * needs setting or execution will resume from 0.
>>> +         */
>>
>> Can you tell me where eip is updated? I don't see any difference
>> between how, for example, int3 is emulated differently from icebp.
>>
>> -boris
> The second hunk in this series, chooses between x86_swint_emulate_icebp
> and x86_swint_emulate_all based on NRIP support.
>
> This cause inject_swint() to either emulate the injection or not.

I was asking about why you are calculating nextrip differently (i.e not 
adding _trap.insn_len) for icebp vs. int3. I read the comment as if it 
was saying that x86_emulate() has already taken care of that. And I 
don't understand where that is done.

-boris

Re: [PATCH 4/6] x86/emulate: Support for emulating software event injection

[Andrew Cooper]

On 24/09/14 14:24, Boris Ostrovsky wrote:
> On 09/24/2014 09:04 AM, Andrew Cooper wrote:
>> On 24/09/14 14:01, Boris Ostrovsky wrote:
>>> On 09/23/2014 11:03 AM, Andrew Cooper wrote:
>>>> AMD SVM requires all software events to have their injection
>>>> emulated if
>>>> hardware lacks NextRIP support.  In addition, `icebp` (opcode 0xf1)
>>>> injection
>>>> requires emulation in all cases, even with hardware NextRIP support.
>>>>
>>>> Emulating full control transfers is overkill for our needs.  All that
>>>> matters
>>>> is that guest userspace can't bypass the descriptor DPL check.  Any
>>>> guest OS
>>>> which would incur other faults as part of injection is going to end
>>>> up with a
>>>> double fault instead, and won't be in a position to care that the
>>>> faulting eip
>>>> is wrong.
>>>>
>>>> Reported-by: Andrei LUTAS <vlutas@bitdefender.com>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>>> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>>>> ---
>>>>    xen/arch/x86/hvm/emulate.c             |    8 +++
>>>>    xen/arch/x86/hvm/svm/svm.c             |   57 +++++++++++++--
>>>>    xen/arch/x86/mm.c                      |    2 +
>>>>    xen/arch/x86/mm/shadow/common.c        |    1 +
>>>>    xen/arch/x86/x86_emulate/x86_emulate.c |  122
>>>> ++++++++++++++++++++++++++++++--
>>>>    xen/arch/x86/x86_emulate/x86_emulate.h |   10 +++
>>>>    6 files changed, 191 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>>>> index 7ee146b..463ccfb 100644
>>>> --- a/xen/arch/x86/hvm/emulate.c
>>>> +++ b/xen/arch/x86/hvm/emulate.c
>>>> @@ -21,6 +21,7 @@
>>>>    #include <asm/hvm/hvm.h>
>>>>    #include <asm/hvm/trace.h>
>>>>    #include <asm/hvm/support.h>
>>>> +#include <asm/hvm/svm/svm.h>
>>>>      static void hvmtrace_io_assist(int is_mmio, ioreq_t *p)
>>>>    {
>>>> @@ -1328,6 +1329,13 @@ static int _hvm_emulate_one(struct
>>>> hvm_emulate_ctxt *hvmemul_ctxt,
>>>>        vio->mmio_retrying = vio->mmio_retry;
>>>>        vio->mmio_retry = 0;
>>>>    +    if ( cpu_has_vmx )
>>>> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_none;
>>>> +    else if ( cpu_has_svm_nrips )
>>>> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_icebp;
>>>> +    else
>>>> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_all;
>>>> +
>>>>        rc = x86_emulate(&hvmemul_ctxt->ctxt, ops);
>>>>          if ( rc == X86EMUL_OKAY && vio->mmio_retry )
>>>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>>>> index de982fd..b6beefc 100644
>>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>>> @@ -1177,11 +1177,12 @@ static void svm_inject_trap(struct hvm_trap
>>>> *trap)
>>>>        struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
>>>>        eventinj_t event = vmcb->eventinj;
>>>>        struct hvm_trap _trap = *trap;
>>>> +    const struct cpu_user_regs *regs = guest_cpu_user_regs();
>>>>          switch ( _trap.vector )
>>>>        {
>>>>        case TRAP_debug:
>>>> -        if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
>>>> +        if ( regs->eflags & X86_EFLAGS_TF )
>>>>            {
>>>>                __restore_debug_registers(vmcb, curr);
>>>>                vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | 0x4000);
>>>> @@ -1209,10 +1210,58 @@ static void svm_inject_trap(struct hvm_trap
>>>> *trap)
>>>>          event.bytes = 0;
>>>>        event.fields.v = 1;
>>>> -    event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
>>>>        event.fields.vector = _trap.vector;
>>>> -    event.fields.ev = (_trap.error_code !=
>>>> HVM_DELIVER_NO_ERROR_CODE);
>>>> -    event.fields.errorcode = _trap.error_code;
>>>> +
>>>> +    /* Refer to AMD Vol 2: System Programming, 15.20 Event
>>>> Injection. */
>>>> +    switch ( _trap.type )
>>>> +    {
>>>> +    case X86_EVENTTYPE_SW_INTERRUPT: /* int $n */
>>>> +        /*
>>>> +         * Injection type 4 (software interrupt) is only supported
>>>> with
>>>> +         * NextRIP support.  Without NextRIP, the emulator will have
>>>> performed
>>>> +         * DPL and presence checks for us.
>>>> +         */
>>>> +        if ( cpu_has_svm_nrips )
>>>> +        {
>>>> +            vmcb->nextrip = regs->eip + _trap.insn_len;
>>>> +            event.fields.type = X86_EVENTTYPE_SW_INTERRUPT;
>>>> +        }
>>>> +        else
>>>> +            event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
>>>> +        break;
>>>> +
>>>> +    case X86_EVENTTYPE_PRI_SW_EXCEPTION: /* icebp */
>>>> +        /*
>>>> +         * icebp's injection must always be emulated.  Software
>>>> injection help
>>>> +         * in x86_emulate has moved eip forward, but NextRIP (if
>>>> used) still
>>>> +         * needs setting or execution will resume from 0.
>>>> +         */
>>>
>>> Can you tell me where eip is updated? I don't see any difference
>>> between how, for example, int3 is emulated differently from icebp.
>>>
>>> -boris
>> The second hunk in this series, chooses between x86_swint_emulate_icebp
>> and x86_swint_emulate_all based on NRIP support.
>>
>> This cause inject_swint() to either emulate the injection or not.
>
> I was asking about why you are calculating nextrip differently (i.e
> not adding _trap.insn_len) for icebp vs. int3. I read the comment as
> if it was saying that x86_emulate() has already taken care of that.
> And I don't understand where that is done.

With NRIPS support, hardware can deal with injecting the trap and doing
DPL/presence checks, which means the eip in the exception frame might
either be a trap or a fault.

regs->eip and vmcb->nextrip bound the trapping instruction, with eip
pointing at it, and nextrip pointing after it.  These are the two
choices which get set in the exception frame, after hardware has decided
whether to send the trap, or fault because of a bad IDT setup.

Without NRIP support, the emulator must move regs->eip on to get the
trap frame eip pointing at the correct address as it is the only
available choice for an exception frame.

For ICEBP, there is no hardware support for injection at all (even with
NRIP available).  The emulator must move regs->eip forward to get the
correct trap eip for the non-NRIP case, but with NRIP support,
vmcb->nextrip must be set, or the resulting trap frame contains an eip of 0.

I suppose this looks a little weird because of the intersection of
x86_swint_emulate_icebp and x86_swint_emulate_all, but it is not correct
to inject a software event with NRIP support for icebp, as that would
cause hardware to do a DPL check and fail to set the external bit in a
#NP error code.

~Andrew

Re: [PATCH 3/6] x86/hvm: Don't discard the SW/HW event distinction from the emulator

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Tuesday, September 23, 2014 8:03 AM
> 
> Injecting emulator software events as hardware exceptions results in a bypass
> of DPL checks.  As the emulator doesn't perform DPL checks itself, guest
> userspace is capable of bypassing DPL checks and injecting arbitrary events.
> 
> Propagating software event information from the emulator allows VMX to
> now
> properly inject software events, including DPL and presence checks, as well
> correct fault/trap frames.
> 
> Reported-by: Andrei LUTAS <vlutas@bitdefender.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Tested-by: Andrei LUTAS <vlutas@bitdefender.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Eddie Dong <eddie.dong@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>

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

> ---
>  xen/arch/x86/hvm/emulate.c        |   41
> ++++++++++++++++++++++++++++---------
>  xen/arch/x86/hvm/io.c             |    2 +-
>  xen/arch/x86/hvm/svm/svm.c        |    2 +-
>  xen/arch/x86/hvm/vmx/realmode.c   |   14 ++++++-------
>  xen/arch/x86/hvm/vmx/vmx.c        |    2 +-
>  xen/include/asm-x86/hvm/emulate.h |    5 ++---
>  6 files changed, 43 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 5d5d765..7ee146b 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -441,9 +441,10 @@ static int hvmemul_virtual_to_linear(
> 
>      /* This is a singleton operation: fail it with an exception. */
>      hvmemul_ctxt->exn_pending = 1;
> -    hvmemul_ctxt->exn_vector = TRAP_gp_fault;
> -    hvmemul_ctxt->exn_error_code = 0;
> -    hvmemul_ctxt->exn_insn_len = 0;
> +    hvmemul_ctxt->trap.vector = TRAP_gp_fault;
> +    hvmemul_ctxt->trap.type = X86_EVENTTYPE_HW_EXCEPTION;
> +    hvmemul_ctxt->trap.error_code = 0;
> +    hvmemul_ctxt->trap.insn_len = 0;
>      return X86EMUL_EXCEPTION;
>  }
> 
> @@ -1111,9 +1112,10 @@ static int hvmemul_inject_hw_exception(
>          container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
> 
>      hvmemul_ctxt->exn_pending = 1;
> -    hvmemul_ctxt->exn_vector = vector;
> -    hvmemul_ctxt->exn_error_code = error_code;
> -    hvmemul_ctxt->exn_insn_len = 0;
> +    hvmemul_ctxt->trap.vector = vector;
> +    hvmemul_ctxt->trap.type = X86_EVENTTYPE_HW_EXCEPTION;
> +    hvmemul_ctxt->trap.error_code = error_code;
> +    hvmemul_ctxt->trap.insn_len = 0;
> 
>      return X86EMUL_OKAY;
>  }
> @@ -1127,10 +1129,29 @@ static int hvmemul_inject_sw_interrupt(
>      struct hvm_emulate_ctxt *hvmemul_ctxt =
>          container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
> 
> +    switch ( type )
> +    {
> +    case x86_swint_icebp:
> +        hvmemul_ctxt->trap.type = X86_EVENTTYPE_PRI_SW_EXCEPTION;
> +        break;
> +
> +    case x86_swint_int3:
> +    case x86_swint_into:
> +        hvmemul_ctxt->trap.type = X86_EVENTTYPE_SW_EXCEPTION;
> +        break;
> +
> +    case x86_swint_int:
> +        hvmemul_ctxt->trap.type = X86_EVENTTYPE_SW_INTERRUPT;
> +        break;
> +
> +    default:
> +        return X86EMUL_UNHANDLEABLE;
> +    }
> +
>      hvmemul_ctxt->exn_pending = 1;
> -    hvmemul_ctxt->exn_vector = vector;
> -    hvmemul_ctxt->exn_error_code = -1;
> -    hvmemul_ctxt->exn_insn_len = insn_len;
> +    hvmemul_ctxt->trap.vector = vector;
> +    hvmemul_ctxt->trap.error_code = HVM_DELIVER_NO_ERROR_CODE;
> +    hvmemul_ctxt->trap.insn_len = insn_len;
> 
>      return X86EMUL_OKAY;
>  }
> @@ -1404,7 +1425,7 @@ void hvm_mem_event_emulate_one(bool_t nowrite,
> unsigned int trapnr,
>          break;
>      case X86EMUL_EXCEPTION:
>          if ( ctx.exn_pending )
> -            hvm_inject_hw_exception(ctx.exn_vector,
> ctx.exn_error_code);
> +            hvm_inject_trap(&ctx.trap);
>          break;
>      }
> 
> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
> index 9f565d6..e5d5e79 100644
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -113,7 +113,7 @@ int handle_mmio(void)
>          return 0;
>      case X86EMUL_EXCEPTION:
>          if ( ctxt.exn_pending )
> -            hvm_inject_hw_exception(ctxt.exn_vector,
> ctxt.exn_error_code);
> +            hvm_inject_trap(&ctxt.trap);
>          break;
>      default:
>          break;
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 5d404ce..de982fd 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2080,7 +2080,7 @@ static void svm_vmexit_ud_intercept(struct
> cpu_user_regs *regs)
>          break;
>      case X86EMUL_EXCEPTION:
>          if ( ctxt.exn_pending )
> -            hvm_inject_hw_exception(ctxt.exn_vector,
> ctxt.exn_error_code);
> +            hvm_inject_trap(&ctxt.trap);
>          /* fall through */
>      default:
>          hvm_emulate_writeback(&ctxt);
> diff --git a/xen/arch/x86/hvm/vmx/realmode.c
> b/xen/arch/x86/hvm/vmx/realmode.c
> index 45066b2..9a6de6c 100644
> --- a/xen/arch/x86/hvm/vmx/realmode.c
> +++ b/xen/arch/x86/hvm/vmx/realmode.c
> @@ -129,27 +129,27 @@ static void realmode_emulate_one(struct
> hvm_emulate_ctxt *hvmemul_ctxt)
>                  gdprintk(XENLOG_ERR, "Exception pending but no
> info.\n");
>                  goto fail;
>              }
> -            hvmemul_ctxt->exn_vector = (uint8_t)intr_info;
> -            hvmemul_ctxt->exn_insn_len = 0;
> +            hvmemul_ctxt->trap.vector = (uint8_t)intr_info;
> +            hvmemul_ctxt->trap.insn_len = 0;
>          }
> 
>          if ( unlikely(curr->domain->debugger_attached) &&
> -             ((hvmemul_ctxt->exn_vector == TRAP_debug) ||
> -              (hvmemul_ctxt->exn_vector == TRAP_int3)) )
> +             ((hvmemul_ctxt->trap.vector == TRAP_debug) ||
> +              (hvmemul_ctxt->trap.vector == TRAP_int3)) )
>          {
>              domain_pause_for_debugger();
>          }
>          else if ( curr->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE )
>          {
>              gdprintk(XENLOG_ERR, "Exception %02x in protected
> mode.\n",
> -                     hvmemul_ctxt->exn_vector);
> +                     hvmemul_ctxt->trap.vector);
>              goto fail;
>          }
>          else
>          {
>              realmode_deliver_exception(
> -                hvmemul_ctxt->exn_vector,
> -                hvmemul_ctxt->exn_insn_len,
> +                hvmemul_ctxt->trap.vector,
> +                hvmemul_ctxt->trap.insn_len,
>                  hvmemul_ctxt);
>          }
>      }
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 84119ed..addaa81 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2510,7 +2510,7 @@ static void vmx_vmexit_ud_intercept(struct
> cpu_user_regs *regs)
>          break;
>      case X86EMUL_EXCEPTION:
>          if ( ctxt.exn_pending )
> -            hvm_inject_hw_exception(ctxt.exn_vector,
> ctxt.exn_error_code);
> +            hvm_inject_trap(&ctxt.trap);
>          /* fall through */
>      default:
>          hvm_emulate_writeback(&ctxt);
> diff --git a/xen/include/asm-x86/hvm/emulate.h
> b/xen/include/asm-x86/hvm/emulate.h
> index efff97e..6cdc57b 100644
> --- a/xen/include/asm-x86/hvm/emulate.h
> +++ b/xen/include/asm-x86/hvm/emulate.h
> @@ -13,6 +13,7 @@
>  #define __ASM_X86_HVM_EMULATE_H__
> 
>  #include <xen/config.h>
> +#include <asm/hvm/hvm.h>
>  #include <asm/x86_emulate.h>
> 
>  struct hvm_emulate_ctxt {
> @@ -28,9 +29,7 @@ struct hvm_emulate_ctxt {
>      unsigned long seg_reg_dirty;
> 
>      bool_t exn_pending;
> -    uint8_t exn_vector;
> -    uint8_t exn_insn_len;
> -    int32_t exn_error_code;
> +    struct hvm_trap trap;
> 
>      uint32_t intr_shadow;
>  };
> --
> 1.7.10.4

Re: [PATCH v2 5/6] x86/hvm: Forced Emulation Prefix for debug builds of Xen

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, September 23, 2014 9:21 AM
> 
> >>> On 23.09.14 at 18:09, <andrew.cooper3@citrix.com> wrote:
> > Analysis of XSAs 105 and 106 show that is possible to force a race condition
> > which causes any arbitrary instruction to be emulated.
> >
> > To aid testing, explicitly introduce the Forced Emulation Prefix for debug
> > builds alone.
> >
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

Re: [PATCH 3/6] x86/hvm: Don't discard the SW/HW event distinction from the emulator

[Boris Ostrovsky]

On 09/23/2014 11:03 AM, Andrew Cooper wrote:
> Injecting emulator software events as hardware exceptions results in a bypass
> of DPL checks.  As the emulator doesn't perform DPL checks itself, guest
> userspace is capable of bypassing DPL checks and injecting arbitrary events.
>
> Propagating software event information from the emulator allows VMX to now
> properly inject software events, including DPL and presence checks, as well
> correct fault/trap frames.
>
> Reported-by: Andrei LUTAS <vlutas@bitdefender.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Tested-by: Andrei LUTAS <vlutas@bitdefender.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Eddie Dong <eddie.dong@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

> ---
>   xen/arch/x86/hvm/emulate.c        |   41 ++++++++++++++++++++++++++++---------
>   xen/arch/x86/hvm/io.c             |    2 +-
>   xen/arch/x86/hvm/svm/svm.c        |    2 +-
>   xen/arch/x86/hvm/vmx/realmode.c   |   14 ++++++-------
>   xen/arch/x86/hvm/vmx/vmx.c        |    2 +-
>   xen/include/asm-x86/hvm/emulate.h |    5 ++---
>   6 files changed, 43 insertions(+), 23 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 5d5d765..7ee146b 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -441,9 +441,10 @@ static int hvmemul_virtual_to_linear(
>   
>       /* This is a singleton operation: fail it with an exception. */
>       hvmemul_ctxt->exn_pending = 1;
> -    hvmemul_ctxt->exn_vector = TRAP_gp_fault;
> -    hvmemul_ctxt->exn_error_code = 0;
> -    hvmemul_ctxt->exn_insn_len = 0;
> +    hvmemul_ctxt->trap.vector = TRAP_gp_fault;
> +    hvmemul_ctxt->trap.type = X86_EVENTTYPE_HW_EXCEPTION;
> +    hvmemul_ctxt->trap.error_code = 0;
> +    hvmemul_ctxt->trap.insn_len = 0;
>       return X86EMUL_EXCEPTION;
>   }
>   
> @@ -1111,9 +1112,10 @@ static int hvmemul_inject_hw_exception(
>           container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>   
>       hvmemul_ctxt->exn_pending = 1;
> -    hvmemul_ctxt->exn_vector = vector;
> -    hvmemul_ctxt->exn_error_code = error_code;
> -    hvmemul_ctxt->exn_insn_len = 0;
> +    hvmemul_ctxt->trap.vector = vector;
> +    hvmemul_ctxt->trap.type = X86_EVENTTYPE_HW_EXCEPTION;
> +    hvmemul_ctxt->trap.error_code = error_code;
> +    hvmemul_ctxt->trap.insn_len = 0;
>   
>       return X86EMUL_OKAY;
>   }
> @@ -1127,10 +1129,29 @@ static int hvmemul_inject_sw_interrupt(
>       struct hvm_emulate_ctxt *hvmemul_ctxt =
>           container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>   
> +    switch ( type )
> +    {
> +    case x86_swint_icebp:
> +        hvmemul_ctxt->trap.type = X86_EVENTTYPE_PRI_SW_EXCEPTION;
> +        break;
> +
> +    case x86_swint_int3:
> +    case x86_swint_into:
> +        hvmemul_ctxt->trap.type = X86_EVENTTYPE_SW_EXCEPTION;
> +        break;
> +
> +    case x86_swint_int:
> +        hvmemul_ctxt->trap.type = X86_EVENTTYPE_SW_INTERRUPT;
> +        break;
> +
> +    default:
> +        return X86EMUL_UNHANDLEABLE;
> +    }
> +
>       hvmemul_ctxt->exn_pending = 1;
> -    hvmemul_ctxt->exn_vector = vector;
> -    hvmemul_ctxt->exn_error_code = -1;
> -    hvmemul_ctxt->exn_insn_len = insn_len;
> +    hvmemul_ctxt->trap.vector = vector;
> +    hvmemul_ctxt->trap.error_code = HVM_DELIVER_NO_ERROR_CODE;
> +    hvmemul_ctxt->trap.insn_len = insn_len;
>   
>       return X86EMUL_OKAY;
>   }
> @@ -1404,7 +1425,7 @@ void hvm_mem_event_emulate_one(bool_t nowrite, unsigned int trapnr,
>           break;
>       case X86EMUL_EXCEPTION:
>           if ( ctx.exn_pending )
> -            hvm_inject_hw_exception(ctx.exn_vector, ctx.exn_error_code);
> +            hvm_inject_trap(&ctx.trap);
>           break;
>       }
>   
> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
> index 9f565d6..e5d5e79 100644
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -113,7 +113,7 @@ int handle_mmio(void)
>           return 0;
>       case X86EMUL_EXCEPTION:
>           if ( ctxt.exn_pending )
> -            hvm_inject_hw_exception(ctxt.exn_vector, ctxt.exn_error_code);
> +            hvm_inject_trap(&ctxt.trap);
>           break;
>       default:
>           break;
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 5d404ce..de982fd 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2080,7 +2080,7 @@ static void svm_vmexit_ud_intercept(struct cpu_user_regs *regs)
>           break;
>       case X86EMUL_EXCEPTION:
>           if ( ctxt.exn_pending )
> -            hvm_inject_hw_exception(ctxt.exn_vector, ctxt.exn_error_code);
> +            hvm_inject_trap(&ctxt.trap);
>           /* fall through */
>       default:
>           hvm_emulate_writeback(&ctxt);
> diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c
> index 45066b2..9a6de6c 100644
> --- a/xen/arch/x86/hvm/vmx/realmode.c
> +++ b/xen/arch/x86/hvm/vmx/realmode.c
> @@ -129,27 +129,27 @@ static void realmode_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt)
>                   gdprintk(XENLOG_ERR, "Exception pending but no info.\n");
>                   goto fail;
>               }
> -            hvmemul_ctxt->exn_vector = (uint8_t)intr_info;
> -            hvmemul_ctxt->exn_insn_len = 0;
> +            hvmemul_ctxt->trap.vector = (uint8_t)intr_info;
> +            hvmemul_ctxt->trap.insn_len = 0;
>           }
>   
>           if ( unlikely(curr->domain->debugger_attached) &&
> -             ((hvmemul_ctxt->exn_vector == TRAP_debug) ||
> -              (hvmemul_ctxt->exn_vector == TRAP_int3)) )
> +             ((hvmemul_ctxt->trap.vector == TRAP_debug) ||
> +              (hvmemul_ctxt->trap.vector == TRAP_int3)) )
>           {
>               domain_pause_for_debugger();
>           }
>           else if ( curr->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE )
>           {
>               gdprintk(XENLOG_ERR, "Exception %02x in protected mode.\n",
> -                     hvmemul_ctxt->exn_vector);
> +                     hvmemul_ctxt->trap.vector);
>               goto fail;
>           }
>           else
>           {
>               realmode_deliver_exception(
> -                hvmemul_ctxt->exn_vector,
> -                hvmemul_ctxt->exn_insn_len,
> +                hvmemul_ctxt->trap.vector,
> +                hvmemul_ctxt->trap.insn_len,
>                   hvmemul_ctxt);
>           }
>       }
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 84119ed..addaa81 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2510,7 +2510,7 @@ static void vmx_vmexit_ud_intercept(struct cpu_user_regs *regs)
>           break;
>       case X86EMUL_EXCEPTION:
>           if ( ctxt.exn_pending )
> -            hvm_inject_hw_exception(ctxt.exn_vector, ctxt.exn_error_code);
> +            hvm_inject_trap(&ctxt.trap);
>           /* fall through */
>       default:
>           hvm_emulate_writeback(&ctxt);
> diff --git a/xen/include/asm-x86/hvm/emulate.h b/xen/include/asm-x86/hvm/emulate.h
> index efff97e..6cdc57b 100644
> --- a/xen/include/asm-x86/hvm/emulate.h
> +++ b/xen/include/asm-x86/hvm/emulate.h
> @@ -13,6 +13,7 @@
>   #define __ASM_X86_HVM_EMULATE_H__
>   
>   #include <xen/config.h>
> +#include <asm/hvm/hvm.h>
>   #include <asm/x86_emulate.h>
>   
>   struct hvm_emulate_ctxt {
> @@ -28,9 +29,7 @@ struct hvm_emulate_ctxt {
>       unsigned long seg_reg_dirty;
>   
>       bool_t exn_pending;
> -    uint8_t exn_vector;
> -    uint8_t exn_insn_len;
> -    int32_t exn_error_code;
> +    struct hvm_trap trap;
>   
>       uint32_t intr_shadow;
>   };

Re: [PATCH 4/6] x86/emulate: Support for emulating software event injection

[Boris Ostrovsky]

On 09/24/2014 10:20 AM, Andrew Cooper wrote:
> On 24/09/14 14:24, Boris Ostrovsky wrote:
>> On 09/24/2014 09:04 AM, Andrew Cooper wrote:
>>> On 24/09/14 14:01, Boris Ostrovsky wrote:
>>>> On 09/23/2014 11:03 AM, Andrew Cooper wrote:
>>>>> AMD SVM requires all software events to have their injection
>>>>> emulated if
>>>>> hardware lacks NextRIP support.  In addition, `icebp` (opcode 0xf1)
>>>>> injection
>>>>> requires emulation in all cases, even with hardware NextRIP support.
>>>>>
>>>>> Emulating full control transfers is overkill for our needs.  All that
>>>>> matters
>>>>> is that guest userspace can't bypass the descriptor DPL check.  Any
>>>>> guest OS
>>>>> which would incur other faults as part of injection is going to end
>>>>> up with a
>>>>> double fault instead, and won't be in a position to care that the
>>>>> faulting eip
>>>>> is wrong.
>>>>>
>>>>> Reported-by: Andrei LUTAS <vlutas@bitdefender.com>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>>>> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>>>>> ---
>>>>>     xen/arch/x86/hvm/emulate.c             |    8 +++
>>>>>     xen/arch/x86/hvm/svm/svm.c             |   57 +++++++++++++--
>>>>>     xen/arch/x86/mm.c                      |    2 +
>>>>>     xen/arch/x86/mm/shadow/common.c        |    1 +
>>>>>     xen/arch/x86/x86_emulate/x86_emulate.c |  122
>>>>> ++++++++++++++++++++++++++++++--
>>>>>     xen/arch/x86/x86_emulate/x86_emulate.h |   10 +++
>>>>>     6 files changed, 191 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>>>>> index 7ee146b..463ccfb 100644
>>>>> --- a/xen/arch/x86/hvm/emulate.c
>>>>> +++ b/xen/arch/x86/hvm/emulate.c
>>>>> @@ -21,6 +21,7 @@
>>>>>     #include <asm/hvm/hvm.h>
>>>>>     #include <asm/hvm/trace.h>
>>>>>     #include <asm/hvm/support.h>
>>>>> +#include <asm/hvm/svm/svm.h>
>>>>>       static void hvmtrace_io_assist(int is_mmio, ioreq_t *p)
>>>>>     {
>>>>> @@ -1328,6 +1329,13 @@ static int _hvm_emulate_one(struct
>>>>> hvm_emulate_ctxt *hvmemul_ctxt,
>>>>>         vio->mmio_retrying = vio->mmio_retry;
>>>>>         vio->mmio_retry = 0;
>>>>>     +    if ( cpu_has_vmx )
>>>>> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_none;
>>>>> +    else if ( cpu_has_svm_nrips )
>>>>> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_icebp;
>>>>> +    else
>>>>> +        hvmemul_ctxt->ctxt.swint_emulate = x86_swint_emulate_all;
>>>>> +
>>>>>         rc = x86_emulate(&hvmemul_ctxt->ctxt, ops);
>>>>>           if ( rc == X86EMUL_OKAY && vio->mmio_retry )
>>>>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>>>>> index de982fd..b6beefc 100644
>>>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>>>> @@ -1177,11 +1177,12 @@ static void svm_inject_trap(struct hvm_trap
>>>>> *trap)
>>>>>         struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
>>>>>         eventinj_t event = vmcb->eventinj;
>>>>>         struct hvm_trap _trap = *trap;
>>>>> +    const struct cpu_user_regs *regs = guest_cpu_user_regs();
>>>>>           switch ( _trap.vector )
>>>>>         {
>>>>>         case TRAP_debug:
>>>>> -        if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
>>>>> +        if ( regs->eflags & X86_EFLAGS_TF )
>>>>>             {
>>>>>                 __restore_debug_registers(vmcb, curr);
>>>>>                 vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | 0x4000);
>>>>> @@ -1209,10 +1210,58 @@ static void svm_inject_trap(struct hvm_trap
>>>>> *trap)
>>>>>           event.bytes = 0;
>>>>>         event.fields.v = 1;
>>>>> -    event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
>>>>>         event.fields.vector = _trap.vector;
>>>>> -    event.fields.ev = (_trap.error_code !=
>>>>> HVM_DELIVER_NO_ERROR_CODE);
>>>>> -    event.fields.errorcode = _trap.error_code;
>>>>> +
>>>>> +    /* Refer to AMD Vol 2: System Programming, 15.20 Event
>>>>> Injection. */
>>>>> +    switch ( _trap.type )
>>>>> +    {
>>>>> +    case X86_EVENTTYPE_SW_INTERRUPT: /* int $n */
>>>>> +        /*
>>>>> +         * Injection type 4 (software interrupt) is only supported
>>>>> with
>>>>> +         * NextRIP support.  Without NextRIP, the emulator will have
>>>>> performed
>>>>> +         * DPL and presence checks for us.
>>>>> +         */
>>>>> +        if ( cpu_has_svm_nrips )
>>>>> +        {
>>>>> +            vmcb->nextrip = regs->eip + _trap.insn_len;
>>>>> +            event.fields.type = X86_EVENTTYPE_SW_INTERRUPT;
>>>>> +        }
>>>>> +        else
>>>>> +            event.fields.type = X86_EVENTTYPE_HW_EXCEPTION;
>>>>> +        break;
>>>>> +
>>>>> +    case X86_EVENTTYPE_PRI_SW_EXCEPTION: /* icebp */
>>>>> +        /*
>>>>> +         * icebp's injection must always be emulated.  Software
>>>>> injection help
>>>>> +         * in x86_emulate has moved eip forward, but NextRIP (if
>>>>> used) still
>>>>> +         * needs setting or execution will resume from 0.
>>>>> +         */
>>>> Can you tell me where eip is updated? I don't see any difference
>>>> between how, for example, int3 is emulated differently from icebp.
>>>>
>>>> -boris
>>> The second hunk in this series, chooses between x86_swint_emulate_icebp
>>> and x86_swint_emulate_all based on NRIP support.
>>>
>>> This cause inject_swint() to either emulate the injection or not.
>> I was asking about why you are calculating nextrip differently (i.e
>> not adding _trap.insn_len) for icebp vs. int3. I read the comment as
>> if it was saying that x86_emulate() has already taken care of that.
>> And I don't understand where that is done.
> With NRIPS support, hardware can deal with injecting the trap and doing
> DPL/presence checks, which means the eip in the exception frame might
> either be a trap or a fault.
>
> regs->eip and vmcb->nextrip bound the trapping instruction, with eip
> pointing at it, and nextrip pointing after it.  These are the two
> choices which get set in the exception frame, after hardware has decided
> whether to send the trap, or fault because of a bad IDT setup.
>
> Without NRIP support, the emulator must move regs->eip on to get the
> trap frame eip pointing at the correct address as it is the only
> available choice for an exception frame.
>
> For ICEBP, there is no hardware support for injection at all (even with
> NRIP available).  The emulator must move regs->eip forward to get the
> correct trap eip for the non-NRIP case, but with NRIP support,
> vmcb->nextrip must be set, or the resulting trap frame contains an eip of 0.
>
> I suppose this looks a little weird because of the intersection of
> x86_swint_emulate_icebp and x86_swint_emulate_all, but it is not correct
> to inject a software event with NRIP support for icebp, as that would
> cause hardware to do a DPL check and fail to set the external bit in a
> #NP error code.

Thanks.

My problem was that I didn't understand what the first 'if' in 
inject_swint() was doing. I first read it as if it was trying to figure 
out which instruction is being emulated.
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Re: [PATCH v2 5/6] x86/hvm: Forced Emulation Prefix for debug builds of Xen

[Boris Ostrovsky]

On 09/23/2014 12:09 PM, Andrew Cooper wrote:
> Analysis of XSAs 105 and 106 show that is possible to force a race condition
> which causes any arbitrary instruction to be emulated.
>
> To aid testing, explicitly introduce the Forced Emulation Prefix for debug
> builds alone.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Eddie Dong <eddie.dong@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>


>
> ---
> v2: (all suggested by Jan)
>   * Use hvm_fetch_from_guest_virt_nofault() in preference to copy_from_guest()
>   * Vastly reduce use of #ifndef NDEBUG
> ---
>   docs/misc/xen-command-line.markdown |   11 +++++++++++
>   xen/arch/x86/hvm/hvm.c              |    5 +++++
>   xen/arch/x86/hvm/svm/svm.c          |   13 +++++++++++++
>   xen/arch/x86/hvm/vmx/vmx.c          |   13 +++++++++++++
>   xen/include/asm-x86/hvm/hvm.h       |    7 +++++++
>   5 files changed, 49 insertions(+)
>
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index af93e17..389701a 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -682,6 +682,17 @@ Bit 11 - MSR operation logging
>   
>   Recognized in debug builds of the hypervisor only.
>   
> +### hvm\_fep
> +> `= <boolean>`
> +
> +> Default: `false`
> +
> +Allow use of the Forced Emulation Prefix in HVM guests, to allow emulation of
> +arbitrary instructions.
> +
> +This option is intended for development purposes, and is only available in
> +debug builds of the hypervisor.
> +
>   ### hvm\_port80
>   > `= <boolean>`
>   
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 5c7e0a4..34f28d0 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -86,6 +86,11 @@ unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
>   static bool_t __initdata opt_hap_enabled = 1;
>   boolean_param("hap", opt_hap_enabled);
>   
> +#ifndef opt_hvm_fep
> +bool_t opt_hvm_fep;
> +boolean_param("hvm_fep", opt_hvm_fep);
> +#endif
> +
>   static int cpu_callback(
>       struct notifier_block *nfb, unsigned long action, void *hcpu)
>   {
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index b6beefc..cda968b 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2118,6 +2118,19 @@ static void svm_vmexit_ud_intercept(struct cpu_user_regs *regs)
>       struct hvm_emulate_ctxt ctxt;
>       int rc;
>   
> +    if ( opt_hvm_fep )
> +    {
> +        char sig[5]; /* ud2; .ascii "xen" */
> +
> +        if ( (hvm_fetch_from_guest_virt_nofault(
> +                  sig, regs->eip, sizeof(sig), 0) == HVMCOPY_okay) &&
> +             (memcmp(sig, "\xf\xbxen", sizeof(sig)) == 0) )
> +        {
> +            regs->eip += sizeof(sig);
> +            regs->eflags &= ~X86_EFLAGS_RF;
> +        }
> +    }
> +
>       hvm_emulate_prepare(&ctxt, regs);
>   
>       rc = hvm_emulate_one(&ctxt);
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index addaa81..7f02ba2 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2499,6 +2499,19 @@ static void vmx_vmexit_ud_intercept(struct cpu_user_regs *regs)
>       struct hvm_emulate_ctxt ctxt;
>       int rc;
>   
> +    if ( opt_hvm_fep )
> +    {
> +        char sig[5]; /* ud2; .ascii "xen" */
> +
> +        if ( (hvm_fetch_from_guest_virt_nofault(
> +                  sig, regs->eip, sizeof(sig), 0) == HVMCOPY_okay) &&
> +             (memcmp(sig, "\xf\xbxen", sizeof(sig)) == 0) )
> +        {
> +            regs->eip += sizeof(sig);
> +            regs->eflags &= ~X86_EFLAGS_RF;
> +        }
> +    }
> +
>       hvm_emulate_prepare(&ctxt, regs);
>   
>       rc = hvm_emulate_one(&ctxt);
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index 3e66276..c0fbc8b 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -517,6 +517,13 @@ bool_t nhvm_vmcx_hap_enabled(struct vcpu *v);
>   /* interrupt */
>   enum hvm_intblk nhvm_interrupt_blocked(struct vcpu *v);
>   
> +#ifndef NDEBUG
> +/* Permit use of the Forced Emulation Prefix in HVM guests */
> +extern bool_t opt_hvm_fep;
> +#else
> +#define opt_hvm_fep 0
> +#endif
> +
>   #endif /* __ASM_X86_HVM_HVM_H__ */
>   
>   /*

Re: [PATCH 6/6] x86/svm: Misc cleanup

[Boris Ostrovsky]

On 09/23/2014 11:03 AM, Andrew Cooper wrote:
> cpu_has_monitor_trap_flag is a VMX control, and has nothing to do with SVM.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>



> ---
>   xen/arch/x86/hvm/svm/svm.c |    2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 4823c80..73f3108 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1188,8 +1188,6 @@ static void svm_inject_trap(struct hvm_trap *trap)
>               __restore_debug_registers(vmcb, curr);
>               vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | 0x4000);
>           }
> -        if ( cpu_has_monitor_trap_flag )
> -            break;
>           /* fall through */
>       case TRAP_int3:
>           if ( curr->domain->debugger_attached )

Re: [PATCH 4/6] x86/emulate: Support for emulating software event injection

[Aravind Gopalakrishnan]

On 9/23/2014 10:03 AM, Andrew Cooper wrote:
> AMD SVM requires all software events to have their injection emulated if
> hardware lacks NextRIP support.  In addition, `icebp` (opcode 0xf1) injection
> requires emulation in all cases, even with hardware NextRIP support.
>
> Emulating full control transfers is overkill for our needs.  All that matters
> is that guest userspace can't bypass the descriptor DPL check.  Any guest OS
> which would incur other faults as part of injection is going to end up with a
> double fault instead, and won't be in a position to care that the faulting eip
> is wrong.
>
> Reported-by: Andrei LUTAS <vlutas@bitdefender.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>

Reviewed-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>