From mboxrd@z Thu Jan  1 00:00:00 1970
From: Gleb Natapov <gleb@redhat.com>
Subject: Re: [PATCH -v2] kvm: Emulate MOVBE
Date: Mon, 22 Apr 2013 12:42:46 +0300
Message-ID: <20130422094246.GN8997@redhat.com>
References: <20130410123901.46b65169@slackpad>
 <20130410121639.GE17919@redhat.com>
 <20130411001815.GA17544@pd.tnic>
 <20130414084303.GE17919@redhat.com>
 <20130414210218.GF20547@pd.tnic>
 <516D37A0.2070706@redhat.com>
 <20130421114649.GC4594@pd.tnic>
 <20130421122321.GD4594@pd.tnic>
 <5174FA96.1040807@redhat.com>
 <20130422093810.GC4637@pd.tnic>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Andre Przywara <andre@andrep.de>, kvm@vger.kernel.org,
	=?utf-8?B?SsO2cmcgUsO2ZGVs?= <joro@8bytes.org>,
	"H. Peter Anvin" <hpa@zytor.com>, x86-ml <x86@kernel.org>
To: Borislav Petkov <bp@alien8.de>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:41797 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1756015Ab3DVJnB (ORCPT <rfc822;kvm@vger.kernel.org>);
	Mon, 22 Apr 2013 05:43:01 -0400
Content-Disposition: inline
In-Reply-To: <20130422093810.GC4637@pd.tnic>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.