KVM: emulator: implement AAD instruction

Message ID	20121210094230.GI19514@redhat.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <kvm-owner@vger.kernel.org> Date: Mon, 10 Dec 2012 11:42:30 +0200 From: Gleb Natapov <gleb@redhat.com> To: kvm@vger.kernel.org Cc: mtosatti@redhat.com Subject: [PATCH] KVM: emulator: implement AAD instruction Message-ID: <20121210094230.GI19514@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Sender: kvm-owner@vger.kernel.org Precedence: bulk

Message ID

20121210094230.GI19514@redhat.com (mailing list archive)

State

New, archived

Headers

Date: Mon, 10 Dec 2012 11:42:30 +0200
From: Gleb Natapov <gleb@redhat.com>
To: kvm@vger.kernel.org
Cc: mtosatti@redhat.com
Subject: [PATCH] KVM: emulator: implement AAD instruction
Message-ID: <20121210094230.GI19514@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Sender: kvm-owner@vger.kernel.org
Precedence: bulk

Commit Message

Gleb Natapov Dec. 10, 2012, 9:42 a.m. UTC

Windows2000 uses it during boot. This fixes
https://bugzilla.kernel.org/show_bug.cgi?id=50921

Signed-off-by: Gleb Natapov <gleb@redhat.com>
--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Paolo Bonzini Dec. 14, 2012, 10:44 a.m. UTC | #1

