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:58:12 +0300
Message-ID: <20130422095812.GB6027@redhat.com>
References: <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>
 <20130422094246.GN8997@redhat.com>
 <20130422095203.GE4637@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]:21524 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752328Ab3DVJ61 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Mon, 22 Apr 2013 05:58:27 -0400
Content-Disposition: inline
In-Reply-To: <20130422095203.GE4637@pd.tnic>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Mon, Apr 22, 2013 at 11:52:03AM +0200, Borislav Petkov wrote:
> On Mon, Apr 22, 2013 at 12:42:46PM +0300, Gleb Natapov wrote:
> > > 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;
> >         }
> > }
> 
> Ok, and for OP_MEM it does look at ctxt->dst.bytes in writeback(),
> AFAICT. And I see other emulated instructions like POPF, for example, do
> this:
> 
> 	ctxt->dst.bytes = ctxt->op_bytes;
> 
> Which means, we can drop all the bullshit in em_movbe and even destroy
> some of the bytes in dst.val but only write out the correct ones. Which
> means, a simpler code and a lot less jumping through hoops.
> 
> Would that be the more accepted practice?
> 
For most instructions the decoder already sets op->bytes to correct value,
given that all flags a correctly specified in opcode table. Explicit
op->bytes setting should be done only if it cannot be expressed by
opcode flags.

--
			Gleb.