[HVM] Inject #PF when mmio instruction fetch fails

instead of crashing the guest.  This can happen if one vcpu pages out
another vcpu's kernel text page while the other is performing an mmio op.
Signed-off-by: Tim Deegan <Tim.Deegan@xensource.com>

diff --git a/xen/arch/x86/hvm/instrlen.c b/xen/arch/x86/hvm/instrlen.c
index 0255d1f7c93d981bf9afb0fa582f00f9cd9027f4..d258e744dc60ca058922893658148ec259384247 100644 (file)
--- a/xen/arch/x86/hvm/instrlen.c
+++ b/xen/arch/x86/hvm/instrlen.c
@@ -9,14 +9,6 @@
  * x86_emulate.c.  Used for MMIO.
  */
 
-/*
- * TODO: The way in which we use hvm_instruction_length is very inefficient as
- * it now stands. It will be worthwhile to return the actual instruction buffer
- * along with the instruction length since one of the reasons we are getting
- * the instruction length is to know how many instruction bytes we need to
- * fetch.
- */
-
 #include <xen/config.h>
 #include <xen/sched.h>
 #include <xen/mm.h>
@@ -194,31 +186,51 @@ static uint8_t twobyte_table[256] = {
 /* 
  * insn_fetch - fetch the next byte from instruction stream
  */
-#define insn_fetch()                                                    \
-({ uint8_t _x;                                                          \
-   if ( length >= 15 )                                                  \
-       return -1;                                                       \
-   if ( inst_copy_from_guest(&_x, pc, 1) != 1 ) {                       \
-       gdprintk(XENLOG_WARNING,                                         \
-                "Cannot read from address %lx (eip %lx, mode %d)\n",    \
-                pc, org_pc, address_bytes);                             \
-       return -1;                                                       \
-   }                                                                    \
-   pc += 1;                                                             \
-   length += 1;                                                         \
-   _x;                                                                  \
+#define insn_fetch()                                                      \
+({ uint8_t _x;                                                            \
+   if ( length >= 15 )                                                    \
+       return -1;                                                         \
+   if ( inst_copy_from_guest(&_x, pc, 1) != 1 ) {                         \
+       unsigned long err;                                                 \
+       struct segment_register cs;                                        \
+       gdprintk(XENLOG_WARNING,                                           \
+                "Cannot read from address %lx (eip %lx, mode %d)\n",      \
+                pc, org_pc, address_bytes);                               \
+       err = 0; /* Must be not-present: we don't enforce reserved bits */ \
+       if ( hvm_nx_enabled(current) )                                     \
+           err |= PFEC_insn_fetch;                                        \
+       hvm_get_segment_register(current, x86_seg_cs, &cs);                \
+       if ( cs.attr.fields.dpl != 0 )                                     \
+           err |= PFEC_user_mode;                                         \
+       hvm_inject_exception(TRAP_page_fault, err, pc);                    \
+       return -1;                                                         \
+   }                                                                      \
+   if ( buf )                                                             \
+       buf[length] = _x;                                                  \
+   length += 1;                                                           \
+   pc += 1;                                                               \
+   _x;                                                                    \
 })
 
+#define insn_skip(_n) do {                      \
+    int _i;                                     \
+    for ( _i = 0; _i < (_n); _i++) {            \
+        (void) insn_fetch();                    \
+    }                                           \
+} while (0)
+
 /**
- * hvm_instruction_length - returns the current instructions length
+ * hvm_instruction_fetch - read the current instruction and return its length
  *
  * @org_pc: guest instruction pointer
- * @mode: guest operating mode
+ * @address_bytes: guest address width
+ * @buf: (optional) buffer to load actual instruction bytes into
  *
- * EXTERNAL this routine calculates the length of the current instruction
- * pointed to by org_pc.  The guest state is _not_ changed by this routine.
+ * Doesn't increment the guest's instruction pointer, but may
+ * issue faults to the guest.  Returns -1 on failure.
  */
-int hvm_instruction_length(unsigned long org_pc, int address_bytes)
+int hvm_instruction_fetch(unsigned long org_pc, int address_bytes,
+                          unsigned char *buf)
 {
     uint8_t b, d, twobyte = 0, rex_prefix = 0, modrm_reg = 0;
     unsigned int op_default, op_bytes, ad_default, ad_bytes, tmp;
@@ -317,18 +329,13 @@ done_prefixes:
             {
             case 0:
                 if ( modrm_rm == 6 ) 
-                {
-                    length += 2;
-                    pc += 2; /* skip disp16 */
-                }
+                    insn_skip(2); /* skip disp16 */
                 break;
             case 1:
-                length += 1;
-                pc += 1; /* skip disp8 */
+                insn_skip(1); /* skip disp8 */
                 break;
             case 2:
-                length += 2;
-                pc += 2; /* skip disp16 */
+                insn_skip(2); /* skip disp16 */
                 break;
             }
         }
@@ -340,33 +347,19 @@ done_prefixes:
             case 0:
                 if ( (modrm_rm == 4) && 
                      ((insn_fetch() & 7) == 5) )
-                {
-                    length += 4;
-                    pc += 4; /* skip disp32 specified by SIB.base */
-                }
+                    insn_skip(4); /* skip disp32 specified by SIB.base */
                 else if ( modrm_rm == 5 )
-                {
-                    length += 4;
-                    pc += 4; /* skip disp32 */
-                }
+                    insn_skip(4); /* skip disp32 */
                 break;
             case 1:
                 if ( modrm_rm == 4 )
-                {
-                    length += 1;
-                    pc += 1;
-                }
-                length += 1;
-                pc += 1; /* skip disp8 */
+                    insn_skip(1);
+                insn_skip(1); /* skip disp8 */
                 break;
             case 2:
                 if ( modrm_rm == 4 )
-                {
-                    length += 1;
-                    pc += 1;
-                }
-                length += 4;
-                pc += 4; /* skip disp32 */
+                    insn_skip(1);
+                insn_skip(4); /* skip disp32 */
                 break;
             }
         }
