[Xen-devel] [PATCH for-4.7 v2 1/2] xen/x86: Remove the use of vm86_mode()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

Xen, being 64bit only, cannot run PV guests in vm86 mode.  HVM guests however
can be running in vm86 mode, and common codepaths need to be able to cope.

The definition of vm86_mode() in x86_64/regs.h is incorrect, as the predicate
is used by non-PV codepaths.

One buggy use is in hvm/emulate.c.  An HVM guest can be in vm86 mode, and
vm86_mode() sliently omits the check.  Luckily, due to the VEX prefix decoding
logic in x86_emulate(), there is no path to the erronious use with EFLAGS_VM
set.

Another potentially problematic use is in show_guest_stack().  In principle,
show_guest_stack() is common code called for both PV and HVM vcpus.  HVM vcpus
exit early (with no reasonable way of making the code generic), making this
part a PV-only codepath.

Open-code its use in emulate.c, matching the surrounding code.  This causes
all other uses to be in PV-only codepaths, making the code to be logically
dead.  Drop it completely, to avoid future misuse.

Part of resulting cleanup removes vm86attr from read_descriptor(), although
retaining one relevant piece of information; i.e. whether we are reading a
selector for an instruction fetch, or a data fetch.

Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
CC: Jan Beulich <JBeulich@xxxxxxxx>

v2:
 * New
---
 xen/arch/x86/hvm/emulate.c        |  2 +-
 xen/arch/x86/traps.c              | 55 ++++++++++++---------------------------
 xen/include/asm-x86/regs.h        |  2 +-
 xen/include/asm-x86/x86_64/regs.h |  1 -
 4 files changed, 19 insertions(+), 41 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index f5ab5bc..cc0b841 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1559,7 +1559,7 @@ static int hvmemul_get_fpu(
         break;
     case X86EMUL_FPU_ymm:
         if ( !(curr->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE) ||
-             vm86_mode(ctxt->regs) ||
+             (ctxt->regs->eflags & X86_EFLAGS_VM) ||
              !(curr->arch.hvm_vcpu.guest_cr[4] & X86_CR4_OSXSAVE) ||
              !(curr->arch.xcr0 & XSTATE_SSE) ||
              !(curr->arch.xcr0 & XSTATE_YMM) )
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 6fbb1cf..8125c53 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -198,17 +198,8 @@ static void show_guest_stack(struct vcpu *v, const struct 
cpu_user_regs *regs)
         return;
     }
 
