Re: [RFC PATCH] Emulate MOVBE

[Andre Przywara <andre@andrep.de>]

On Wed, 10 Apr 2013 01:46:02 +0200
Borislav Petkov <bp@alien8.de> wrote:

> Hi guys,
> 
> so I was trying to repro tglx's bug in smpboot.c and for some reason,
> the most reliable way to trigger it was to boot an 32-bit atom smp
> guest in kvm (don't ask :)).
> 
> The problem, however, was that atom wants MOVBE and qemu doesn't
> emulate it yet (except Richard's patches which I used in order to be
> able to actually even boot a guest).
> 
> However, without hw acceleration, qemu is pretty slow, and waiting for
> an smp guest to boot in sw-only emulation is not fun. So, just for
> funsies, I decided to give the MOVBE emulation a try.
> 
> Patch is below, 8 core smp atom guest boots fine and in 6-ish seconds
> with it. :-)
> 
> I know, I know, it still needs cleaning up and proper rFLAGS handling
> but that is for later. For now, I'd very much like to hear whether
> this approach even makes sense and if so, what should be improved.
> 
> Thanks, and thanks to Andre and Jörg for their help.
> 
> Not-yet-signed-off-by: Borislav Petkov <bp@suse.de>

You are missing an (hackish) older patch which enables the MOVBE CPUID
bit even if the host does not have it (malformed due to c&p):
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -273,7 +273,7 @@ static int do_cpuid_ent(struct kvm_cpuid_entry2
*entry, u32 function,
		cpuid_mask(&entry->ecx, 4); /* we support x2apic
emulation even if host does not support
		 * it since we emulate x2apic in software */
-		entry->ecx |= F(X2APIC);
+		entry->ecx |= (F(X2APIC) | F(MOVBE));
		break;
	/* function 2 entries are STATEFUL. That is, repeated cpuid
commands
	 * may return different values. This forces us to get_cpu()
before

> 
> --
> diff --git a/arch/x86/include/asm/kvm_emulate.h
> b/arch/x86/include/asm/kvm_emulate.h index 15f960c06ff7..ae01c765cd77
> 100644 --- a/arch/x86/include/asm/kvm_emulate.h
> +++ b/arch/x86/include/asm/kvm_emulate.h
> @@ -281,6 +281,7 @@ struct x86_emulate_ctxt {
>  
>  	/* decode cache */
>  	u8 twobyte;
> +	u8 thirdbyte;
>  	u8 b;
>  	u8 intercept;
>  	u8 lock_prefix;
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index a335cc6cde72..0ccff339359d 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -152,6 +152,7 @@
>  #define Avx         ((u64)1 << 43)  /* Advanced Vector Extensions */
>  #define Fastop      ((u64)1 << 44)  /* Use opcode::u.fastop */
>  #define NoWrite     ((u64)1 << 45)  /* No writeback */
> +#define ThreeByte   ((u64)1 << 46)  /* Three byte opcodes 0F 38 and
> 0F 3A */ 
>  #define X2(x...) x, x
>  #define X3(x...) X2(x), x
> @@ -3107,6 +3108,34 @@ static int em_mov(struct x86_emulate_ctxt
> *ctxt) return X86EMUL_CONTINUE;
>  }
>  
> +static int em_movbe(struct x86_emulate_ctxt *ctxt)
> +{
> +	char *valptr = ctxt->src.valptr;
> +
> +	switch (ctxt->op_bytes) {
> +	case 2:
> +		*(u16 *)valptr = swab16(*(u16 *)valptr);
> +		break;
> +	case 4:
> +		*(u32 *)valptr = swab32(*(u32 *)valptr);
> +
> +		/*
> +		 * clear upper dword for 32-bit operand size in
> 64-bit mode.
> +		 */
> +		if (ctxt->mode == X86EMUL_MODE_PROT64)
> +			*((u32 *)valptr + 1) = 0x0;
> +		break;
> +	case 8:
> +		*(u64 *)valptr = swab64(*(u64 *)valptr);
> +		break;
> +	default:
> +		return X86EMUL_PROPAGATE_FAULT;
> +	}
> +
> +	memcpy(ctxt->dst.valptr, ctxt->src.valptr, ctxt->op_bytes);
> +	return X86EMUL_CONTINUE;
> +}
> +
On 04/09/2013 05:03 PM, Borislav Petkov wrote:
> 
> Note to self: this destroys the src operand but it shouldn't. Fix it
> tomorrow.
> 

What about this one?

	*(u16 *)(ctxt->dst.valptr) = swab16(*(u16 *)(ctxt->src.valptr));

Respective versions for the other operand sizes, of course, and drop
the final memcpy.

>  static int em_cr_write(struct x86_emulate_ctxt *ctxt)
>  {
>  	if (ctxt->ops->set_cr(ctxt, ctxt->modrm_reg, ctxt->src.val))
> @@ -3974,7 +4003,8 @@ static const struct opcode twobyte_table[256] =
> { I(ImplicitOps | VendorSpecific, em_sysenter),
>  	I(ImplicitOps | Priv | VendorSpecific, em_sysexit),
>  	N, N,
> -	N, N, N, N, N, N, N, N,
> +	I(ModRM | Mov | ThreeByte | VendorSpecific, em_movbe),
> +	N, N, N, N, N, N, N,

In a real world VendorSpecific should be replaced with something more
meaningful. Depends on KVMs intention to emulate instructions, actually
out of scope for a pure virtualizer.

What is the opinion from the KVM folks on this? Shall we start to
emulate instructions the host does not provide? In this particular case
a relatively simple patch fixes a problem (starting Atom optimized
kernels on non-Atom machines).

(And if one can believe the AMD Fam16h SWOG [1], PS4^Wfuture AMD
processors have MOVBE, so it's not even actually one CPU anymore).

>  	/* 0x40 - 0x4F */
>  	X16(D(DstReg | SrcMem | ModRM | Mov)),
>  	/* 0x50 - 0x5F */
> @@ -4323,6 +4353,15 @@ done_prefixes:
>  	}
>  	ctxt->d = opcode.flags;
>  
> +	if (ctxt->d & ThreeByte) {
> +		ctxt->thirdbyte = insn_fetch(u8, ctxt);
> +
> +		if (ctxt->thirdbyte == 0xf0)
> +			ctxt->d |= DstReg | SrcMem;
> +		else
> +			ctxt->d |= DstMem | SrcReg;
> +	}
> +

As already agreed on on IRC, we should properly check for the other
opcodes in the 0F 38/3A table. Or make it right and implement a real
sub-table.

Regards,
Andre.


[1]http://support.amd.com/us/Processor_TechDocs/52128_16h_Software_Opt_Guide.zip