From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 1/2] x86 emulator: Add unary mul, imul, div, and idiv instructions Date: Sun, 08 Aug 2010 16:27:44 -0400 Message-ID: <4C5F1340.7030906@redhat.com> References: <1281234459-9248-1-git-send-email-m.gamal005@gmail.com> <4C5E9658.6070803@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: mtosatti@redhat.com, kvm@vger.kernel.org To: Mohammed Gamal Return-path: Received: from mx1.redhat.com ([209.132.183.28]:24683 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754178Ab0HHU1w (ORCPT ); Sun, 8 Aug 2010 16:27:52 -0400 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On 08/08/2010 07:53 AM, Mohammed Gamal wrote: > On Sun, Aug 8, 2010 at 2:34 PM, Avi Kivity wrote: >> On 08/08/2010 05:27 AM, Mohammed Gamal wrote: >>> This adds unary mul, imul, div, and idiv instructions (group 3 r/m 4-7). >>> >>> Signed-off-by: Mohammed Gamal >>> --- >>> arch/x86/kvm/emulate.c | 41 ++++++++++++++++++++++++++++++++++++++++- >>> 1 files changed, 40 insertions(+), 1 deletions(-) >>> >>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c >>> index 7d78832..6d1ec53 100644 >>> --- a/arch/x86/kvm/emulate.c >>> +++ b/arch/x86/kvm/emulate.c >>> @@ -315,6 +315,31 @@ struct group_dual { >>> } \ >>> } while (0) >>> >>> +#define __emulate_1op_src(_op, _src, _ax, _dx, _eflags, _suffix) >>> \ >> Not just 1op - add rax_rdx to the name to indicate these are implicit >> operands. >> >>> + do { \ >>> + unsigned long _tmp; \ >>> + \ >>> + __asm__ __volatile__ ( \ >>> + _PRE_EFLAGS("0", "4", "1") \ >>> + _op _suffix " %5; " \ >>> + _POST_EFLAGS("0", "4", "1") \ >>> + : "=m" (_eflags), "=&r" (_tmp), \ >>> + "=a" (_ax), "=d" (_dx) \ >>> + : "i" (EFLAGS_MASK), "m" ((_src).val), \ >>> + "a" (_ax), "d" (_dx)); \ >>> + } while (0) >> The byte form of the instruction doesn't update dx, and the word form >> doesn't update edx[16:31]. So the "=a" and "=d" operands need to be "+a" >> and "+d" so the compiler loads them before the operation is started. >> I see you have "a" and "d" also in the inputs section. So the code is correct. >> Please add a test to the effect, for example start with eax=0x12345678, and >> multiply (byte size) 0x80 by 0x40, > We already have a value store in eax, so I can only either multiply it > by 0x80 or 0x40. Or do you mean we should have eax=0x12345680 and then > perhaps multply by 0x40 and make sure the upper 16 bits are preserved? Yes. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.