Re: [PATCH 1/2] x86 emulator: Add unary mul, imul, div, and idiv instructions

[Avi Kivity <avi@redhat.com>]

  On 08/08/2010 07:53 AM, Mohammed Gamal wrote:
> On Sun, Aug 8, 2010 at 2:34 PM, Avi Kivity<avi@redhat.com>  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<m.gamal005@gmail.com>
>>> ---
>>>   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.