-    if ( vm86_mode(regs) )
-    {
-        stack = (unsigned long *)((regs->ss << 4) + (regs->esp & 0xffff));
-        printk("Guest stack trace from ss:sp = %04x:%04x (VM86)\n  ",
-               regs->ss, (uint16_t)(regs->esp & 0xffff));
-    }
-    else
-    {
-        stack = (unsigned long *)regs->esp;
-        printk("Guest stack trace from "__OP"sp=%p:\n  ", stack);
-    }
+    stack = (unsigned long *)regs->esp;
+    printk("Guest stack trace from "__OP"sp=%p:\n  ", stack);
 
     if ( !access_ok(stack, sizeof(*stack)) )
     {
@@ -1683,28 +1674,20 @@ static int read_descriptor(unsigned int sel,
                            unsigned long *base,
                            unsigned long *limit,
                            unsigned int *ar,
-                           unsigned int vm86attr)
+                           bool_t insn_fetch)
 {
     struct desc_struct desc;
 
-    if ( !vm86_mode(regs) )
-    {
-        if ( sel < 4)
-            desc.b = desc.a = 0;
-        else if ( __get_user(desc,
-                        (const struct desc_struct *)(!(sel & 4)
-                                                     ? GDT_VIRT_START(v)
-                                                     : LDT_VIRT_START(v))
-                        + (sel >> 3)) )
-            return 0;
-        if ( !(vm86attr & _SEGMENT_CODE) )
-            desc.b &= ~_SEGMENT_L;
-    }
-    else
-    {
-        desc.a = (sel << 20) | 0xffff;
-        desc.b = vm86attr | (sel >> 12);
-    }
+    if ( sel < 4)
+        desc.b = desc.a = 0;
+    else if ( __get_user(desc,
+                         (const struct desc_struct *)(!(sel & 4)
+                                                      ? GDT_VIRT_START(v)
+                                                      : LDT_VIRT_START(v))
+                         + (sel >> 3)) )
+        return 0;
+    if ( !insn_fetch )
+        desc.b &= ~_SEGMENT_L;
 
     *ar = desc.b & 0x00f0ff00;
     if ( !(desc.b & _SEGMENT_L) )
@@ -1715,7 +1698,7 @@ static int read_descriptor(unsigned int sel,
         if ( desc.b & _SEGMENT_G )
             *limit = ((*limit + 1) << 12) - 1;
 #ifndef NDEBUG
-        if ( !vm86_mode(regs) && (sel > 3) )
+        if ( sel > 3 )
         {
             unsigned int a, l;
             unsigned char valid;
@@ -1805,8 +1788,7 @@ static int guest_io_okay(
     int user_mode = !(v->arch.flags & TF_kernel_mode);
 #define TOGGLE_MODE() if ( user_mode ) toggle_guest_mode(v)
 
-    if ( !vm86_mode(regs) &&
-         (v->arch.pv_vcpu.iopl >= (guest_kernel_mode(v, regs) ? 1 : 3)) )
+    if ( v->arch.pv_vcpu.iopl >= (guest_kernel_mode(v, regs) ? 1 : 3) )
         return 1;
 
     if ( v->arch.pv_vcpu.iobmp_limit > (port + bytes) )
@@ -2094,8 +2076,7 @@ static int emulate_privileged_op(struct cpu_user_regs 
*regs)
     bool_t vpmu_msr;
 
     if ( !read_descriptor(regs->cs, v, regs,
-                          &code_base, &code_limit, &ar,
-                          _SEGMENT_CODE|_SEGMENT_S|_SEGMENT_DPL|_SEGMENT_P) )
+                          &code_base, &code_limit, &ar, 1) )
         goto fail;
     op_default = op_bytes = (ar & (_SEGMENT_L|_SEGMENT_DB)) ? 4 : 2;
     ad_default = ad_bytes = (ar & _SEGMENT_L) ? 8 : op_default;
@@ -2187,9 +2168,7 @@ static int emulate_privileged_op(struct cpu_user_regs 
*regs)
         if ( !(ar & _SEGMENT_L) )
         {
             if ( !read_descriptor(data_sel, v, regs,
-                                  &data_base, &data_limit, &ar,
-                                  _SEGMENT_WR|_SEGMENT_S|_SEGMENT_DPL|
-                                  _SEGMENT_P) )
+                                  &data_base, &data_limit, &ar, 0) )
                 goto fail;
             if ( !(ar & _SEGMENT_S) ||
                  !(ar & _SEGMENT_P) ||
diff --git a/xen/include/asm-x86/regs.h b/xen/include/asm-x86/regs.h
index a6338f0..22efc42 100644
--- a/xen/include/asm-x86/regs.h
+++ b/xen/include/asm-x86/regs.h
@@ -10,7 +10,7 @@
     /* Frame pointer must point into current CPU stack. */                    \
     ASSERT(diff < STACK_SIZE);                                                \
     /* If not a guest frame, it must be a hypervisor frame. */                \
-    ASSERT((diff == 0) || (!vm86_mode(r) && (r->cs == __HYPERVISOR_CS)));     \
+    ASSERT((diff == 0) || (r->cs == __HYPERVISOR_CS));                        \
     /* Return TRUE if it's a guest frame. */                                  \
     (diff == 0);                                                              \
 })
diff --git a/xen/include/asm-x86/x86_64/regs.h 
b/xen/include/asm-x86/x86_64/regs.h
index 3cdc702..171cf9a 100644
--- a/xen/include/asm-x86/x86_64/regs.h
+++ b/xen/include/asm-x86/x86_64/regs.h
@@ -4,7 +4,6 @@
 #include <xen/types.h>
 #include <public/xen.h>
 
-#define vm86_mode(r) (0) /* No VM86 support in long mode. */
 #define ring_0(r)    (((r)->cs & 3) == 0)
 #define ring_1(r)    (((r)->cs & 3) == 1)
 #define ring_2(r)    (((r)->cs & 3) == 2)
-- 
2.1.4


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