[PATCH 0/2] KVM: x86: Fix REP-string effect on RCX/RSI/RDI

[PATCH 0/2] KVM: x86: Fix REP-string effect on RCX/RSI/RDI

[Nadav Amit]

This patch-set fixes KVM behavior when handling a REP-string instruction that
runs with an address-size of 32-bit.  In this case ECX/EDI/ESI are used as
counter and pointers, and the high 32-bits should be cleared.

The first patch handles with the simple case. The second one handles the
corner-case in which ECX is initially zero.  It appears that Intel and AMD
behave differently in this case (and some experiments suggest even different
Intel generations act differently), and I could not find any documentation that
describes it. Yet, the behavior of INS/OUTS can be observed by the guest and
VMware appears to get it right.

Thanks for reviewing the patches.

Nadav Amit (2):
  KVM: x86: Fix update RCX/RDI/RSI on REP-string
  KVM: x86: Fix zero iterations REP-string

 arch/x86/kvm/emulate.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

-- 
2.1.4

[PATCH 1/2] KVM: x86: Fix update RCX/RDI/RSI on REP-string

[Nadav Amit]

When REP-string instruction is preceded with an address-size prefix,
ECX/EDI/ESI are used as the operation counter and pointers.  When they are
updated, the high 32-bits of RCX/RDI/RSI are cleared, similarly to the way they
are updated on every 32-bit register operation.  Fix it.

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index c1bc650..296d58e 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -524,13 +524,9 @@ static void masked_increment(ulong *reg, ulong mask, int inc)
 static inline void
 register_address_increment(struct x86_emulate_ctxt *ctxt, int reg, int inc)
 {
-	ulong mask;
+	ulong *preg = reg_rmw(ctxt, reg);
 
-	if (ctxt->ad_bytes == sizeof(unsigned long))
-		mask = ~0UL;
-	else
-		mask = ad_mask(ctxt);
-	masked_increment(reg_rmw(ctxt, reg), mask, inc);
+	assign_register(preg, *preg + inc, ctxt->ad_bytes);
 }
 
 static void rsp_increment(struct x86_emulate_ctxt *ctxt, int inc)
-- 
2.1.4

[PATCH 2/2] KVM: x86: Fix zero iterations REP-string

[Nadav Amit]

When a REP-string is executed in 64-bit mode with an address-size prefix,
ECX/EDI/ESI are used as counter and pointers. When ECX is initially zero, Intel
CPUs clear the high 32-bits of RCX, and recent Intel CPUs update the high bits
of the pointers in MOVS/STOS. This behavior is specific to Intel according to
few experiments.

As one may guess, this is an undocumented behavior. Yet, it is observable in
the guest, since at least VMX traps REP-INS/OUTS even when ECX=0. Note that
VMware appears to get it right.  The behavior can be observed using the
following code:

 #include <stdio.h>

 #define LOW_MASK	(0xffffffff00000000ull)
 #define ALL_MASK	(0xffffffffffffffffull)
 #define TEST(opcode)							\
	do {								\
	asm volatile(".byte 0xf2 \n\t .byte 0x67 \n\t .byte " opcode "\n\t" \
			: "=S"(s), "=c"(c), "=D"(d) 			\
			: "S"(ALL_MASK), "c"(LOW_MASK), "D"(ALL_MASK));	\
	printf("opcode %s rcx=%llx rsi=%llx rdi=%llx\n",		\
		opcode, c, s, d);					\
	} while(0)

void main()
{
	unsigned long long s, d, c;
	iopl(3);
	TEST("0x6c");
	TEST("0x6d");
	TEST("0x6e");
	TEST("0x6f");
	TEST("0xa4");
	TEST("0xa5");
	TEST("0xa6");
	TEST("0xa7");
	TEST("0xaa");
	TEST("0xab");
	TEST("0xae");
	TEST("0xaf");
}

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 296d58e..e726c50 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2591,6 +2591,30 @@ static bool emulator_io_permited(struct x86_emulate_ctxt *ctxt,
 	return true;
 }
 
+static void string_registers_quirk(struct x86_emulate_ctxt *ctxt)
+{
+	/*
+	 * Intel CPUs mask the counter and pointers in quite strange
+	 * manner when ECX is zero due to REP-string optimizations.
+	 */
+#ifdef CONFIG_X86_64
+	if (ctxt->ad_bytes != 4 || !vendor_intel(ctxt))
+		return;
+
+	*reg_write(ctxt, VCPU_REGS_RCX) = 0;
+
+	switch (ctxt->b) {
+	case 0xa4:	/* movsb */
+	case 0xa5:	/* movsd/w */
+		*reg_rmw(ctxt, VCPU_REGS_RSI) &= (u32)-1;
+		/* fall through */
+	case 0xaa:	/* stosb */
+	case 0xab:	/* stosd/w */
+		*reg_rmw(ctxt, VCPU_REGS_RDI) &= (u32)-1;
+	}
+#endif
+}
+
 static void save_state_to_tss16(struct x86_emulate_ctxt *ctxt,
 				struct tss_segment_16 *tss)
 {
@@ -5004,6 +5028,7 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
 		if (ctxt->rep_prefix && (ctxt->d & String)) {
 			/* All REP prefixes have the same first termination condition */
 			if (address_mask(ctxt, reg_read(ctxt, VCPU_REGS_RCX)) == 0) {
+				string_registers_quirk(ctxt);
 				ctxt->eip = ctxt->_eip;
 				ctxt->eflags &= ~X86_EFLAGS_RF;
 				goto done;
-- 
2.1.4

Re: [PATCH 0/2] KVM: x86: Fix REP-string effect on RCX/RSI/RDI

[Paolo Bonzini]

On 28/04/2015 12:05, Nadav Amit wrote:
> This patch-set fixes KVM behavior when handling a REP-string instruction that
> runs with an address-size of 32-bit.  In this case ECX/EDI/ESI are used as
> counter and pointers, and the high 32-bits should be cleared.
> 
> The first patch handles with the simple case. The second one handles the
> corner-case in which ECX is initially zero.  It appears that Intel and AMD
> behave differently in this case (and some experiments suggest even different
> Intel generations act differently), and I could not find any documentation that
> describes it. Yet, the behavior of INS/OUTS can be observed by the guest and
> VMware appears to get it right.
> 
> Thanks for reviewing the patches.
> 
> Nadav Amit (2):
>   KVM: x86: Fix update RCX/RDI/RSI on REP-string
>   KVM: x86: Fix zero iterations REP-string
> 
>  arch/x86/kvm/emulate.c | 33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 

Applied, thanks.

Paolo