@@ -387,12 +380,10 @@ done_prefixes:
         tmp = (d & ByteOp) ? 1 : op_bytes;
         if ( tmp == 8 ) tmp = 4;
         /* NB. Immediates are sign-extended as necessary. */
-        length += tmp;
-        pc += tmp;
+        insn_skip(tmp);
         break;
     case SrcImmByte:
-        length += 1;
-        pc += 1;
+        insn_skip(1);
         break;
     }
 
@@ -402,8 +393,7 @@ done_prefixes:
     switch ( b )
     {
     case 0xa0 ... 0xa3: /* mov */
-        length += ad_bytes;
-        pc += ad_bytes; /* skip src/dst displacement */
+        insn_skip(ad_bytes); /* skip src/dst displacement */
         break;
     case 0xf6 ... 0xf7: /* Grp3 */
         switch ( modrm_reg )
@@ -412,8 +402,7 @@ done_prefixes:
             /* Special case in Grp3: test has an immediate source operand. */
             tmp = (d & ByteOp) ? 1 : op_bytes;
             if ( tmp == 8 ) tmp = 4;
-            length += tmp;
-            pc += tmp;
+            insn_skip(tmp);
             break;
         }
         break;

diff --git a/xen/arch/x86/hvm/platform.c b/xen/arch/x86/hvm/platform.c
index 5de07703229cd2603a86dcc665ff610a9defdfac..3d69e9cca52507c826456db1907f0a196349f7ad 100644 (file)
--- a/xen/arch/x86/hvm/platform.c
+++ b/xen/arch/x86/hvm/platform.c
@@ -1041,17 +1041,13 @@ void handle_mmio(unsigned long gpa)
         /* real or vm86 modes */
         address_bytes = 2;
     inst_addr = hvm_get_segment_base(v, x86_seg_cs) + regs->eip;
-    inst_len = hvm_instruction_length(inst_addr, address_bytes);
+    memset(inst, 0, MAX_INST_LEN);
+    inst_len = hvm_instruction_fetch(inst_addr, address_bytes, inst);
     if ( inst_len <= 0 )
     {
-        printk("handle_mmio: failed to get instruction length\n");
-        domain_crash_synchronous();
-    }
-
-    memset(inst, 0, MAX_INST_LEN);
-    if ( inst_copy_from_guest(inst, inst_addr, inst_len) != inst_len ) {
-        printk("handle_mmio: failed to copy instruction\n");
-        domain_crash_synchronous();
+        gdprintk(XENLOG_DEBUG, "handle_mmio: failed to get instruction\n");
+        /* hvm_instruction_fetch() will have injected a #PF; get out now */
+        return;
     }
 
     if ( mmio_decode(address_bytes, inst, mmio_op, &ad_size,

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 1aff21aa227f9b1872c1551688be7646a3f66ef1..e7fa07300c7dea30649f0c804cb1450ccd8e222f 100644 (file)
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -777,7 +777,7 @@ int vmx_vmcs_restore(struct vcpu *v, struct hvm_hw_cpu *c)
             else
                 addrbytes = 2;
 
-            ilen = hvm_instruction_length(c->rip, addrbytes);
+            ilen = hvm_instruction_fetch(c->rip, addrbytes, NULL);
             __vmwrite(VM_ENTRY_INSTRUCTION_LEN, ilen);
         }
 

diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 6adaedad3104ba7c214704b17b809361b0de9056..56eaf619f3a3d749f207d242054a81bb98e3c625 100644 (file)
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -229,7 +229,8 @@ hvm_guest_x86_mode(struct vcpu *v)
     return hvm_funcs.guest_x86_mode(v);
 }
 
-int hvm_instruction_length(unsigned long pc, int address_bytes);
+int hvm_instruction_fetch(unsigned long pc, int address_bytes,
+                          unsigned char *buf);
 
 static inline void
 hvm_update_host_cr3(struct vcpu *v)