Re: [PATCH -v2] kvm: Emulate MOVBE

[Borislav Petkov <bp@alien8.de>]

On Sun, Apr 14, 2013 at 11:43:03AM +0300, Gleb Natapov wrote:
> > +#define EmulateOnUD ((u64)1 << 46)  /* emulate if unsupported by the host */
> >  
> Just rename VendorSpecific to EmulateOnUD. The meaning is the same.

done.

> > +static int em_movbe(struct x86_emulate_ctxt *ctxt)
> > +{
> > +	switch (ctxt->op_bytes) {
> > +	case 2:
> > +		*(u16 *)ctxt->dst.valptr = swab16(*(u16 *)ctxt->src.valptr);
> What's wrong with: ctxt->dst.val = swab16((u16)ctxt->src.val)?

This doesn't work for the 16-bit swab because of the following
functionality of MOVBE:

"When the operand size is 16 bits, the upper word of the destination
register remains unchanged."

Now here's what gcc produces here:

	movzwl	112(%rdi), %eax	# ctxt_5(D)->src.D.27823.val, tmp83
	rolw	$8, %ax	#, tmp83
	movzwl	%ax, %eax	# tmp83, tmp84
	movq	%rax, 240(%rdi)	# tmp84,

See the zero-extension happening twice (second one should not be needed,
even) and it is actually writing the whole 64-bit value in %rax back
which effectively destroys the upper word?

However, this, a bit uglier version works:

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

	movzwl  112(%rdi), %eax # ctxt_5(D)->src.D.27823.val, tmp82
	rolw    $8, %ax #, tmp82
	movw    %ax, 240(%rdi)  # tmp82, MEM[(u16 *)ctxt_5(D) + 240B]

Whichever version we take, I'll slap a comment explaining why we're
doing it differently for 16-bit.

> > +	case 4:
> > +		*(u32 *)ctxt->dst.valptr = swab32(*(u32 *)ctxt->src.valptr);
> > +
> > +		/*
> > +		 * clear upper dword for 32-bit operand size in 64-bit mode.
> > +		 */
> > +		if (ctxt->mode == X86EMUL_MODE_PROT64)
> > +			*((u32 *)ctxt->dst.valptr + 1) = 0x0;
> It should be clean without this this.

Right, this should probably work because for 32-bit, we don't care
about the zero-extended upper dword and in 64-bit mode we want to do it
anyway.

> > +		break;
> > +	case 8:
> > +		*(u64 *)ctxt->dst.valptr = swab64(*(u64 *)ctxt->src.valptr);

Ditto for this one.

> > +static const struct opcode threebyte_table[] = {
> > +	[0xf0] = I(DstReg | SrcMem | ModRM | Mov | EmulateOnUD, em_movbe),
> > +	[0xf1] = I(DstMem | SrcReg | ModRM | Mov | EmulateOnUD, em_movbe),
> > +};
> > +
> Populate the whole table explicitly please. You can use X16(N) to make it
> short. The encoding is also wrong since 0F 38 F0/F1 instruction decoding
> depend on a prefix.

... on the absence of a prefix, actually.

> Same opcode is decoded as CRC32 with f2 prefix. We have Prefix
> mechanism for such instructions, but it currently assumes that 66 and
> f2/f3 prefixes are mutually exclusive, but in this case they are not,
> ugh.

Yeah, I'm lazy and wanted to leave the honors to the poor soul who
implements the whole 0F_38h opcode map. :-)

Ok, I probably could do something similar to pfx_vmovntpx with the GP
macro for the F0/F1 opcodes. I'll give it a try when I get a chance
soonish.

> > +
> > +		if (ctxt->b == 0x38)
> > +			opcode = threebyte_table[insn_fetch(u8, ctxt)];
> ctxt->b = insn_fetch(u8, ctxt);
> opcode = threebyte_table[ctxt->b];
> 
> Otherwise OpImplicit type of decoding will not work for three byte
> instructions. Also should rename twobyte into opbytes and set it to 1,
> 2 or 3. I prefer that three byte instruction decoding will be sent as a
> separate patch.

Ok.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--