[PATCH 0/4] x86: XSA-156 follow-ups

[PATCH 0/4] x86: XSA-156 follow-ups

[Jan Beulich]

Not much interrelation between them other than all having been
noticed while looking into the various aspects of XSA-156 (including
the investigations regarding possible further similar exploit
mechanisms).

1: x86/HVM: don't inject #DB with error code
2: x86/HVM: unify and fix #UD intercept
3: x86/SVM: don't exceed segment limit when fetching instruction bytes
4: x86/traps: honor EXT bit in error codes

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

[PATCH 1/4] x86/HVM: don't inject #DB with error code

[Jan Beulich]

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

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

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4133,7 +4133,7 @@ void hvm_task_switch(
         goto out;
 
     if ( (tss.trace & 1) && !exn_raised )
-        hvm_inject_hw_exception(TRAP_debug, tss_sel & 0xfff8);
+        hvm_inject_hw_exception(TRAP_debug, HVM_DELIVER_NO_ERROR_CODE);
 
     tr.attr.fields.type = 0xb; /* busy 32-bit tss */
     hvm_set_segment_register(v, x86_seg_tr, &tr);




[-- Attachment #2: x86-HVM-task-switch-#DB.patch --]
[-- Type: text/plain, Size: 516 bytes --]

x86/HVM: don't inject #DB with error code

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

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4133,7 +4133,7 @@ void hvm_task_switch(
         goto out;
 
     if ( (tss.trace & 1) && !exn_raised )
-        hvm_inject_hw_exception(TRAP_debug, tss_sel & 0xfff8);
+        hvm_inject_hw_exception(TRAP_debug, HVM_DELIVER_NO_ERROR_CODE);
 
     tr.attr.fields.type = 0xb; /* busy 32-bit tss */
     hvm_set_segment_register(v, x86_seg_tr, &tr);

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

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

[PATCH 2/4] x86/HVM: unify and fix #UD intercept

[Jan Beulich]

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

The SVM and VMX versions really were identical, so instead of fixing
the same issue in two places, fold them at once. The issue fixed is the
missing seg:off -> linear translation of the current code address.

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

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -92,9 +92,12 @@ unsigned long __section(".bss.page_align
 static bool_t __initdata opt_hap_enabled = 1;
 boolean_param("hap", opt_hap_enabled);
 
-#ifndef opt_hvm_fep
-bool_t opt_hvm_fep;
+#ifndef NDEBUG
+/* Permit use of the Forced Emulation Prefix in HVM guests */
+static bool_t opt_hvm_fep;
 boolean_param("hvm_fep", opt_hvm_fep);
+#else
+#define opt_hvm_fep 0
 #endif
 
 /* Xen command-line option to enable altp2m */
@@ -4931,6 +4934,49 @@ gp_fault:
     return X86EMUL_EXCEPTION;
 }
 
+void hvm_ud_intercept(struct cpu_user_regs *regs)
+{
+    struct hvm_emulate_ctxt ctxt;
+
+    if ( opt_hvm_fep )
+    {
+        struct vcpu *cur = current;
+        struct segment_register cs;
+        unsigned long addr;
+        char sig[5]; /* ud2; .ascii "xen" */
+
+        hvm_get_segment_register(cur, x86_seg_cs, &cs);
+        if ( hvm_virtual_to_linear_addr(x86_seg_cs, &cs, regs->eip,
+                                        sizeof(sig), hvm_access_insn_fetch,
+                                        (hvm_long_mode_enabled(cur) &&
+                                         cs.attr.fields.l) ? 64 :
+                                        cs.attr.fields.db ? 32 : 16, &addr) &&
+             (hvm_fetch_from_guest_virt_nofault(sig, addr, 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);
+
+    switch ( hvm_emulate_one(&ctxt) )
+    {
+    case X86EMUL_UNHANDLEABLE:
+        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
+        break;
+    case X86EMUL_EXCEPTION:
+        if ( ctxt.exn_pending )
+            hvm_inject_trap(&ctxt.trap);
+        /* fall through */
+    default:
+        hvm_emulate_writeback(&ctxt);
+        break;
+    }
+}
+
 enum hvm_intblk hvm_interrupt_blocked(struct vcpu *v, struct hvm_intack intack)
 {
     unsigned long intr_shadow;
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2123,43 +2123,6 @@ svm_vmexit_do_vmsave(struct vmcb_struct
     return;
 }
 
-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);
-
-    switch ( rc )
-    {
-    case X86EMUL_UNHANDLEABLE:
-        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
-        break;
-    case X86EMUL_EXCEPTION:
-        if ( ctxt.exn_pending )
-            hvm_inject_trap(&ctxt.trap);
-        /* fall through */
-    default:
-        hvm_emulate_writeback(&ctxt);
-        break;
-    }
-}
-
 static int svm_is_erratum_383(struct cpu_user_regs *regs)
 {
     uint64_t msr_content;
@@ -2491,7 +2454,7 @@ void svm_vmexit_handler(struct cpu_user_
         break;
 
     case VMEXIT_EXCEPTION_UD:
-        svm_vmexit_ud_intercept(regs);
+        hvm_ud_intercept(regs);
         break;
 
     /* Asynchronous event, handled when we STGI'd after the VMEXIT. */
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2747,43 +2747,6 @@ void vmx_enter_realmode(struct cpu_user_
     regs->eflags |= (X86_EFLAGS_VM | X86_EFLAGS_IOPL);
 }
 
-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);
-
-    switch ( rc )
-    {
-    case X86EMUL_UNHANDLEABLE:
-        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
-        break;
-    case X86EMUL_EXCEPTION:
-        if ( ctxt.exn_pending )
-            hvm_inject_trap(&ctxt.trap);
-        /* fall through */
-    default:
-        hvm_emulate_writeback(&ctxt);
-        break;
-    }
-}
-
 static int vmx_handle_eoi_write(void)
 {
     unsigned long exit_qualification;
@@ -3138,7 +3101,7 @@ void vmx_vmexit_handler(struct cpu_user_
             break;
         case TRAP_invalid_op:
             HVMTRACE_1D(TRAP, vector);
-            vmx_vmexit_ud_intercept(regs);
+            hvm_ud_intercept(regs);
             break;
         default:
             HVMTRACE_1D(TRAP, vector);
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -551,13 +551,6 @@ static inline bool_t hvm_altp2m_supporte
     return hvm_funcs.altp2m_supported;
 }
 
-#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
-
 /* updates the current hardware p2m */
 void altp2m_vcpu_update_p2m(struct vcpu *v);
 
--- a/xen/include/asm-x86/hvm/support.h
+++ b/xen/include/asm-x86/hvm/support.h
@@ -131,6 +131,7 @@ int hvm_msr_write_intercept(
     unsigned int msr, uint64_t msr_content, bool_t may_defer);
 int hvm_mov_to_cr(unsigned int cr, unsigned int gpr);
 int hvm_mov_from_cr(unsigned int cr, unsigned int gpr);
+void hvm_ud_intercept(struct cpu_user_regs *);
 
 #endif /* __ASM_X86_HVM_SUPPORT_H__ */
 



[-- Attachment #2: x86-HVM-guest-fetch-virt.patch --]
[-- Type: text/plain, Size: 6396 bytes --]

x86/HVM: unify and fix #UD intercept

The SVM and VMX versions really were identical, so instead of fixing
the same issue in two places, fold them at once. The issue fixed is the
missing seg:off -> linear translation of the current code address.

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

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -92,9 +92,12 @@ unsigned long __section(".bss.page_align
 static bool_t __initdata opt_hap_enabled = 1;
 boolean_param("hap", opt_hap_enabled);
 
-#ifndef opt_hvm_fep
-bool_t opt_hvm_fep;
+#ifndef NDEBUG
+/* Permit use of the Forced Emulation Prefix in HVM guests */
+static bool_t opt_hvm_fep;
 boolean_param("hvm_fep", opt_hvm_fep);
+#else
+#define opt_hvm_fep 0
 #endif
 
 /* Xen command-line option to enable altp2m */
@@ -4931,6 +4934,49 @@ gp_fault:
     return X86EMUL_EXCEPTION;
 }
 
+void hvm_ud_intercept(struct cpu_user_regs *regs)
+{
+    struct hvm_emulate_ctxt ctxt;
+
+    if ( opt_hvm_fep )
+    {
+        struct vcpu *cur = current;
+        struct segment_register cs;
+        unsigned long addr;
+        char sig[5]; /* ud2; .ascii "xen" */
+
+        hvm_get_segment_register(cur, x86_seg_cs, &cs);
+        if ( hvm_virtual_to_linear_addr(x86_seg_cs, &cs, regs->eip,
+                                        sizeof(sig), hvm_access_insn_fetch,
+                                        (hvm_long_mode_enabled(cur) &&
+                                         cs.attr.fields.l) ? 64 :
+                                        cs.attr.fields.db ? 32 : 16, &addr) &&
+             (hvm_fetch_from_guest_virt_nofault(sig, addr, 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);
+
+    switch ( hvm_emulate_one(&ctxt) )
+    {
+    case X86EMUL_UNHANDLEABLE:
+        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
+        break;
+    case X86EMUL_EXCEPTION:
+        if ( ctxt.exn_pending )
+            hvm_inject_trap(&ctxt.trap);
+        /* fall through */
+    default:
+        hvm_emulate_writeback(&ctxt);
+        break;
+    }
+}
+
 enum hvm_intblk hvm_interrupt_blocked(struct vcpu *v, struct hvm_intack intack)
 {
     unsigned long intr_shadow;
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2123,43 +2123,6 @@ svm_vmexit_do_vmsave(struct vmcb_struct
     return;
 }
 
-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);
-
-    switch ( rc )
-    {
-    case X86EMUL_UNHANDLEABLE:
-        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
-        break;
-    case X86EMUL_EXCEPTION:
-        if ( ctxt.exn_pending )
-            hvm_inject_trap(&ctxt.trap);
-        /* fall through */
-    default:
-        hvm_emulate_writeback(&ctxt);
-        break;
-    }
-}
-
 static int svm_is_erratum_383(struct cpu_user_regs *regs)
 {
     uint64_t msr_content;
@@ -2491,7 +2454,7 @@ void svm_vmexit_handler(struct cpu_user_
         break;
 
     case VMEXIT_EXCEPTION_UD:
-        svm_vmexit_ud_intercept(regs);
+        hvm_ud_intercept(regs);
         break;
 
     /* Asynchronous event, handled when we STGI'd after the VMEXIT. */
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2747,43 +2747,6 @@ void vmx_enter_realmode(struct cpu_user_
     regs->eflags |= (X86_EFLAGS_VM | X86_EFLAGS_IOPL);
 }
 
-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);
-
-    switch ( rc )
-    {
-    case X86EMUL_UNHANDLEABLE:
-        hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
-        break;
-    case X86EMUL_EXCEPTION:
-        if ( ctxt.exn_pending )
-            hvm_inject_trap(&ctxt.trap);
-        /* fall through */
-    default:
-        hvm_emulate_writeback(&ctxt);
-        break;
-    }
-}
-
 static int vmx_handle_eoi_write(void)
 {
     unsigned long exit_qualification;
@@ -3138,7 +3101,7 @@ void vmx_vmexit_handler(struct cpu_user_
             break;
         case TRAP_invalid_op:
             HVMTRACE_1D(TRAP, vector);
-            vmx_vmexit_ud_intercept(regs);
+            hvm_ud_intercept(regs);
             break;
         default:
             HVMTRACE_1D(TRAP, vector);
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -551,13 +551,6 @@ static inline bool_t hvm_altp2m_supporte
     return hvm_funcs.altp2m_supported;
 }
 
-#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
-
 /* updates the current hardware p2m */
 void altp2m_vcpu_update_p2m(struct vcpu *v);
 
--- a/xen/include/asm-x86/hvm/support.h
+++ b/xen/include/asm-x86/hvm/support.h
@@ -131,6 +131,7 @@ int hvm_msr_write_intercept(
     unsigned int msr, uint64_t msr_content, bool_t may_defer);
 int hvm_mov_to_cr(unsigned int cr, unsigned int gpr);
 int hvm_mov_from_cr(unsigned int cr, unsigned int gpr);
+void hvm_ud_intercept(struct cpu_user_regs *);
 
 #endif /* __ASM_X86_HVM_SUPPORT_H__ */
 

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

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

[PATCH 3/4] x86/SVM: don't exceed segment limit when fetching instruction bytes

[Jan Beulich]

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

Also consistently use the vmcb local variable whenever possible.

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

--- a/xen/arch/x86/hvm/svm/emulate.c
+++ b/xen/arch/x86/hvm/svm/emulate.c
@@ -47,12 +47,17 @@ static unsigned int is_prefix(u8 opc)
     return 0;
 }
 
-static unsigned long svm_rip2pointer(struct vcpu *v)
+static unsigned long svm_rip2pointer(struct vcpu *v, unsigned long *limit)
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
-    unsigned long p = vmcb->cs.base + guest_cpu_user_regs()->eip;
+    unsigned long p = vmcb->cs.base + vmcb->rip;
+
     if ( !(vmcb->cs.attr.fields.l && hvm_long_mode_enabled(v)) )
+    {
+        *limit = vmcb->cs.limit;
         return (u32)p; /* mask to 32 bits */
+    }
+    *limit = ~0UL;
     return p;
 }
 
@@ -125,11 +130,10 @@ static const u8 *const opc_bytes[INSTR_M
     [INSTR_INVLPGA] = OPCODE_INVLPGA,
 };
 
-static int fetch(struct vcpu *v, u8 *buf, unsigned long addr, int len)
+static bool_t fetch(const struct vmcb_struct *vmcb, u8 *buf,
+                   unsigned long addr, unsigned int len)
 {
-    uint32_t pfec;
-
-    pfec = (vmcb_get_cpl(v->arch.hvm_svm.vmcb) == 3) ? PFEC_user_mode : 0;
+    uint32_t pfec = (vmcb_get_cpl(vmcb) == 3) ? PFEC_user_mode : 0;
 
     switch ( hvm_fetch_from_guest_virt(buf, addr, len, pfec) )
     {
@@ -141,7 +145,7 @@ static int fetch(struct vcpu *v, u8 *buf
     default:
         /* Not OK: fetches from non-RAM pages are not supportable. */
         gdprintk(XENLOG_WARNING, "Bad instruction fetch at %#lx (%#lx)\n",
-                 (unsigned long) guest_cpu_user_regs()->eip, addr);
+                 vmcb->rip, addr);
         hvm_inject_hw_exception(TRAP_gp_fault, 0);
         return 0;
     }
@@ -156,8 +160,8 @@ int __get_instruction_length_from_list(s
     enum instruction_index instr = 0;
     u8 buf[MAX_INST_LEN];
     const u8 *opcode = NULL;
-    unsigned long fetch_addr;
-    unsigned int fetch_len;
+    unsigned long fetch_addr, fetch_limit;
+    unsigned int fetch_len, max_len;
 
     if ( (inst_len = svm_nextrip_insn_length(v)) != 0 )
         return inst_len;
@@ -167,21 +171,24 @@ int __get_instruction_length_from_list(s
 
     /* Fetch up to the next page break; we'll fetch from the next page
      * later if we have to. */
-    fetch_addr = svm_rip2pointer(v);
-    fetch_len = min_t(unsigned int, MAX_INST_LEN,
+    fetch_addr = svm_rip2pointer(v, &fetch_limit);
+    if ( vmcb->rip > fetch_limit )
+        return 0;
+    max_len = min(fetch_limit - vmcb->rip + 1, MAX_INST_LEN + 0UL);
+    fetch_len = min_t(unsigned int, max_len,
                       PAGE_SIZE - (fetch_addr & ~PAGE_MASK));
-    if ( !fetch(v, buf, fetch_addr, fetch_len) )
+    if ( !fetch(vmcb, buf, fetch_addr, fetch_len) )
         return 0;
 
-    while ( (inst_len < MAX_INST_LEN) && is_prefix(buf[inst_len]) )
+    while ( (inst_len < max_len) && is_prefix(buf[inst_len]) )
     {
         inst_len++;
         if ( inst_len >= fetch_len )
         {
-            if ( !fetch(v, buf + fetch_len, fetch_addr + fetch_len,
-                        MAX_INST_LEN - fetch_len) )
+            if ( !fetch(vmcb, buf + fetch_len, fetch_addr + fetch_len,
+                        max_len - fetch_len) )
                 return 0;
-            fetch_len = MAX_INST_LEN;
+            fetch_len = max_len;
         }
     }
 
@@ -190,15 +197,14 @@ int __get_instruction_length_from_list(s
         instr = list[j];
         opcode = opc_bytes[instr];
 
-        for ( i = 0; (i < opcode[0]) && ((inst_len + i) < MAX_INST_LEN); i++ )
+        for ( i = 0; (i < opcode[0]) && ((inst_len + i) < max_len); i++ )
         {
             if ( (inst_len + i) >= fetch_len ) 
-            { 
-                if ( !fetch(v, buf + fetch_len, 
-                            fetch_addr + fetch_len, 
-                            MAX_INST_LEN - fetch_len) ) 
+            {
+                if ( !fetch(vmcb, buf + fetch_len, fetch_addr + fetch_len,
+                            max_len - fetch_len) )
                     return 0;
-                fetch_len = MAX_INST_LEN;
+                fetch_len = max_len;
             }
 
             if ( buf[inst_len+i] != opcode[i+1] )
@@ -216,7 +222,7 @@ int __get_instruction_length_from_list(s
 
  done:
     inst_len += opcode[0];
-    ASSERT(inst_len <= MAX_INST_LEN);
+    ASSERT(inst_len <= max_len);
     return inst_len;
 }
 
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -551,7 +551,7 @@ static inline void vmcb_set_##_name(stru
     vmcb->_##_name = value;                                                 \
     vmcb->cleanbits.fields._cleanbit = 0;                                   \
 }                                                                           \
-static inline _type vmcb_get_##_name(struct vmcb_struct *vmcb)              \
+static inline _type vmcb_get_##_name(const struct vmcb_struct *vmcb)        \
 {                                                                           \
     return vmcb->_##_name;                                                  \
 }



[-- Attachment #2: SVM-get-insn-len-honor-limit.patch --]
[-- Type: text/plain, Size: 5289 bytes --]

x86/SVM: don't exceed segment limit when fetching instruction bytes

Also consistently use the vmcb local variable whenever possible.

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

--- a/xen/arch/x86/hvm/svm/emulate.c
+++ b/xen/arch/x86/hvm/svm/emulate.c
@@ -47,12 +47,17 @@ static unsigned int is_prefix(u8 opc)
     return 0;
 }
 
-static unsigned long svm_rip2pointer(struct vcpu *v)
+static unsigned long svm_rip2pointer(struct vcpu *v, unsigned long *limit)
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
-    unsigned long p = vmcb->cs.base + guest_cpu_user_regs()->eip;
+    unsigned long p = vmcb->cs.base + vmcb->rip;
+
     if ( !(vmcb->cs.attr.fields.l && hvm_long_mode_enabled(v)) )
+    {
+        *limit = vmcb->cs.limit;
         return (u32)p; /* mask to 32 bits */
+    }
+    *limit = ~0UL;
     return p;
 }
 
@@ -125,11 +130,10 @@ static const u8 *const opc_bytes[INSTR_M
     [INSTR_INVLPGA] = OPCODE_INVLPGA,
 };
 
-static int fetch(struct vcpu *v, u8 *buf, unsigned long addr, int len)
+static bool_t fetch(const struct vmcb_struct *vmcb, u8 *buf,
+                   unsigned long addr, unsigned int len)
 {
-    uint32_t pfec;
-
-    pfec = (vmcb_get_cpl(v->arch.hvm_svm.vmcb) == 3) ? PFEC_user_mode : 0;
+    uint32_t pfec = (vmcb_get_cpl(vmcb) == 3) ? PFEC_user_mode : 0;
 
     switch ( hvm_fetch_from_guest_virt(buf, addr, len, pfec) )
     {
@@ -141,7 +145,7 @@ static int fetch(struct vcpu *v, u8 *buf
     default:
         /* Not OK: fetches from non-RAM pages are not supportable. */
         gdprintk(XENLOG_WARNING, "Bad instruction fetch at %#lx (%#lx)\n",
-                 (unsigned long) guest_cpu_user_regs()->eip, addr);
+                 vmcb->rip, addr);
         hvm_inject_hw_exception(TRAP_gp_fault, 0);
         return 0;
     }
@@ -156,8 +160,8 @@ int __get_instruction_length_from_list(s
     enum instruction_index instr = 0;
     u8 buf[MAX_INST_LEN];
     const u8 *opcode = NULL;
-    unsigned long fetch_addr;
-    unsigned int fetch_len;
+    unsigned long fetch_addr, fetch_limit;
+    unsigned int fetch_len, max_len;
 
     if ( (inst_len = svm_nextrip_insn_length(v)) != 0 )
         return inst_len;
@@ -167,21 +171,24 @@ int __get_instruction_length_from_list(s
 
     /* Fetch up to the next page break; we'll fetch from the next page
      * later if we have to. */
-    fetch_addr = svm_rip2pointer(v);
-    fetch_len = min_t(unsigned int, MAX_INST_LEN,
+    fetch_addr = svm_rip2pointer(v, &fetch_limit);
+    if ( vmcb->rip > fetch_limit )
+        return 0;
+    max_len = min(fetch_limit - vmcb->rip + 1, MAX_INST_LEN + 0UL);
+    fetch_len = min_t(unsigned int, max_len,
                       PAGE_SIZE - (fetch_addr & ~PAGE_MASK));
-    if ( !fetch(v, buf, fetch_addr, fetch_len) )
+    if ( !fetch(vmcb, buf, fetch_addr, fetch_len) )
         return 0;
 
-    while ( (inst_len < MAX_INST_LEN) && is_prefix(buf[inst_len]) )
+    while ( (inst_len < max_len) && is_prefix(buf[inst_len]) )
     {
         inst_len++;
         if ( inst_len >= fetch_len )
         {
-            if ( !fetch(v, buf + fetch_len, fetch_addr + fetch_len,
-                        MAX_INST_LEN - fetch_len) )
+            if ( !fetch(vmcb, buf + fetch_len, fetch_addr + fetch_len,
+                        max_len - fetch_len) )
                 return 0;
-            fetch_len = MAX_INST_LEN;
+            fetch_len = max_len;
         }
     }
 
@@ -190,15 +197,14 @@ int __get_instruction_length_from_list(s
         instr = list[j];
         opcode = opc_bytes[instr];
 
-        for ( i = 0; (i < opcode[0]) && ((inst_len + i) < MAX_INST_LEN); i++ )
+        for ( i = 0; (i < opcode[0]) && ((inst_len + i) < max_len); i++ )
         {
             if ( (inst_len + i) >= fetch_len ) 
-            { 
-                if ( !fetch(v, buf + fetch_len, 
-                            fetch_addr + fetch_len, 
-                            MAX_INST_LEN - fetch_len) ) 
+            {
+                if ( !fetch(vmcb, buf + fetch_len, fetch_addr + fetch_len,
+                            max_len - fetch_len) )
                     return 0;
-                fetch_len = MAX_INST_LEN;
+                fetch_len = max_len;
             }
 
             if ( buf[inst_len+i] != opcode[i+1] )
@@ -216,7 +222,7 @@ int __get_instruction_length_from_list(s
 
  done:
     inst_len += opcode[0];
-    ASSERT(inst_len <= MAX_INST_LEN);
+    ASSERT(inst_len <= max_len);
     return inst_len;
 }
 
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -551,7 +551,7 @@ static inline void vmcb_set_##_name(stru
     vmcb->_##_name = value;                                                 \
     vmcb->cleanbits.fields._cleanbit = 0;                                   \
 }                                                                           \
-static inline _type vmcb_get_##_name(struct vmcb_struct *vmcb)              \
+static inline _type vmcb_get_##_name(const struct vmcb_struct *vmcb)        \
 {                                                                           \
     return vmcb->_##_name;                                                  \
 }

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

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

[PATCH 4/4] x86/traps: honor EXT bit in error codes

[Jan Beulich]

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

The specification does not explicitly limit the use of this bit to
exceptions that can have selector style error codes, so to be on the
safe side we should deal with it being set even on error codes formally
documented to be always zero (if they're indeed always zero, the change
is simply dead code in those cases).

Introduce and use (where suitable) X86_XEC_* constants to make the code
easier to read.

To match the placement of the "hardware_trap" lable, the "hardware_gp"
one gets moved slightly too.

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

--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -431,16 +431,16 @@ static enum mce_result mce_action(const
 
 /*
  * Return:
- * -1: if system can't be recovered
+ * 1: if system can't be recovered
  * 0: Continue to next step
  */
-static int mce_urgent_action(const struct cpu_user_regs *regs,
-                              mctelem_cookie_t mctc)
+static bool_t mce_urgent_action(const struct cpu_user_regs *regs,
+                                mctelem_cookie_t mctc)
 {
     uint64_t gstatus;
 
-    if ( mctc == NULL)
-        return 0;
+    if ( regs->error_code & X86_XEC_EXT )
+        return 1;
 
     gstatus = mca_rdmsr(MSR_IA32_MCG_STATUS);
 
@@ -455,9 +455,9 @@ static int mce_urgent_action(const struc
      */
     if ( !(gstatus & MCG_STATUS_RIPV) &&
          (!(gstatus & MCG_STATUS_EIPV) || !guest_mode(regs)) )
-        return -1;
+        return 1;
 
-    return mce_action(regs, mctc) == MCER_RESET ? -1 : 0;
+    return mctc && mce_action(regs, mctc) == MCER_RESET;
 }
 
 /* Shared #MC handler. */
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -618,6 +618,9 @@ static void do_trap(struct cpu_user_regs
     unsigned int trapnr = regs->entry_vector;
     unsigned long fixup;
 
+    if ( use_error_code && (regs->error_code & X86_XEC_EXT) )
+        goto hardware_trap;
+
     DEBUGGER_trap_entry(trapnr, regs);
 
     if ( guest_mode(regs) )
@@ -644,6 +647,7 @@ static void do_trap(struct cpu_user_regs
         return;
     }
 
+ hardware_trap:
     DEBUGGER_trap_fatal(trapnr, regs);
 
     show_execution_state(regs);
@@ -1265,13 +1269,14 @@ static int handle_gdt_ldt_mapping_fault(
             tb = propagate_page_fault(curr->arch.pv_vcpu.ldt_base + offset,
                                       regs->error_code);
             if ( tb )
-                tb->error_code = ((u16)offset & ~3) | 4;
+                tb->error_code = (offset & ~(X86_XEC_EXT | X86_XEC_IDT)) |
+                                 X86_XEC_TI;
         }
     }
     else
     {
         /* GDT fault: handle the fault as #GP(selector). */
-        regs->error_code = (u16)offset & ~7;
+        regs->error_code = offset & ~(X86_XEC_EXT | X86_XEC_IDT | X86_XEC_TI);
         (void)do_general_protection(regs);
     }
 
@@ -3231,7 +3236,7 @@ void do_general_protection(struct cpu_us
 
     DEBUGGER_trap_entry(TRAP_gp_fault, regs);
 
-    if ( regs->error_code & 1 )
+    if ( regs->error_code & X86_XEC_EXT )
         goto hardware_gp;
 
     if ( !guest_mode(regs) )
@@ -3257,7 +3262,7 @@ void do_general_protection(struct cpu_us
      * instruction. The DPL specified by the guest OS for these vectors is NOT
      * CHECKED!!
      */
-    if ( (regs->error_code & 3) == 2 )
+    if ( regs->error_code & X86_XEC_IDT )
     {
         /* This fault must be due to <INT n> instruction. */
         const struct trap_info *ti;
@@ -3299,9 +3304,9 @@ void do_general_protection(struct cpu_us
         return;
     }
 
+ hardware_gp:
     DEBUGGER_trap_fatal(TRAP_gp_fault, regs);
 
- hardware_gp:
     show_execution_state(regs);
     panic("GENERAL PROTECTION FAULT\n[error_code=%04x]", regs->error_code);
 }
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -338,7 +338,7 @@ int80_slow_path:
          * Setup entry vector and error code as if this was a GPF caused by an
          * IDT entry with DPL==0.
          */
-        movl  $((0x80 << 3) | 0x2),UREGS_error_code(%rsp)
+        movl  $((0x80 << 3) | X86_XEC_IDT),UREGS_error_code(%rsp)
         SAVE_PRESERVED
         movl  $TRAP_gp_fault,UREGS_entry_vector(%rsp)
         /* A GPF wouldn't have incremented the instruction pointer. */
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -143,6 +143,11 @@
 #define PFEC_page_paged     (1U<<5)
 #define PFEC_page_shared    (1U<<6)
 
+/* Other exception error code values. */
+#define X86_XEC_EXT         (_AC(1,U) << 0)
+#define X86_XEC_IDT         (_AC(1,U) << 1)
+#define X86_XEC_TI          (_AC(1,U) << 2)
+
 #define XEN_MINIMAL_CR4 (X86_CR4_PGE | X86_CR4_PAE)
 
 #define XEN_SYSCALL_MASK (X86_EFLAGS_AC|X86_EFLAGS_VM|X86_EFLAGS_RF|    \



[-- Attachment #2: x86-trap-honor-EXT.patch --]
[-- Type: text/plain, Size: 4883 bytes --]

x86/traps: honor EXT bit in error codes

The specification does not explicitly limit the use of this bit to
exceptions that can have selector style error codes, so to be on the
safe side we should deal with it being set even on error codes formally
documented to be always zero (if they're indeed always zero, the change
is simply dead code in those cases).

Introduce and use (where suitable) X86_XEC_* constants to make the code
easier to read.

To match the placement of the "hardware_trap" lable, the "hardware_gp"
one gets moved slightly too.

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

--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -431,16 +431,16 @@ static enum mce_result mce_action(const
 
 /*
  * Return:
- * -1: if system can't be recovered
+ * 1: if system can't be recovered
  * 0: Continue to next step
  */
-static int mce_urgent_action(const struct cpu_user_regs *regs,
-                              mctelem_cookie_t mctc)
+static bool_t mce_urgent_action(const struct cpu_user_regs *regs,
+                                mctelem_cookie_t mctc)
 {
     uint64_t gstatus;
 
-    if ( mctc == NULL)
-        return 0;
+    if ( regs->error_code & X86_XEC_EXT )
+        return 1;
 
     gstatus = mca_rdmsr(MSR_IA32_MCG_STATUS);
 
@@ -455,9 +455,9 @@ static int mce_urgent_action(const struc
      */
     if ( !(gstatus & MCG_STATUS_RIPV) &&
          (!(gstatus & MCG_STATUS_EIPV) || !guest_mode(regs)) )
-        return -1;
+        return 1;
 
-    return mce_action(regs, mctc) == MCER_RESET ? -1 : 0;
+    return mctc && mce_action(regs, mctc) == MCER_RESET;
 }
 
 /* Shared #MC handler. */
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -618,6 +618,9 @@ static void do_trap(struct cpu_user_regs
     unsigned int trapnr = regs->entry_vector;
     unsigned long fixup;
 
+    if ( use_error_code && (regs->error_code & X86_XEC_EXT) )
+        goto hardware_trap;
+
     DEBUGGER_trap_entry(trapnr, regs);
 
     if ( guest_mode(regs) )
@@ -644,6 +647,7 @@ static void do_trap(struct cpu_user_regs
         return;
     }
 
+ hardware_trap:
     DEBUGGER_trap_fatal(trapnr, regs);
 
     show_execution_state(regs);
@@ -1265,13 +1269,14 @@ static int handle_gdt_ldt_mapping_fault(
             tb = propagate_page_fault(curr->arch.pv_vcpu.ldt_base + offset,
                                       regs->error_code);
             if ( tb )
-                tb->error_code = ((u16)offset & ~3) | 4;
+                tb->error_code = (offset & ~(X86_XEC_EXT | X86_XEC_IDT)) |
+                                 X86_XEC_TI;
         }
     }
     else
     {
         /* GDT fault: handle the fault as #GP(selector). */
-        regs->error_code = (u16)offset & ~7;
+        regs->error_code = offset & ~(X86_XEC_EXT | X86_XEC_IDT | X86_XEC_TI);
         (void)do_general_protection(regs);
     }
 
@@ -3231,7 +3236,7 @@ void do_general_protection(struct cpu_us
 
     DEBUGGER_trap_entry(TRAP_gp_fault, regs);
 
-    if ( regs->error_code & 1 )
+    if ( regs->error_code & X86_XEC_EXT )
         goto hardware_gp;
 
     if ( !guest_mode(regs) )
@@ -3257,7 +3262,7 @@ void do_general_protection(struct cpu_us
      * instruction. The DPL specified by the guest OS for these vectors is NOT
      * CHECKED!!
      */
-    if ( (regs->error_code & 3) == 2 )
+    if ( regs->error_code & X86_XEC_IDT )
     {
         /* This fault must be due to <INT n> instruction. */
         const struct trap_info *ti;
@@ -3299,9 +3304,9 @@ void do_general_protection(struct cpu_us
         return;
     }
 
+ hardware_gp:
     DEBUGGER_trap_fatal(TRAP_gp_fault, regs);
 
- hardware_gp:
     show_execution_state(regs);
     panic("GENERAL PROTECTION FAULT\n[error_code=%04x]", regs->error_code);
 }
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -338,7 +338,7 @@ int80_slow_path:
          * Setup entry vector and error code as if this was a GPF caused by an
          * IDT entry with DPL==0.
          */
-        movl  $((0x80 << 3) | 0x2),UREGS_error_code(%rsp)
+        movl  $((0x80 << 3) | X86_XEC_IDT),UREGS_error_code(%rsp)
         SAVE_PRESERVED
         movl  $TRAP_gp_fault,UREGS_entry_vector(%rsp)
         /* A GPF wouldn't have incremented the instruction pointer. */
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -143,6 +143,11 @@
 #define PFEC_page_paged     (1U<<5)
 #define PFEC_page_shared    (1U<<6)
 
+/* Other exception error code values. */
+#define X86_XEC_EXT         (_AC(1,U) << 0)
+#define X86_XEC_IDT         (_AC(1,U) << 1)
+#define X86_XEC_TI          (_AC(1,U) << 2)
+
 #define XEN_MINIMAL_CR4 (X86_CR4_PGE | X86_CR4_PAE)
 
 #define XEN_SYSCALL_MASK (X86_EFLAGS_AC|X86_EFLAGS_VM|X86_EFLAGS_RF|    \

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

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

Re: [PATCH 1/4] x86/HVM: don't inject #DB with error code

[Andrew Cooper]

On 10/11/15 17:38, Jan Beulich wrote:
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper@citrix.com>

Re: [PATCH 2/4] x86/HVM: unify and fix #UD intercept

[Andrew Cooper]

On 10/11/15 17:39, Jan Beulich wrote:
> The SVM and VMX versions really were identical, so instead of fixing
> the same issue in two places, fold them at once. The issue fixed is the
> missing seg:off -> linear translation of the current code address.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 4/4] x86/traps: honor EXT bit in error codes

[Andrew Cooper]

On 10/11/15 17:40, Jan Beulich wrote:
> The specification does not explicitly limit the use of this bit to
> exceptions that can have selector style error codes, so to be on the
> safe side we should deal with it being set even on error codes formally
> documented to be always zero (if they're indeed always zero, the change
> is simply dead code in those cases).
>
> Introduce and use (where suitable) X86_XEC_* constants to make the code
> easier to read.
>
> To match the placement of the "hardware_trap" lable, the "hardware_gp"
> one gets moved slightly too.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/cpu/mcheck/mce.c
> +++ b/xen/arch/x86/cpu/mcheck/mce.c
> @@ -431,16 +431,16 @@ static enum mce_result mce_action(const
>  
>  /*
>   * Return:
> - * -1: if system can't be recovered
> + * 1: if system can't be recovered
>   * 0: Continue to next step
>   */
> -static int mce_urgent_action(const struct cpu_user_regs *regs,
> -                              mctelem_cookie_t mctc)
> +static bool_t mce_urgent_action(const struct cpu_user_regs *regs,
> +                                mctelem_cookie_t mctc)
>  {
>      uint64_t gstatus;
>  
> -    if ( mctc == NULL)
> -        return 0;
> +    if ( regs->error_code & X86_XEC_EXT )
> +        return 1;

#MC doesn't push an error code.  0 is pushed by the machine check handler.

>  
>      gstatus = mca_rdmsr(MSR_IA32_MCG_STATUS);
>  
> @@ -455,9 +455,9 @@ static int mce_urgent_action(const struc
>       */
>      if ( !(gstatus & MCG_STATUS_RIPV) &&
>           (!(gstatus & MCG_STATUS_EIPV) || !guest_mode(regs)) )
> -        return -1;
> +        return 1;
>  
> -    return mce_action(regs, mctc) == MCER_RESET ? -1 : 0;
> +    return mctc && mce_action(regs, mctc) == MCER_RESET;
>  }
>  
>  /* Shared #MC handler. */
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -618,6 +618,9 @@ static void do_trap(struct cpu_user_regs
>      unsigned int trapnr = regs->entry_vector;
>      unsigned long fixup;
>  
> +    if ( use_error_code && (regs->error_code & X86_XEC_EXT) )

In the case that use_error_code is 0, regs->error_code will be filled
with 0.

Looking at the code, the parameter is redundant and could be derived
from regs->entry_vector alone.

> +        goto hardware_trap;
> +
>      DEBUGGER_trap_entry(trapnr, regs);
>  
>      if ( guest_mode(regs) )
> @@ -644,6 +647,7 @@ static void do_trap(struct cpu_user_regs
>          return;
>      }
>  
> + hardware_trap:
>      DEBUGGER_trap_fatal(trapnr, regs);
>  
>      show_execution_state(regs);
> @@ -1265,13 +1269,14 @@ static int handle_gdt_ldt_mapping_fault(
>              tb = propagate_page_fault(curr->arch.pv_vcpu.ldt_base + offset,
>                                        regs->error_code);
>              if ( tb )
> -                tb->error_code = ((u16)offset & ~3) | 4;
> +                tb->error_code = (offset & ~(X86_XEC_EXT | X86_XEC_IDT)) |
> +                                 X86_XEC_TI;
>          }
>      }
>      else
>      {
>          /* GDT fault: handle the fault as #GP(selector). */
> -        regs->error_code = (u16)offset & ~7;
> +        regs->error_code = offset & ~(X86_XEC_EXT | X86_XEC_IDT | X86_XEC_TI);
>          (void)do_general_protection(regs);
>      }
>  
> @@ -3231,7 +3236,7 @@ void do_general_protection(struct cpu_us
>  
>      DEBUGGER_trap_entry(TRAP_gp_fault, regs);
>  
> -    if ( regs->error_code & 1 )
> +    if ( regs->error_code & X86_XEC_EXT )
>          goto hardware_gp;
>  
>      if ( !guest_mode(regs) )
> @@ -3257,7 +3262,7 @@ void do_general_protection(struct cpu_us
>       * instruction. The DPL specified by the guest OS for these vectors is NOT
>       * CHECKED!!
>       */
> -    if ( (regs->error_code & 3) == 2 )
> +    if ( regs->error_code & X86_XEC_IDT )

The code here has changed.  It is still technically correct because EXT
breaks earlier, but please do update the comment which currently talks
about the EXT check you have just removed.

~Andrew

>      {
>          /* This fault must be due to <INT n> instruction. */
>          const struct trap_info *ti;
> @@ -3299,9 +3304,9 @@ void do_general_protection(struct cpu_us
>          return;
>      }
>  
> + hardware_gp:
>      DEBUGGER_trap_fatal(TRAP_gp_fault, regs);
>  
> - hardware_gp:
>      show_execution_state(regs);
>      panic("GENERAL PROTECTION FAULT\n[error_code=%04x]", regs->error_code);
>  }
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -338,7 +338,7 @@ int80_slow_path:
>           * Setup entry vector and error code as if this was a GPF caused by an
>           * IDT entry with DPL==0.
>           */
> -        movl  $((0x80 << 3) | 0x2),UREGS_error_code(%rsp)
> +        movl  $((0x80 << 3) | X86_XEC_IDT),UREGS_error_code(%rsp)
>          SAVE_PRESERVED
>          movl  $TRAP_gp_fault,UREGS_entry_vector(%rsp)
>          /* A GPF wouldn't have incremented the instruction pointer. */
> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -143,6 +143,11 @@
>  #define PFEC_page_paged     (1U<<5)
>  #define PFEC_page_shared    (1U<<6)
>  
> +/* Other exception error code values. */
> +#define X86_XEC_EXT         (_AC(1,U) << 0)
> +#define X86_XEC_IDT         (_AC(1,U) << 1)
> +#define X86_XEC_TI          (_AC(1,U) << 2)
> +
>  #define XEN_MINIMAL_CR4 (X86_CR4_PGE | X86_CR4_PAE)
>  
>  #define XEN_SYSCALL_MASK (X86_EFLAGS_AC|X86_EFLAGS_VM|X86_EFLAGS_RF|    \
>
>

Re: [PATCH 3/4] x86/SVM: don't exceed segment limit when fetching instruction bytes

[Boris Ostrovsky]

On 11/10/2015 12:40 PM, Jan Beulich wrote:
> Also consistently use the vmcb local variable whenever possible.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/hvm/svm/emulate.c
> +++ b/xen/arch/x86/hvm/svm/emulate.c

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

[PATCH RFC 5/4] x86: #PF error code adjustments

[Jan Beulich]

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

Add a definition for the (for now unused) protection key related error
code bit, moving our own custom ones out of the way. In the course of
checking the uses of the latter I realized that while right now they
can only get set on their own, callers would better not depend on that
property and check just for the bit rather than matching the entire
value.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
RFC because I noticed that nothing seems to ever set PFEC_page_paged,
so I wonder whether we really need that flag.

It also seems to me that the part of paging_gva_to_gfn() dealing with
the nested case can't be quite right: Neither is there any check after
mode->gva_to_gfn() (namely ignoring INVALID_GFN being returned), nor
does the handling of the two involved error code values seem
reasonable. One of the many reasons why nested HVM can't be expected to
reach "supported" state any time soon, I guess.

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -447,7 +447,7 @@ static int hvmemul_linear_to_phys(
     }
     else if ( (pfn = paging_gva_to_gfn(curr, addr, &pfec)) == INVALID_GFN )
     {
-        if ( pfec == PFEC_page_paged || pfec == PFEC_page_shared )
+        if ( pfec & (PFEC_page_paged | PFEC_page_shared) )
             return X86EMUL_RETRY;
         hvm_inject_page_fault(pfec, addr);
         return X86EMUL_EXCEPTION;
@@ -464,7 +464,7 @@ static int hvmemul_linear_to_phys(
         /* Is it contiguous with the preceding PFNs? If not then we're done. */
         if ( (npfn == INVALID_GFN) || (npfn != (pfn + (reverse ? -i : i))) )
         {
-            if ( pfec == PFEC_page_paged || pfec == PFEC_page_shared )
+            if ( pfec & (PFEC_page_paged | PFEC_page_shared) )
                 return X86EMUL_RETRY;
             done /= bytes_per_rep;
             if ( done == 0 )
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3821,7 +3821,7 @@ static void *hvm_map_entry(unsigned long
      */
     pfec = PFEC_page_present;
     gfn = paging_gva_to_gfn(current, va, &pfec);
-    if ( (pfec == PFEC_page_paged) || (pfec == PFEC_page_shared) )
+    if ( pfec & (PFEC_page_paged | PFEC_page_shared) )
         goto fail;
 
     v = hvm_map_guest_frame_rw(gfn, 0, writable);
@@ -4212,9 +4212,9 @@ static enum hvm_copy_result __hvm_copy(
             gfn = paging_gva_to_gfn(curr, addr, &pfec);
             if ( gfn == INVALID_GFN )
             {
-                if ( pfec == PFEC_page_paged )
+                if ( pfec & PFEC_page_paged )
                     return HVMCOPY_gfn_paged_out;
-                if ( pfec == PFEC_page_shared )
+                if ( pfec & PFEC_page_shared )
                     return HVMCOPY_gfn_shared;
                 if ( flags & HVMCOPY_fault )
                     hvm_inject_page_fault(pfec, addr);
@@ -4327,9 +4327,9 @@ static enum hvm_copy_result __hvm_clear(
         gfn = paging_gva_to_gfn(curr, addr, &pfec);
         if ( gfn == INVALID_GFN )
         {
-            if ( pfec == PFEC_page_paged )
+            if ( pfec & PFEC_page_paged )
                 return HVMCOPY_gfn_paged_out;
-            if ( pfec == PFEC_page_shared )
+            if ( pfec & PFEC_page_shared )
                 return HVMCOPY_gfn_shared;
             return HVMCOPY_bad_gva_to_gfn;
         }
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -140,8 +140,10 @@
 #define PFEC_user_mode      (1U<<2)
 #define PFEC_reserved_bit   (1U<<3)
 #define PFEC_insn_fetch     (1U<<4)
-#define PFEC_page_paged     (1U<<5)
-#define PFEC_page_shared    (1U<<6)
+#define PFEC_prot_key       (1U<<5)
+/* Internally used only flags. */
+#define PFEC_page_paged     (1U<<16)
+#define PFEC_page_shared    (1U<<17)
 
 /* Other exception error code values. */
 #define X86_XEC_EXT         (_AC(1,U) << 0)




[-- Attachment #2: x86-adjust-PFEC.patch --]
[-- Type: text/plain, Size: 3938 bytes --]

x86: #PF error code adjustments

Add a definition for the (for now unused) protection key related error
code bit, moving our own custom ones out of the way. In the course of
checking the uses of the latter I realized that while right now they
can only get set on their own, callers would better not depend on that
property and check just for the bit rather than matching the entire
value.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
RFC because I noticed that nothing seems to ever set PFEC_page_paged,
so I wonder whether we really need that flag.

It also seems to me that the part of paging_gva_to_gfn() dealing with
the nested case can't be quite right: Neither is there any check after
mode->gva_to_gfn() (namely ignoring INVALID_GFN being returned), nor
does the handling of the two involved error code values seem
reasonable. One of the many reasons why nested HVM can't be expected to
reach "supported" state any time soon, I guess.

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -447,7 +447,7 @@ static int hvmemul_linear_to_phys(
     }
     else if ( (pfn = paging_gva_to_gfn(curr, addr, &pfec)) == INVALID_GFN )
     {
-        if ( pfec == PFEC_page_paged || pfec == PFEC_page_shared )
+        if ( pfec & (PFEC_page_paged | PFEC_page_shared) )
             return X86EMUL_RETRY;
         hvm_inject_page_fault(pfec, addr);
         return X86EMUL_EXCEPTION;
@@ -464,7 +464,7 @@ static int hvmemul_linear_to_phys(
         /* Is it contiguous with the preceding PFNs? If not then we're done. */
         if ( (npfn == INVALID_GFN) || (npfn != (pfn + (reverse ? -i : i))) )
         {
-            if ( pfec == PFEC_page_paged || pfec == PFEC_page_shared )
+            if ( pfec & (PFEC_page_paged | PFEC_page_shared) )
                 return X86EMUL_RETRY;
             done /= bytes_per_rep;
             if ( done == 0 )
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3821,7 +3821,7 @@ static void *hvm_map_entry(unsigned long
      */
     pfec = PFEC_page_present;
     gfn = paging_gva_to_gfn(current, va, &pfec);
-    if ( (pfec == PFEC_page_paged) || (pfec == PFEC_page_shared) )
+    if ( pfec & (PFEC_page_paged | PFEC_page_shared) )
         goto fail;
 
     v = hvm_map_guest_frame_rw(gfn, 0, writable);
@@ -4212,9 +4212,9 @@ static enum hvm_copy_result __hvm_copy(
             gfn = paging_gva_to_gfn(curr, addr, &pfec);
             if ( gfn == INVALID_GFN )
             {
-                if ( pfec == PFEC_page_paged )
+                if ( pfec & PFEC_page_paged )
                     return HVMCOPY_gfn_paged_out;
-                if ( pfec == PFEC_page_shared )
+                if ( pfec & PFEC_page_shared )
                     return HVMCOPY_gfn_shared;
                 if ( flags & HVMCOPY_fault )
                     hvm_inject_page_fault(pfec, addr);
@@ -4327,9 +4327,9 @@ static enum hvm_copy_result __hvm_clear(
         gfn = paging_gva_to_gfn(curr, addr, &pfec);
         if ( gfn == INVALID_GFN )
         {
-            if ( pfec == PFEC_page_paged )
+            if ( pfec & PFEC_page_paged )
                 return HVMCOPY_gfn_paged_out;
-            if ( pfec == PFEC_page_shared )
+            if ( pfec & PFEC_page_shared )
                 return HVMCOPY_gfn_shared;
             return HVMCOPY_bad_gva_to_gfn;
         }
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -140,8 +140,10 @@
 #define PFEC_user_mode      (1U<<2)
 #define PFEC_reserved_bit   (1U<<3)
 #define PFEC_insn_fetch     (1U<<4)
-#define PFEC_page_paged     (1U<<5)
-#define PFEC_page_shared    (1U<<6)
+#define PFEC_prot_key       (1U<<5)
+/* Internally used only flags. */
+#define PFEC_page_paged     (1U<<16)
+#define PFEC_page_shared    (1U<<17)
 
 /* Other exception error code values. */
 #define X86_XEC_EXT         (_AC(1,U) << 0)

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

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

[PATCH 6/4] x86/event: correct debug event generation

[Jan Beulich]

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

RIP is not a linear address, and hence should not on its own be subject
to GVA -> GFN translation. Once at it, move all of the (perhaps
expensive) operations in the two functions into their main if()'s body,
and improve the error code passed to the translation function.

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

--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -144,36 +144,59 @@ void hvm_event_guest_request(void)
     }
 }
 
-int hvm_event_int3(unsigned long gla)
+int hvm_event_int3(unsigned long rip)
 {
     int rc = 0;
-    uint32_t pfec = PFEC_page_present;
     struct vcpu *curr = current;
-    vm_event_request_t req = {
-        .reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT,
-        .vcpu_id = curr->vcpu_id,
-        .u.software_breakpoint.gfn = paging_gva_to_gfn(curr, gla, &pfec)
-    };
 
     if ( curr->domain->arch.monitor.software_breakpoint_enabled )
+    {
+        struct segment_register sreg;
+        uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
+        vm_event_request_t req = {
+            .reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT,
+            .vcpu_id = curr->vcpu_id,
+        };
+
+        hvm_get_segment_register(curr, x86_seg_ss, &sreg);
+        if ( sreg.attr.fields.dpl == 3 )
+            pfec |= PFEC_user_mode;
+
+        hvm_get_segment_register(curr, x86_seg_cs, &sreg);
+        req.u.software_breakpoint.gfn = paging_gva_to_gfn(curr,
+                                                          sreg.base + rip,
+                                                          &pfec);
+
         rc = hvm_event_traps(1, &req);
+    }
 
     return rc;
 }
 
-int hvm_event_single_step(unsigned long gla)
+int hvm_event_single_step(unsigned long rip)
 {
     int rc = 0;
-    uint32_t pfec = PFEC_page_present;
     struct vcpu *curr = current;
-    vm_event_request_t req = {
-        .reason = VM_EVENT_REASON_SINGLESTEP,
-        .vcpu_id = curr->vcpu_id,
-        .u.singlestep.gfn = paging_gva_to_gfn(curr, gla, &pfec)
-    };
 
     if ( curr->domain->arch.monitor.singlestep_enabled )
+    {
+        struct segment_register sreg;
+        uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
+        vm_event_request_t req = {
+            .reason = VM_EVENT_REASON_SINGLESTEP,
+            .vcpu_id = curr->vcpu_id,
+        };
+
+        hvm_get_segment_register(curr, x86_seg_ss, &sreg);
+        if ( sreg.attr.fields.dpl == 3 )
+            pfec |= PFEC_user_mode;
+
+        hvm_get_segment_register(curr, x86_seg_cs, &sreg);
+        req.u.singlestep.gfn = paging_gva_to_gfn(curr, sreg.base + rip,
+                                                 &pfec);
+
         rc = hvm_event_traps(1, &req);
+    }
 
     return rc;
 }
--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/asm-x86/hvm/event.h
@@ -28,8 +28,8 @@ bool_t hvm_event_cr(unsigned int index,
     hvm_event_cr(VM_EVENT_X86_##what, new, old)
 void hvm_event_msr(unsigned int msr, uint64_t value);
 /* Called for current VCPU: returns -1 if no listener */
-int hvm_event_int3(unsigned long gla);
-int hvm_event_single_step(unsigned long gla);
+int hvm_event_int3(unsigned long rip);
+int hvm_event_single_step(unsigned long rip);
 void hvm_event_guest_request(void);
 
 #endif /* __ASM_X86_HVM_EVENT_H__ */




[-- Attachment #2: x86-HVM-debug-event-translations.patch --]
[-- Type: text/plain, Size: 3406 bytes --]

x86/event: correct debug event generation

RIP is not a linear address, and hence should not on its own be subject
to GVA -> GFN translation. Once at it, move all of the (perhaps
expensive) operations in the two functions into their main if()'s body,
and improve the error code passed to the translation function.

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

--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -144,36 +144,59 @@ void hvm_event_guest_request(void)
     }
 }
 
-int hvm_event_int3(unsigned long gla)
+int hvm_event_int3(unsigned long rip)
 {
     int rc = 0;
-    uint32_t pfec = PFEC_page_present;
     struct vcpu *curr = current;
-    vm_event_request_t req = {
-        .reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT,
-        .vcpu_id = curr->vcpu_id,
-        .u.software_breakpoint.gfn = paging_gva_to_gfn(curr, gla, &pfec)
-    };
 
     if ( curr->domain->arch.monitor.software_breakpoint_enabled )
+    {
+        struct segment_register sreg;
+        uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
+        vm_event_request_t req = {
+            .reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT,
+            .vcpu_id = curr->vcpu_id,
+        };
+
+        hvm_get_segment_register(curr, x86_seg_ss, &sreg);
+        if ( sreg.attr.fields.dpl == 3 )
+            pfec |= PFEC_user_mode;
+
+        hvm_get_segment_register(curr, x86_seg_cs, &sreg);
+        req.u.software_breakpoint.gfn = paging_gva_to_gfn(curr,
+                                                          sreg.base + rip,
+                                                          &pfec);
+
         rc = hvm_event_traps(1, &req);
+    }
 
     return rc;
 }
 
-int hvm_event_single_step(unsigned long gla)
+int hvm_event_single_step(unsigned long rip)
 {
     int rc = 0;
-    uint32_t pfec = PFEC_page_present;
     struct vcpu *curr = current;
-    vm_event_request_t req = {
-        .reason = VM_EVENT_REASON_SINGLESTEP,
-        .vcpu_id = curr->vcpu_id,
-        .u.singlestep.gfn = paging_gva_to_gfn(curr, gla, &pfec)
-    };
 
     if ( curr->domain->arch.monitor.singlestep_enabled )
+    {
+        struct segment_register sreg;
+        uint32_t pfec = PFEC_page_present | PFEC_insn_fetch;
+        vm_event_request_t req = {
+            .reason = VM_EVENT_REASON_SINGLESTEP,
+            .vcpu_id = curr->vcpu_id,
+        };
+
+        hvm_get_segment_register(curr, x86_seg_ss, &sreg);
+        if ( sreg.attr.fields.dpl == 3 )
+            pfec |= PFEC_user_mode;
+
+        hvm_get_segment_register(curr, x86_seg_cs, &sreg);
+        req.u.singlestep.gfn = paging_gva_to_gfn(curr, sreg.base + rip,
+                                                 &pfec);
+
         rc = hvm_event_traps(1, &req);
+    }
 
     return rc;
 }
--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/asm-x86/hvm/event.h
@@ -28,8 +28,8 @@ bool_t hvm_event_cr(unsigned int index,
     hvm_event_cr(VM_EVENT_X86_##what, new, old)
 void hvm_event_msr(unsigned int msr, uint64_t value);
 /* Called for current VCPU: returns -1 if no listener */
-int hvm_event_int3(unsigned long gla);
-int hvm_event_single_step(unsigned long gla);
+int hvm_event_int3(unsigned long rip);
+int hvm_event_single_step(unsigned long rip);
 void hvm_event_guest_request(void);
 
 #endif /* __ASM_X86_HVM_EVENT_H__ */

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

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

Re: [PATCH 6/4] x86/event: correct debug event generation

[Razvan Cojocaru]

On 11/11/2015 10:39 AM, Jan Beulich wrote:
> RIP is not a linear address, and hence should not on its own be subject
> to GVA -> GFN translation. Once at it, move all of the (perhaps
> expensive) operations in the two functions into their main if()'s body,
> and improve the error code passed to the translation function.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>


Thanks,
Razvan

Re: [PATCH 4/4] x86/traps: honor EXT bit in error codes

[Jan Beulich]

>>> On 10.11.15 at 19:20, <andrew.cooper3@citrix.com> wrote:
> On 10/11/15 17:40, Jan Beulich wrote:
>> @@ -3257,7 +3262,7 @@ void do_general_protection(struct cpu_us
>>       * instruction. The DPL specified by the guest OS for these vectors is NOT
>>       * CHECKED!!
>>       */
>> -    if ( (regs->error_code & 3) == 2 )
>> +    if ( regs->error_code & X86_XEC_IDT )
> 
> The code here has changed.  It is still technically correct because EXT
> breaks earlier, but please do update the comment which currently talks
> about the EXT check you have just removed.

It talks about it, yes, but not in a way making it incompatible with
the code after the change. Anyway, I inserted "which got already
checked above".

Jan

[PATCH v2 4/4] x86/traps: honor EXT bit in error codes

[Jan Beulich]

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

The specification does not explicitly limit the use of this bit to
exceptions that can have selector style error codes, so to be on the
safe side we should deal with it being set even on error codes formally
documented to be always zero (if they're indeed always zero, the change
is simply dead code in those cases).

Introduce and use (where suitable) X86_XEC_* constants to make the code
easier to read.

To match the placement of the "hardware_trap" label, the "hardware_gp"
one gets moved slightly too.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Drop #MC related changes. Simplify initial condition added to
    do_trap(). Adjust a comment in do_general_protection().

--- unstable.orig/xen/arch/x86/traps.c	2015-10-13 12:52:27.000000000 +0200
+++ unstable/xen/arch/x86/traps.c	2015-11-11 09:51:34.000000000 +0100
@@ -618,6 +618,9 @@ static void do_trap(struct cpu_user_regs
     unsigned int trapnr = regs->entry_vector;
     unsigned long fixup;
 
+    if ( regs->error_code & X86_XEC_EXT )
+        goto hardware_trap;
+
     DEBUGGER_trap_entry(trapnr, regs);
 
     if ( guest_mode(regs) )
@@ -644,6 +647,7 @@ static void do_trap(struct cpu_user_regs
         return;
     }
 
+ hardware_trap:
     DEBUGGER_trap_fatal(trapnr, regs);
 
     show_execution_state(regs);
@@ -1265,13 +1269,14 @@ static int handle_gdt_ldt_mapping_fault(
             tb = propagate_page_fault(curr->arch.pv_vcpu.ldt_base + offset,
                                       regs->error_code);
             if ( tb )
-                tb->error_code = ((u16)offset & ~3) | 4;
+                tb->error_code = (offset & ~(X86_XEC_EXT | X86_XEC_IDT)) |
+                                 X86_XEC_TI;
         }
     }
     else
     {
         /* GDT fault: handle the fault as #GP(selector). */
-        regs->error_code = (u16)offset & ~7;
+        regs->error_code = offset & ~(X86_XEC_EXT | X86_XEC_IDT | X86_XEC_TI);
         (void)do_general_protection(regs);
     }
 
@@ -3231,7 +3236,7 @@ void do_general_protection(struct cpu_us
 
     DEBUGGER_trap_entry(TRAP_gp_fault, regs);
 
-    if ( regs->error_code & 1 )
+    if ( regs->error_code & X86_XEC_EXT )
         goto hardware_gp;
 
     if ( !guest_mode(regs) )
@@ -3250,14 +3255,15 @@ void do_general_protection(struct cpu_us
      * 
      * Instead, a GPF occurs with the faulting IDT vector in the error code.
      * Bit 1 is set to indicate that an IDT entry caused the fault. Bit 0 is 
-     * clear to indicate that it's a software fault, not hardware.
+     * clear (which got already checked above) to indicate that it's a software
+     * fault, not a hardware one.
      * 
      * NOTE: Vectors 3 and 4 are dealt with from their own handler. This is
      * okay because they can only be triggered by an explicit DPL-checked
      * instruction. The DPL specified by the guest OS for these vectors is NOT
      * CHECKED!!
      */
-    if ( (regs->error_code & 3) == 2 )
+    if ( regs->error_code & X86_XEC_IDT )
     {
         /* This fault must be due to <INT n> instruction. */
         const struct trap_info *ti;
@@ -3299,9 +3305,9 @@ void do_general_protection(struct cpu_us
         return;
     }
 
+ hardware_gp:
     DEBUGGER_trap_fatal(TRAP_gp_fault, regs);
 
- hardware_gp:
     show_execution_state(regs);
     panic("GENERAL PROTECTION FAULT\n[error_code=%04x]", regs->error_code);
 }
--- unstable.orig/xen/arch/x86/x86_64/entry.S	2015-11-11 09:45:03.000000000 +0100
+++ unstable/xen/arch/x86/x86_64/entry.S	2015-11-04 09:18:27.000000000 +0100
@@ -338,7 +338,7 @@ int80_slow_path:
          * Setup entry vector and error code as if this was a GPF caused by an
          * IDT entry with DPL==0.
          */
-        movl  $((0x80 << 3) | 0x2),UREGS_error_code(%rsp)
+        movl  $((0x80 << 3) | X86_XEC_IDT),UREGS_error_code(%rsp)
         SAVE_PRESERVED
         movl  $TRAP_gp_fault,UREGS_entry_vector(%rsp)
         /* A GPF wouldn't have incremented the instruction pointer. */
--- unstable.orig/xen/include/asm-x86/processor.h	2015-11-11 09:45:03.000000000 +0100
+++ unstable/xen/include/asm-x86/processor.h	2015-11-04 09:21:34.000000000 +0100
@@ -143,6 +143,11 @@
 #define PFEC_page_paged     (1U<<5)
 #define PFEC_page_shared    (1U<<6)
 
+/* Other exception error code values. */
+#define X86_XEC_EXT         (_AC(1,U) << 0)
+#define X86_XEC_IDT         (_AC(1,U) << 1)
+#define X86_XEC_TI          (_AC(1,U) << 2)
+
 #define XEN_MINIMAL_CR4 (X86_CR4_PGE | X86_CR4_PAE)
 
 #define XEN_SYSCALL_MASK (X86_EFLAGS_AC|X86_EFLAGS_VM|X86_EFLAGS_RF|    \



[-- Attachment #2: x86-trap-honor-EXT.patch --]
[-- Type: text/plain, Size: 4705 bytes --]

x86/traps: honor EXT bit in error codes

The specification does not explicitly limit the use of this bit to
exceptions that can have selector style error codes, so to be on the
safe side we should deal with it being set even on error codes formally
documented to be always zero (if they're indeed always zero, the change
is simply dead code in those cases).

Introduce and use (where suitable) X86_XEC_* constants to make the code
easier to read.

To match the placement of the "hardware_trap" label, the "hardware_gp"
one gets moved slightly too.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Drop #MC related changes. Simplify initial condition added to
    do_trap(). Adjust a comment in do_general_protection().

--- unstable.orig/xen/arch/x86/traps.c	2015-10-13 12:52:27.000000000 +0200
+++ unstable/xen/arch/x86/traps.c	2015-11-11 09:51:34.000000000 +0100
@@ -618,6 +618,9 @@ static void do_trap(struct cpu_user_regs
     unsigned int trapnr = regs->entry_vector;
     unsigned long fixup;
 
+    if ( regs->error_code & X86_XEC_EXT )
+        goto hardware_trap;
+
     DEBUGGER_trap_entry(trapnr, regs);
 
     if ( guest_mode(regs) )
@@ -644,6 +647,7 @@ static void do_trap(struct cpu_user_regs
         return;
     }
 
+ hardware_trap:
     DEBUGGER_trap_fatal(trapnr, regs);
 
     show_execution_state(regs);
@@ -1265,13 +1269,14 @@ static int handle_gdt_ldt_mapping_fault(
             tb = propagate_page_fault(curr->arch.pv_vcpu.ldt_base + offset,
                                       regs->error_code);
             if ( tb )
-                tb->error_code = ((u16)offset & ~3) | 4;
+                tb->error_code = (offset & ~(X86_XEC_EXT | X86_XEC_IDT)) |
+                                 X86_XEC_TI;
         }
     }
     else
     {
         /* GDT fault: handle the fault as #GP(selector). */
-        regs->error_code = (u16)offset & ~7;
+        regs->error_code = offset & ~(X86_XEC_EXT | X86_XEC_IDT | X86_XEC_TI);
         (void)do_general_protection(regs);
     }
 
@@ -3231,7 +3236,7 @@ void do_general_protection(struct cpu_us
 
     DEBUGGER_trap_entry(TRAP_gp_fault, regs);
 
-    if ( regs->error_code & 1 )
+    if ( regs->error_code & X86_XEC_EXT )
         goto hardware_gp;
 
     if ( !guest_mode(regs) )
@@ -3250,14 +3255,15 @@ void do_general_protection(struct cpu_us
      * 
      * Instead, a GPF occurs with the faulting IDT vector in the error code.
      * Bit 1 is set to indicate that an IDT entry caused the fault. Bit 0 is 
-     * clear to indicate that it's a software fault, not hardware.
+     * clear (which got already checked above) to indicate that it's a software
+     * fault, not a hardware one.
      * 
      * NOTE: Vectors 3 and 4 are dealt with from their own handler. This is
      * okay because they can only be triggered by an explicit DPL-checked
      * instruction. The DPL specified by the guest OS for these vectors is NOT
      * CHECKED!!
      */
-    if ( (regs->error_code & 3) == 2 )
+    if ( regs->error_code & X86_XEC_IDT )
     {
         /* This fault must be due to <INT n> instruction. */
         const struct trap_info *ti;
@@ -3299,9 +3305,9 @@ void do_general_protection(struct cpu_us
         return;
     }
 
+ hardware_gp:
     DEBUGGER_trap_fatal(TRAP_gp_fault, regs);
 
- hardware_gp:
     show_execution_state(regs);
     panic("GENERAL PROTECTION FAULT\n[error_code=%04x]", regs->error_code);
 }
--- unstable.orig/xen/arch/x86/x86_64/entry.S	2015-11-11 09:45:03.000000000 +0100
+++ unstable/xen/arch/x86/x86_64/entry.S	2015-11-04 09:18:27.000000000 +0100
@@ -338,7 +338,7 @@ int80_slow_path:
          * Setup entry vector and error code as if this was a GPF caused by an
          * IDT entry with DPL==0.
          */
-        movl  $((0x80 << 3) | 0x2),UREGS_error_code(%rsp)
+        movl  $((0x80 << 3) | X86_XEC_IDT),UREGS_error_code(%rsp)
         SAVE_PRESERVED
         movl  $TRAP_gp_fault,UREGS_entry_vector(%rsp)
         /* A GPF wouldn't have incremented the instruction pointer. */
--- unstable.orig/xen/include/asm-x86/processor.h	2015-11-11 09:45:03.000000000 +0100
+++ unstable/xen/include/asm-x86/processor.h	2015-11-04 09:21:34.000000000 +0100
@@ -143,6 +143,11 @@
 #define PFEC_page_paged     (1U<<5)
 #define PFEC_page_shared    (1U<<6)
 
+/* Other exception error code values. */
+#define X86_XEC_EXT         (_AC(1,U) << 0)
+#define X86_XEC_IDT         (_AC(1,U) << 1)
+#define X86_XEC_TI          (_AC(1,U) << 2)
+
 #define XEN_MINIMAL_CR4 (X86_CR4_PGE | X86_CR4_PAE)
 
 #define XEN_SYSCALL_MASK (X86_EFLAGS_AC|X86_EFLAGS_VM|X86_EFLAGS_RF|    \

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

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

Re: [PATCH v2 4/4] x86/traps: honor EXT bit in error codes

[Andrew Cooper]

On 11/11/15 09:23, Jan Beulich wrote:
> The specification does not explicitly limit the use of this bit to
> exceptions that can have selector style error codes, so to be on the
> safe side we should deal with it being set even on error codes formally
> documented to be always zero (if they're indeed always zero, the change
> is simply dead code in those cases).
>
> Introduce and use (where suitable) X86_XEC_* constants to make the code
> easier to read.
>
> To match the placement of the "hardware_trap" label, the "hardware_gp"
> one gets moved slightly too.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 3/4] x86/SVM: don't exceed segment limit when fetching instruction bytes

[Andrew Cooper]

On 10/11/15 17:40, Jan Beulich wrote:
> Also consistently use the vmcb local variable whenever possible.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> with one trivial fix.

>
> --- a/xen/arch/x86/hvm/svm/emulate.c
> +++ b/xen/arch/x86/hvm/svm/emulate.c
> @@ -47,12 +47,17 @@ static unsigned int is_prefix(u8 opc)
>      return 0;
>  }
>  
> -static unsigned long svm_rip2pointer(struct vcpu *v)
> +static unsigned long svm_rip2pointer(struct vcpu *v, unsigned long *limit)
>  {
>      struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> -    unsigned long p = vmcb->cs.base + guest_cpu_user_regs()->eip;
> +    unsigned long p = vmcb->cs.base + vmcb->rip;
> +
>      if ( !(vmcb->cs.attr.fields.l && hvm_long_mode_enabled(v)) )
> +    {
> +        *limit = vmcb->cs.limit;
>          return (u32)p; /* mask to 32 bits */
> +    }
> +    *limit = ~0UL;
>      return p;
>  }
>  
> @@ -125,11 +130,10 @@ static const u8 *const opc_bytes[INSTR_M
>      [INSTR_INVLPGA] = OPCODE_INVLPGA,
>  };
>  
> -static int fetch(struct vcpu *v, u8 *buf, unsigned long addr, int len)
> +static bool_t fetch(const struct vmcb_struct *vmcb, u8 *buf,
> +                   unsigned long addr, unsigned int len)

(mis)indentation of addr

~Andrew

Re: [PATCH 6/4] x86/event: correct debug event generation

[Andrew Cooper]

On 11/11/15 08:39, Jan Beulich wrote:
> RIP is not a linear address, and hence should not on its own be subject
> to GVA -> GFN translation. Once at it, move all of the (perhaps
> expensive) operations in the two functions into their main if()'s body,
> and improve the error code passed to the translation function.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH RFC 5/4] x86: #PF error code adjustments

[Andrew Cooper]

On 11/11/15 08:33, Jan Beulich wrote:
> Add a definition for the (for now unused) protection key related error
> code bit, moving our own custom ones out of the way. In the course of
> checking the uses of the latter I realized that while right now they
> can only get set on their own, callers would better not depend on that
> property and check just for the bit rather than matching the entire
> value.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

For the code presented, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

> ---
> RFC because I noticed that nothing seems to ever set PFEC_page_paged,
> so I wonder whether we really need that flag.

It is set in hap_p2m_ga_to_gfn() for frames with types of P2M_PAGING_TYPES

Did you miss this, or wish to imply that it is actually dead code?

>
> It also seems to me that the part of paging_gva_to_gfn() dealing with
> the nested case can't be quite right: Neither is there any check after
> mode->gva_to_gfn() (namely ignoring INVALID_GFN being returned), nor
> does the handling of the two involved error code values seem
> reasonable. One of the many reasons why nested HVM can't be expected to
> reach "supported" state any time soon, I guess.

I concur.  Yet another item on the "nested" todo list.

Re: [PATCH RFC 5/4] x86: #PF error code adjustments

[Jan Beulich]

>>> On 11.11.15 at 17:30, <andrew.cooper3@citrix.com> wrote:
> On 11/11/15 08:33, Jan Beulich wrote:
>> RFC because I noticed that nothing seems to ever set PFEC_page_paged,
>> so I wonder whether we really need that flag.
> 
> It is set in hap_p2m_ga_to_gfn() for frames with types of P2M_PAGING_TYPES
> 
> Did you miss this, or wish to imply that it is actually dead code?

I missed it, apparently due to a bad grep pattern I used in an attempt
to see paged and shared at once, but not present.

Jan

Re: [PATCH 1/4] x86/HVM: don't inject #DB with error code

[Tian, Kevin]

> From: Jan Beulich
> Sent: Wednesday, November 11, 2015 1:38 AM
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

A simple description why error code is not required is preferred here.

> 
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4133,7 +4133,7 @@ void hvm_task_switch(
>          goto out;
> 
>      if ( (tss.trace & 1) && !exn_raised )
> -        hvm_inject_hw_exception(TRAP_debug, tss_sel & 0xfff8);
> +        hvm_inject_hw_exception(TRAP_debug, HVM_DELIVER_NO_ERROR_CODE);
> 
>      tr.attr.fields.type = 0xb; /* busy 32-bit tss */
>      hvm_set_segment_register(v, x86_seg_tr, &tr);
> 
> 

Re: [PATCH 2/4] x86/HVM: unify and fix #UD intercept

[Tian, Kevin]

> From: Jan Beulich
> Sent: Wednesday, November 11, 2015 1:39 AM
> 
> The SVM and VMX versions really were identical, so instead of fixing
> the same issue in two places, fold them at once. The issue fixed is the
> missing seg:off -> linear translation of the current code address.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Re: [PATCH 1/4] x86/HVM: don't inject #DB with error code

[Jan Beulich]

>>> On 03.12.15 at 08:46, <kevin.tian@intel.com> wrote:
>>  From: Jan Beulich
>> Sent: Wednesday, November 11, 2015 1:38 AM
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> A simple description why error code is not required is preferred here.

Well, anyone knowing what #DB means should also know that an
error code is not only not required, but wrong to be supplied here.
But this is moot anyway since the patch has gone in already.

Jan

Re: [PATCH 1/4] x86/HVM: don't inject #DB with error code

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, December 03, 2015 4:15 PM
> 
> >>> On 03.12.15 at 08:46, <kevin.tian@intel.com> wrote:
> >>  From: Jan Beulich
> >> Sent: Wednesday, November 11, 2015 1:38 AM
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >
> > A simple description why error code is not required is preferred here.
> 
> Well, anyone knowing what #DB means should also know that an
> error code is not only not required, but wrong to be supplied here.
> But this is moot anyway since the patch has gone in already.
> 

bad on me as it doesn't occur in my mind w/o checking spec. :-) 

btw are you still requiring Intel review for whole this patch set? still
looking at 4/4 now...

Thanks
Kevin

Re: [PATCH v2 4/4] x86/traps: honor EXT bit in error codes

[Tian, Kevin]

> From: Jan Beulich
> Sent: Wednesday, November 11, 2015 5:23 PM
> 
> The specification does not explicitly limit the use of this bit to
> exceptions that can have selector style error codes, so to be on the
> safe side we should deal with it being set even on error codes formally
> documented to be always zero (if they're indeed always zero, the change
> is simply dead code in those cases).
> 
> Introduce and use (where suitable) X86_XEC_* constants to make the code
> easier to read.
> 
> To match the placement of the "hardware_trap" label, the "hardware_gp"
> one gets moved slightly too.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Drop #MC related changes. Simplify initial condition added to
>     do_trap(). Adjust a comment in do_general_protection().
> 

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

Re: [PATCH 1/4] x86/HVM: don't inject #DB with error code

[Jan Beulich]

>>> On 03.12.15 at 09:38, <kevin.tian@intel.com> wrote:
> btw are you still requiring Intel review for whole this patch set? still
> looking at 4/4 now...

Iirc the whole series went in meanwhile. I.e. I wouldn't call it
"require", but "nice to have" (ensuring you don't spot an issue
with what already got committed).

Jan