[PATCH] KVM: x86: MOV CR/DR emulation should ignore mod

[PATCH] KVM: x86: MOV CR/DR emulation should ignore mod

[Nadav Amit]

MOV CR/DR instructions ignore the mod field (in the ModR/M byte). As the SDM
states: "The 2 bits in the mod field are ignored".  Accordingly, the second
operand of these instructions is always a general purpose register.

The current emulator implementation does not do so. If the mod bits do not
equal 3, it expects the second operand to be in memory.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/emulate.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 2fa7ab0..e4e833d 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -161,6 +161,7 @@
 #define Fastop      ((u64)1 << 44)  /* Use opcode::u.fastop */
 #define NoWrite     ((u64)1 << 45)  /* No writeback */
 #define SrcWrite    ((u64)1 << 46)  /* Write back src operand */
+#define NoMod	    ((u64)1 << 47)  /* Mod field is ignored */
 
 #define DstXacc     (DstAccLo | SrcAccHi | SrcWrite)
 
@@ -1077,7 +1078,7 @@ static int decode_modrm(struct x86_emulate_ctxt *ctxt,
 	ctxt->modrm_rm |= (ctxt->modrm & 0x07);
 	ctxt->modrm_seg = VCPU_SREG_DS;
 
-	if (ctxt->modrm_mod == 3) {
+	if (ctxt->modrm_mod == 3 || (ctxt->d & NoMod)) {
 		op->type = OP_REG;
 		op->bytes = (ctxt->d & ByteOp) ? 1 : ctxt->op_bytes;
 		op->addr.reg = decode_register(ctxt, ctxt->modrm_rm,
@@ -3877,10 +3878,12 @@ static const struct opcode twobyte_table[256] = {
 	N, N, N, N, N, N, N, N,
 	D(ImplicitOps | ModRM), N, N, N, N, N, N, D(ImplicitOps | ModRM),
 	/* 0x20 - 0x2F */
-	DIP(ModRM | DstMem | Priv | Op3264, cr_read, check_cr_read),
-	DIP(ModRM | DstMem | Priv | Op3264, dr_read, check_dr_read),
-	IIP(ModRM | SrcMem | Priv | Op3264, em_cr_write, cr_write, check_cr_write),
-	IIP(ModRM | SrcMem | Priv | Op3264, em_dr_write, dr_write, check_dr_write),
+	DIP(ModRM | DstMem | Priv | Op3264 | NoMod, cr_read, check_cr_read),
+	DIP(ModRM | DstMem | Priv | Op3264 | NoMod, dr_read, check_dr_read),
+	IIP(ModRM | SrcMem | Priv | Op3264 | NoMod, em_cr_write, cr_write,
+						check_cr_write),
+	IIP(ModRM | SrcMem | Priv | Op3264 | NoMod, em_dr_write, dr_write,
+						check_dr_write),
 	N, N, N, N,
 	GP(ModRM | DstReg | SrcMem | Mov | Sse, &pfx_0f_28_0f_29),
 	GP(ModRM | DstMem | SrcReg | Mov | Sse, &pfx_0f_28_0f_29),
-- 
1.9.1

Re: [PATCH] KVM: x86: MOV CR/DR emulation should ignore mod

[Paolo Bonzini]

Il 25/05/2014 22:05, Nadav Amit ha scritto:
> MOV CR/DR instructions ignore the mod field (in the ModR/M byte). As the SDM
> states: "The 2 bits in the mod field are ignored".  Accordingly, the second
> operand of these instructions is always a general purpose register.
>
> The current emulator implementation does not do so. If the mod bits do not
> equal 3, it expects the second operand to be in memory.
>
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  arch/x86/kvm/emulate.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 2fa7ab0..e4e833d 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -161,6 +161,7 @@
>  #define Fastop      ((u64)1 << 44)  /* Use opcode::u.fastop */
>  #define NoWrite     ((u64)1 << 45)  /* No writeback */
>  #define SrcWrite    ((u64)1 << 46)  /* Write back src operand */
> +#define NoMod	    ((u64)1 << 47)  /* Mod field is ignored */
>
>  #define DstXacc     (DstAccLo | SrcAccHi | SrcWrite)
>
> @@ -1077,7 +1078,7 @@ static int decode_modrm(struct x86_emulate_ctxt *ctxt,
>  	ctxt->modrm_rm |= (ctxt->modrm & 0x07);
>  	ctxt->modrm_seg = VCPU_SREG_DS;
>
> -	if (ctxt->modrm_mod == 3) {
> +	if (ctxt->modrm_mod == 3 || (ctxt->d & NoMod)) {
>  		op->type = OP_REG;
>  		op->bytes = (ctxt->d & ByteOp) ? 1 : ctxt->op_bytes;
>  		op->addr.reg = decode_register(ctxt, ctxt->modrm_rm,
> @@ -3877,10 +3878,12 @@ static const struct opcode twobyte_table[256] = {
>  	N, N, N, N, N, N, N, N,
>  	D(ImplicitOps | ModRM), N, N, N, N, N, N, D(ImplicitOps | ModRM),
>  	/* 0x20 - 0x2F */
> -	DIP(ModRM | DstMem | Priv | Op3264, cr_read, check_cr_read),
> -	DIP(ModRM | DstMem | Priv | Op3264, dr_read, check_dr_read),
> -	IIP(ModRM | SrcMem | Priv | Op3264, em_cr_write, cr_write, check_cr_write),
> -	IIP(ModRM | SrcMem | Priv | Op3264, em_dr_write, dr_write, check_dr_write),
> +	DIP(ModRM | DstMem | Priv | Op3264 | NoMod, cr_read, check_cr_read),
> +	DIP(ModRM | DstMem | Priv | Op3264 | NoMod, dr_read, check_dr_read),
> +	IIP(ModRM | SrcMem | Priv | Op3264 | NoMod, em_cr_write, cr_write,
> +						check_cr_write),
> +	IIP(ModRM | SrcMem | Priv | Op3264 | NoMod, em_dr_write, dr_write,
> +						check_dr_write),
>  	N, N, N, N,
>  	GP(ModRM | DstReg | SrcMem | Mov | Sse, &pfx_0f_28_0f_29),
>  	GP(ModRM | DstMem | SrcReg | Mov | Sse, &pfx_0f_28_0f_29),
>

This is easy to test in kvm-unit-tests.  Please provide a patch for that 
as well. :)

Paolo

[PATCH kvm-unit-tests] x86: test mov DR with ignored mod bits

[Nadav Amit]

Tests whether the emulator ignores the mod bits on mov DR instruction.  The
test performs regular mov to DR0 and reads back the data using custom mov from
DR0 while mod bits are set to zero. The expected result is obviously the value
which was written to DR0 before.

The test is performed in realmode and is only expected to fail if the CPU does
not support unrestricted mode. Note that mov-CR/DR in protected mode are
usually not handled by the emulator, as their data is available in the exit
information fields.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 x86/realmode.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/x86/realmode.c b/x86/realmode.c
index 9c22d41..839ac34 100644
--- a/x86/realmode.c
+++ b/x86/realmode.c
@@ -1636,6 +1636,16 @@ static void test_perf_memory_rmw(void)
 	print_serial(" cycles/emulated memory RMW instruction\n");
 }
 
+void test_dr_mod(void)
+{
+	MK_INSN(drmod, "movl %ebx, %dr0\n\t"
+		       ".byte 0x0f \n\t .byte 0x21 \n\t .byte 0x0\n\t");
+	inregs.eax = 0xdead;
+	inregs.ebx = 0xaced;
+	exec_in_big_real_mode(&insn_drmod);
+	report("mov dr with mod bits", R_AX | R_BX, outregs.eax == 0xaced);
+}
+
 void realmode_start(void)
 {
 	test_null();
@@ -1681,6 +1691,7 @@ void realmode_start(void)
 	test_xlat();
 	test_salc();
 	test_fninit();
+	test_dr_mod();
 	test_nopl();
 	test_perf_loop();
 	test_perf_mov();
-- 
1.9.1

Re: [PATCH kvm-unit-tests] x86: test mov DR with ignored mod bits

[Paolo Bonzini]

Il 26/05/2014 23:06, Nadav Amit ha scritto:
> Tests whether the emulator ignores the mod bits on mov DR instruction.  The
> test performs regular mov to DR0 and reads back the data using custom mov from
> DR0 while mod bits are set to zero. The expected result is obviously the value
> which was written to DR0 before.
>
> The test is performed in realmode and is only expected to fail if the CPU does
> not support unrestricted mode. Note that mov-CR/DR in protected mode are
> usually not handled by the emulator, as their data is available in the exit
> information fields.
>
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  x86/realmode.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/x86/realmode.c b/x86/realmode.c
> index 9c22d41..839ac34 100644
> --- a/x86/realmode.c
> +++ b/x86/realmode.c
> @@ -1636,6 +1636,16 @@ static void test_perf_memory_rmw(void)
>  	print_serial(" cycles/emulated memory RMW instruction\n");
>  }
>
> +void test_dr_mod(void)
> +{
> +	MK_INSN(drmod, "movl %ebx, %dr0\n\t"
> +		       ".byte 0x0f \n\t .byte 0x21 \n\t .byte 0x0\n\t");
> +	inregs.eax = 0xdead;
> +	inregs.ebx = 0xaced;
> +	exec_in_big_real_mode(&insn_drmod);
> +	report("mov dr with mod bits", R_AX | R_BX, outregs.eax == 0xaced);
> +}
> +
>  void realmode_start(void)
>  {
>  	test_null();
> @@ -1681,6 +1691,7 @@ void realmode_start(void)
>  	test_xlat();
>  	test_salc();
>  	test_fninit();
> +	test_dr_mod();
>  	test_nopl();
>  	test_perf_loop();
>  	test_perf_mov();
>

Thanks, applied.

Paolo