Re: Emulation of shld instruction doesn't work

[Avi Kivity <avi@redhat.com>]

Guillaume Thouvenin wrote:
> Hello,
>
>  I need to emulate the shld instruction (needed when enabling invalid
> guest state framework).
>
>  I added the code that emulates shld in kvm (see the end of this email)
> and I also made a test case in user/test/x86/realmode.c. I think that
> the code that emulated the instruction is fine but when I run kvmctl
> with file realmode.flat it failed. Here is the test that I added in
> realmode.c:
>   

Instructions that modify memory are best tested in emulator.c rather 
than realmode.c.

> diff --git a/user/test/x86/realmode.c b/user/test/x86/realmode.c
> index 69ded37..8533240 100644
> --- a/user/test/x86/realmode.c
> +++ b/user/test/x86/realmode.c
> @@ -141,6 +141,22 @@ int regs_equal(const struct regs *r1, const struct regs *r2, int ignore)
>                 );                                 \
>         extern u8 insn_##name[], insn_##name##_end[]
>  
> +void test_shld(const struct regs *inregs, struct regs *outregs)
> +{
> +       MK_INSN(shld_test, "mov $0xbe, %ebx\n\t"
> +                          "mov $0xef000000, %ecx\n\t"
>   

Do the assignments in C, modifying the inregs parameter rather than in 
assembly.


> @@ -230,7 +230,8 @@ static u16 twobyte_table[256] = {
>  	/* 0x90 - 0x9F */
>  	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
>  	/* 0xA0 - 0xA7 */
> -	0, 0, 0, DstMem | SrcReg | ModRM | BitOp, 0, 0, 0, 0,
> +	0, 0, 0, DstMem | SrcReg | ModRM | BitOp,
> +	DstMem | SrcReg | ModRM, DstMem | SrcReg | ModRM, 0, 0,
>  	/* 0xA8 - 0xAF */
>  	0, 0, 0, DstMem | SrcReg | ModRM | BitOp, 0, 0, ModRM, 0,
>  	/* 0xB0 - 0xB7 */
> @@ -501,6 +502,16 @@ static u16 group2_table[] = {
>  	(_type)_x;							\
>  })
>  
> +/*
> + * Reflects the parity of the given argument. It is set if
> + * the number of ones is even.
> + */
> +static int even_parity(uint8_t v)
> +{
> +	asm ( "test %b0,%b0; setp %b0" : "=a" (v) : "0" (v) );
> +	return v;
> +}
> +
>  static inline unsigned long ad_mask(struct decode_cache *c)
>  {
>  	return (1UL << (c->ad_bytes << 3)) - 1;
> @@ -1993,6 +2004,46 @@ twobyte_insn:
>  		c->src.val &= (c->dst.bytes << 3) - 1;
>  		emulate_2op_SrcV_nobyte("bt", c->src, c->dst, ctxt->eflags);
>  		break;
> +	case 0xa4: /* shld imm8, r, r/m */
> +	case 0xa5: /* shld cl, r, r/m */ {
> +		uint8_t size;
> +		uint8_t count;
> +
> +		size = c->dst.bytes << 3;
> +		count = (c->b & 1) ? (uint8_t) c->regs[VCPU_REGS_RCX] : insn_fetch(u8, 1, c->eip);
>   

You need to fetch during the decode phase rather than the execution 
phase.  This is probably the source of the problem.

shld has three operands, so we need to add s Src2 decode set.  I guess 
we can start with Src2One, Src2CL, and Src2Imm8 to support shld and 
expand it later.

> +
> +		if (count == 0)	/* No operation */
> +			break;
> +
> +		if (count > size) {
> +			printk(KERN_INFO "shld: bad parameters \n");
> +			break;
> +		}
>   

The parameters aren't bad; the processor ignores excess bits.

> +
> +		c->dst.orig_val = c->dst.val;
> +		c->dst.type = OP_REG;
> +		c->dst.val <<= count;
> +		c->dst.val |= ((c->src.val >> (size - count)) & ((1ull << count) - 1));
> +
> +		/* Flags affected */
> +		ctxt->eflags &= ~(EFLG_OF|EFLG_SF|EFLG_ZF|EFLG_PF|EFLG_CF);
> +
> +		/* Set CF with left bit shifted if count is 1 or greater */
> +		if ((c->dst.orig_val >> (size - count)) & 1)
> +			ctxt->eflags |= EFLG_CF;
> +		
> +		/* For 1-bit shit, OF is set if a sign change occured, otherwise it
> +		 * is cleared. For shift greater than 1 it is undefined */
> +		if (((c->dst.orig_val ^ c->dst.val) >> (size - 1)) & 1)
> +			ctxt->eflags |= EFLG_OF;
> +
> +		/* SF, ZF, and PF flags are set according to the value of the result */
> +		ctxt->eflags |= ((c->dst.val >> (size - 1)) & 1) ? EFLG_SF : 0;
> +		ctxt->eflags |= (c->dst.val == 0) ? EFLG_ZF : 0;
> +		ctxt->eflags |= even_parity(c->dst.val) ? EFLG_PF : 0;
> +
>   

This needs to be implemented like the rest of the instructions, with the 
processor calculating the flags and result.


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.