Il 10/12/2012 10:42, Gleb Natapov ha scritto:
> Windows2000 uses it during boot. This fixes
> https://bugzilla.kernel.org/show_bug.cgi?id=50921
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 39171cb..92c7292 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -2852,6 +2852,27 @@ static int em_das(struct x86_emulate_ctxt *ctxt)
>  	return X86EMUL_CONTINUE;
>  }
>  
> +static int em_aad(struct x86_emulate_ctxt *ctxt)
> +{
> +	u8 al = ctxt->dst.val & 0xff;
> +	u8 ah = (ctxt->dst.val >> 8) & 0xff;
> +
> +	al = (al + (ah * ctxt->src.val)) & 0xff;
> +
> +	ctxt->dst.val = (ctxt->dst.val & 0xffff0000) | al;
> +
> +	ctxt->eflags &= ~(X86_EFLAGS_PF | X86_EFLAGS_SF | X86_EFLAGS_ZF);
> +
> +	if (!al)
> +		ctxt->eflags |= X86_EFLAGS_ZF;
> +	if (!(al & 1))
> +		ctxt->eflags |= X86_EFLAGS_PF;

This is wrong, it should check the parity of al (even=1, odd=0).

Perhaps you can use the trick of em_das:

        /* Set PF, ZF, SF */
        ctxt->src.type = OP_IMM;
        ctxt->src.val = 0;
        ctxt->src.bytes = 1;
        emulate_2op_SrcV(ctxt, "or");

Paolo

> +	if (al & 0x80)
> +		ctxt->eflags |= X86_EFLAGS_SF;
> +
> +	return X86EMUL_CONTINUE;
> +}
> +
>  static int em_call(struct x86_emulate_ctxt *ctxt)
>  {
>  	long rel = ctxt->src.val;
> @@ -3801,7 +3822,7 @@ static const struct opcode opcode_table[256] = {
>  	D(ImplicitOps | No64), II(ImplicitOps, em_iret, iret),
>  	/* 0xD0 - 0xD7 */
>  	D2bv(DstMem | SrcOne | ModRM), D2bv(DstMem | ModRM),
> -	N, N, N, N,
> +	N, I(DstAcc | SrcImmByte | No64, em_aad), N, N,
>  	/* 0xD8 - 0xDF */
>  	N, N, N, N, N, N, N, N,
>  	/* 0xE0 - 0xE7 */
> --
> 			Gleb.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Gleb Natapov Dec. 14, 2012, 11:29 a.m. UTC | #2

On Fri, Dec 14, 2012 at 11:44:22AM +0100, Paolo Bonzini wrote:
> Il 10/12/2012 10:42, Gleb Natapov ha scritto:
> > Windows2000 uses it during boot. This fixes
> > https://bugzilla.kernel.org/show_bug.cgi?id=50921
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> > index 39171cb..92c7292 100644
> > --- a/arch/x86/kvm/emulate.c
> > +++ b/arch/x86/kvm/emulate.c
> > @@ -2852,6 +2852,27 @@ static int em_das(struct x86_emulate_ctxt *ctxt)
> >  	return X86EMUL_CONTINUE;
> >  }
> >  
> > +static int em_aad(struct x86_emulate_ctxt *ctxt)
> > +{
> > +	u8 al = ctxt->dst.val & 0xff;
> > +	u8 ah = (ctxt->dst.val >> 8) & 0xff;
> > +
> > +	al = (al + (ah * ctxt->src.val)) & 0xff;
> > +
> > +	ctxt->dst.val = (ctxt->dst.val & 0xffff0000) | al;
> > +
> > +	ctxt->eflags &= ~(X86_EFLAGS_PF | X86_EFLAGS_SF | X86_EFLAGS_ZF);
> > +
> > +	if (!al)
> > +		ctxt->eflags |= X86_EFLAGS_ZF;
> > +	if (!(al & 1))
> > +		ctxt->eflags |= X86_EFLAGS_PF;
> 
> This is wrong, it should check the parity of al (even=1, odd=0).
> 
Oops, yes it should check number of bits set, not value itself.

> Perhaps you can use the trick of em_das:
> 
>         /* Set PF, ZF, SF */
>         ctxt->src.type = OP_IMM;
>         ctxt->src.val = 0;
>         ctxt->src.bytes = 1;
>         emulate_2op_SrcV(ctxt, "or");
Will do.

> 
> Paolo
> 
> > +	if (al & 0x80)
> > +		ctxt->eflags |= X86_EFLAGS_SF;
> > +
> > +	return X86EMUL_CONTINUE;
> > +}
> > +
> >  static int em_call(struct x86_emulate_ctxt *ctxt)
> >  {
> >  	long rel = ctxt->src.val;
> > @@ -3801,7 +3822,7 @@ static const struct opcode opcode_table[256] = {
> >  	D(ImplicitOps | No64), II(ImplicitOps, em_iret, iret),
> >  	/* 0xD0 - 0xD7 */
> >  	D2bv(DstMem | SrcOne | ModRM), D2bv(DstMem | ModRM),
> > -	N, N, N, N,
> > +	N, I(DstAcc | SrcImmByte | No64, em_aad), N, N,
> >  	/* 0xD8 - 0xDF */
> >  	N, N, N, N, N, N, N, N,
> >  	/* 0xE0 - 0xE7 */
> > --
> > 			Gleb.
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 

--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Gleb Natapov Dec. 14, 2012, 12:42 p.m. UTC | #3

On Fri, Dec 14, 2012 at 01:29:19PM +0200, Gleb Natapov wrote:
> On Fri, Dec 14, 2012 at 11:44:22AM +0100, Paolo Bonzini wrote:
> > Il 10/12/2012 10:42, Gleb Natapov ha scritto:
> > > Windows2000 uses it during boot. This fixes
> > > https://bugzilla.kernel.org/show_bug.cgi?id=50921
> > > 
> > > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> > > index 39171cb..92c7292 100644
> > > --- a/arch/x86/kvm/emulate.c
> > > +++ b/arch/x86/kvm/emulate.c
> > > @@ -2852,6 +2852,27 @@ static int em_das(struct x86_emulate_ctxt *ctxt)
> > >  	return X86EMUL_CONTINUE;
> > >  }
> > >  
> > > +static int em_aad(struct x86_emulate_ctxt *ctxt)
> > > +{
> > > +	u8 al = ctxt->dst.val & 0xff;
> > > +	u8 ah = (ctxt->dst.val >> 8) & 0xff;
> > > +
> > > +	al = (al + (ah * ctxt->src.val)) & 0xff;
> > > +
> > > +	ctxt->dst.val = (ctxt->dst.val & 0xffff0000) | al;
> > > +
> > > +	ctxt->eflags &= ~(X86_EFLAGS_PF | X86_EFLAGS_SF | X86_EFLAGS_ZF);
> > > +
> > > +	if (!al)
> > > +		ctxt->eflags |= X86_EFLAGS_ZF;
> > > +	if (!(al & 1))
> > > +		ctxt->eflags |= X86_EFLAGS_PF;
> > 
> > This is wrong, it should check the parity of al (even=1, odd=0).
> > 
> Oops, yes it should check number of bits set, not value itself.
> 
> > Perhaps you can use the trick of em_das:
> > 
> >         /* Set PF, ZF, SF */
> >         ctxt->src.type = OP_IMM;
> >         ctxt->src.val = 0;
> >         ctxt->src.bytes = 1;
> >         emulate_2op_SrcV(ctxt, "or");
> Will do.
> 
The patch is applied already. Paolo do you mind to send the fix?

--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paolo Bonzini Dec. 14, 2012, 1:33 p.m. UTC | #4

Il 14/12/2012 13:42, Gleb Natapov ha scritto:
> On Fri, Dec 14, 2012 at 01:29:19PM +0200, Gleb Natapov wrote:
>> On Fri, Dec 14, 2012 at 11:44:22AM +0100, Paolo Bonzini wrote:
>>> Il 10/12/2012 10:42, Gleb Natapov ha scritto:
>>>> Windows2000 uses it during boot. This fixes
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=50921
>>>>
>>>> Signed-off-by: Gleb Natapov <gleb@redhat.com>
>>>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>>>> index 39171cb..92c7292 100644
>>>> --- a/arch/x86/kvm/emulate.c
>>>> +++ b/arch/x86/kvm/emulate.c
>>>> @@ -2852,6 +2852,27 @@ static int em_das(struct x86_emulate_ctxt *ctxt)
>>>>  	return X86EMUL_CONTINUE;
>>>>  }
>>>>  
>>>> +static int em_aad(struct x86_emulate_ctxt *ctxt)
>>>> +{
>>>> +	u8 al = ctxt->dst.val & 0xff;
>>>> +	u8 ah = (ctxt->dst.val >> 8) & 0xff;
>>>> +
>>>> +	al = (al + (ah * ctxt->src.val)) & 0xff;
>>>> +
>>>> +	ctxt->dst.val = (ctxt->dst.val & 0xffff0000) | al;
>>>> +
>>>> +	ctxt->eflags &= ~(X86_EFLAGS_PF | X86_EFLAGS_SF | X86_EFLAGS_ZF);
>>>> +
>>>> +	if (!al)
>>>> +		ctxt->eflags |= X86_EFLAGS_ZF;
>>>> +	if (!(al & 1))
>>>> +		ctxt->eflags |= X86_EFLAGS_PF;
>>>
>>> This is wrong, it should check the parity of al (even=1, odd=0).
>>>
>> Oops, yes it should check number of bits set, not value itself.
>>
>>> Perhaps you can use the trick of em_das:
>>>
>>>         /* Set PF, ZF, SF */
>>>         ctxt->src.type = OP_IMM;
>>>         ctxt->src.val = 0;
>>>         ctxt->src.bytes = 1;
>>>         emulate_2op_SrcV(ctxt, "or");
>> Will do.
>>
> The patch is applied already. Paolo do you mind to send the fix?

Ok, I'll also add a testcase.

Paolo

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 39171cb..92c7292 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2852,6 +2852,27 @@  static int em_das(struct x86_emulate_ctxt *ctxt)
 	return X86EMUL_CONTINUE;
 }
 
+static int em_aad(struct x86_emulate_ctxt *ctxt)
+{
+	u8 al = ctxt->dst.val & 0xff;
+	u8 ah = (ctxt->dst.val >> 8) & 0xff;
+
+	al = (al + (ah * ctxt->src.val)) & 0xff;
+
+	ctxt->dst.val = (ctxt->dst.val & 0xffff0000) | al;
+
+	ctxt->eflags &= ~(X86_EFLAGS_PF | X86_EFLAGS_SF | X86_EFLAGS_ZF);
+
+	if (!al)
+		ctxt->eflags |= X86_EFLAGS_ZF;
+	if (!(al & 1))
+		ctxt->eflags |= X86_EFLAGS_PF;
+	if (al & 0x80)
+		ctxt->eflags |= X86_EFLAGS_SF;
+
+	return X86EMUL_CONTINUE;
+}
+
 static int em_call(struct x86_emulate_ctxt *ctxt)
 {
 	long rel = ctxt->src.val;
@@ -3801,7 +3822,7 @@  static const struct opcode opcode_table[256] = {
 	D(ImplicitOps | No64), II(ImplicitOps, em_iret, iret),
 	/* 0xD0 - 0xD7 */
 	D2bv(DstMem | SrcOne | ModRM), D2bv(DstMem | ModRM),
-	N, N, N, N,
+	N, I(DstAcc | SrcImmByte | No64, em_aad), N, N,
 	/* 0xD8 - 0xDF */
 	N, N, N, N, N, N, N, N,
 	/* 0xE0 - 0xE7 */

KVM: emulator: implement AAD instruction

Commit Message

Comments

Patch