From: Tim Deegan Date: Tue, 31 Jul 2007 10:37:27 +0000 (+0100) Subject: [HVM] Inject #PF when mmio instruction fetch fails X-Git-Tag: 3.2.0-rc1~386^2~5^2~3 X-Git-Url: http://xenbits.xensource.com/gitweb?a=commitdiff_plain;h=8f5a6682da6bd054b4090a0f1b40f3e44f9abe4b;p=xen.git [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 --- diff --git a/xen/arch/x86/hvm/instrlen.c b/xen/arch/x86/hvm/instrlen.c index 0255d1f7c9..d258e744dc 100644 --- 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 #include #include @@ -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 5de0770322..3d69e9cca5 100644 --- 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 1aff21aa22..e7fa07300c 100644 --- 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 6adaedad31..56eaf619f3 100644 --- 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)