Re: [PATCH -v2] kvm: Emulate MOVBE

[Gleb Natapov <gleb@redhat.com>]

On Mon, Apr 22, 2013 at 11:38:10AM +0200, Borislav Petkov wrote:
> On Mon, Apr 22, 2013 at 10:53:42AM +0200, Paolo Bonzini wrote:
> > Il 21/04/2013 14:23, Borislav Petkov ha scritto:
> > > On Sun, Apr 21, 2013 at 01:46:50PM +0200, Borislav Petkov wrote:
> > >> We probably need something with copying values to a temp variable or so.
> > > 
> > > Basically something like that:
> > > 
> > >         case 2:
> > >                 /*
> > >                  * From MOVBE definition: "...When the operand size is 16 bits,
> > >                  * the upper word of the destination register remains unchanged
> > >                  * ..."
> > >                  *
> > >                  * Both casting ->valptr and ->val to u16 breaks strict aliasing
> > >                  * rules so we have to do the operation almost per hand.
> > >                  */
> > >                 tmp = (u16)ctxt->src.val;
> > >                 ctxt->dst.val &= ~0xffffUL;
> > >                 ctxt->dst.val |= (unsigned long)swab16(tmp);
> > > 		break;
> > > 
> > > This passes all gcc checks, even the stricter ones when building with W=3.
> > 
> > I thought the valptr one was ok.
> 
> Yep, it looked like that too. And, it could actually really be ok and
> the gcc's warning here is bogus. I'll try to talk to gcc people about
> it.
> 
> > I find this one more readable, too. How does the generated code look
> > like?
> 
> Well, so so:
> 
> 	movzwl	112(%rdi), %eax	# ctxt_5(D)->src.D.27823.val, tmp87
> 	movq	240(%rdi), %rdx	# ctxt_5(D)->dst.D.27823.val, tmp89
> 	xorw	%dx, %dx	# tmp89
> 	rolw	$8, %ax	#, tmp87
> 	movzwl	%ax, %eax	# tmp87, tmp91
> 
> I have hard time understanding why it is adding this insn here - it can
> simply drop it and continue with the 64-bit OR. It's not like it changes
> anything...
> 
> 	orq	%rdx, %rax	# tmp89, tmp91
> 	movq	%rax, 240(%rdi)	# tmp91, ctxt_5(D)->dst.D.27823.val
> 
> Btw, I wanted to ask: when kvm commits the results, does it look at
> ctxt->op_bytes to know exactly how many bytes to write to the guest?
> Because if it does, we can save ourselves the trouble here.
> 
> Or does it simply write both the full sizeof(unsigned long) bytes of
> ->src.val and ->dst.val to the guest?
> 
No, it does this in case of register operand:

static void write_register_operand(struct operand *op)
{
        /* The 4-byte case *is* correct: in 64-bit mode we zero-extend.  */
        switch (op->bytes) {
        case 1:
                *(u8 *)op->addr.reg = (u8)op->val;
                break;
        case 2:
                *(u16 *)op->addr.reg = (u16)op->val;
                break;
        case 4:
                *op->addr.reg = (u32)op->val;
                break;  /* 64b: zero-extend */
        case 8:
                *op->addr.reg = op->val;
                break;
        }
}

--
			Gleb.