2013-10-31 10:29:53

by Paolo Bonzini

[permalink] [raw]
Subject: [PATCH] KVM: x86: emulate SAHF instruction

Yet another instruction that we fail to emulate, this time found
in Windows 2008R2 32-bit.

Cc: [email protected]
Signed-off-by: Paolo Bonzini <[email protected]>
---
Testcase on its way. BTW, lahf/sahf is another candidate for
#UD emulation.

arch/x86/kvm/emulate.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 8e2a07bd8eac..ef750e75c930 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3296,6 +3296,18 @@ static int em_cpuid(struct x86_emulate_ctxt *ctxt)
return X86EMUL_CONTINUE;
}

+static int em_sahf(struct x86_emulate_ctxt *ctxt)
+{
+ u32 flags;
+
+ flags = EFLG_CF | EFLG_PF | EFLG_AF | EFLG_ZF | EFLG_SF;
+ flags &= *reg_rmw(ctxt, VCPU_REGS_RAX) >> 8;
+
+ ctxt->eflags &= ~0xffUL;
+ ctxt->eflags |= flags | X86_EFLAGS_FIXED;
+ return X86EMUL_CONTINUE;
+}
+
static int em_lahf(struct x86_emulate_ctxt *ctxt)
{
*reg_rmw(ctxt, VCPU_REGS_RAX) &= ~0xff00UL;
@@ -3788,7 +3800,7 @@ static const struct opcode opcode_table[256] = {
DI(SrcAcc | DstReg, pause), X7(D(SrcAcc | DstReg)),
/* 0x98 - 0x9F */
D(DstAcc | SrcNone), I(ImplicitOps | SrcAcc, em_cwd),
- I(SrcImmFAddr | No64, em_call_far), N,
+ I(SrcImmFAddr | No64, em_call_far), I(ImplicitOps, em_sahf),
II(ImplicitOps | Stack, em_pushf, pushf),
II(ImplicitOps | Stack, em_popf, popf), N, I(ImplicitOps, em_lahf),
/* 0xA0 - 0xA7 */
--
1.8.3.1


2013-10-31 14:21:23

by Gleb Natapov

[permalink] [raw]
Subject: Re: [PATCH] KVM: x86: emulate SAHF instruction

On Thu, Oct 31, 2013 at 11:29:42AM +0100, Paolo Bonzini wrote:
> Yet another instruction that we fail to emulate, this time found
> in Windows 2008R2 32-bit.
>
> Cc: [email protected]
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> Testcase on its way. BTW, lahf/sahf is another candidate for
> #UD emulation.
>
> arch/x86/kvm/emulate.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 8e2a07bd8eac..ef750e75c930 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -3296,6 +3296,18 @@ static int em_cpuid(struct x86_emulate_ctxt *ctxt)
> return X86EMUL_CONTINUE;
> }
>
> +static int em_sahf(struct x86_emulate_ctxt *ctxt)
> +{
> + u32 flags;
> +
Shouldn't we check CPUID.80000001H.ECX[0] = 1 in 64 bit mode?

> + flags = EFLG_CF | EFLG_PF | EFLG_AF | EFLG_ZF | EFLG_SF;
> + flags &= *reg_rmw(ctxt, VCPU_REGS_RAX) >> 8;
> +
> + ctxt->eflags &= ~0xffUL;
> + ctxt->eflags |= flags | X86_EFLAGS_FIXED;
> + return X86EMUL_CONTINUE;
> +}
> +
> static int em_lahf(struct x86_emulate_ctxt *ctxt)
> {
> *reg_rmw(ctxt, VCPU_REGS_RAX) &= ~0xff00UL;
> @@ -3788,7 +3800,7 @@ static const struct opcode opcode_table[256] = {
> DI(SrcAcc | DstReg, pause), X7(D(SrcAcc | DstReg)),
> /* 0x98 - 0x9F */
> D(DstAcc | SrcNone), I(ImplicitOps | SrcAcc, em_cwd),
> - I(SrcImmFAddr | No64, em_call_far), N,
> + I(SrcImmFAddr | No64, em_call_far), I(ImplicitOps, em_sahf),
> II(ImplicitOps | Stack, em_pushf, pushf),
> II(ImplicitOps | Stack, em_popf, popf), N, I(ImplicitOps, em_lahf),
> /* 0xA0 - 0xA7 */
> --
> 1.8.3.1

--
Gleb.

2013-10-31 14:27:54

by Paolo Bonzini

[permalink] [raw]
Subject: Re: [PATCH] KVM: x86: emulate SAHF instruction

Il 31/10/2013 15:21, Gleb Natapov ha scritto:
> On Thu, Oct 31, 2013 at 11:29:42AM +0100, Paolo Bonzini wrote:
>> Yet another instruction that we fail to emulate, this time found
>> in Windows 2008R2 32-bit.
>>
>> Cc: [email protected]
>> Signed-off-by: Paolo Bonzini <[email protected]>
>> ---
>> Testcase on its way. BTW, lahf/sahf is another candidate for
>> #UD emulation.
>>
>> arch/x86/kvm/emulate.c | 14 +++++++++++++-
>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index 8e2a07bd8eac..ef750e75c930 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -3296,6 +3296,18 @@ static int em_cpuid(struct x86_emulate_ctxt *ctxt)
>> return X86EMUL_CONTINUE;
>> }
>>
>> +static int em_sahf(struct x86_emulate_ctxt *ctxt)
>> +{
>> + u32 flags;
>> +
> Shouldn't we check CPUID.80000001H.ECX[0] = 1 in 64 bit mode?

If we want to we should check for it in em_lahf too. But we don't
usually check for CPUID bits. The recently added movbe is an exception,
and syscall too, but we don't do that for SSE or MMX instructions.

The way I understand it, either AMD was lazy, or they wanted to use
lahf/sahf as prefixes later on. But it didn't work out that way, so I
think it's fine to skip the check.

Paolo

>> + flags = EFLG_CF | EFLG_PF | EFLG_AF | EFLG_ZF | EFLG_SF;
>> + flags &= *reg_rmw(ctxt, VCPU_REGS_RAX) >> 8;
>> +
>> + ctxt->eflags &= ~0xffUL;
>> + ctxt->eflags |= flags | X86_EFLAGS_FIXED;
>> + return X86EMUL_CONTINUE;
>> +}
>> +
>> static int em_lahf(struct x86_emulate_ctxt *ctxt)
>> {
>> *reg_rmw(ctxt, VCPU_REGS_RAX) &= ~0xff00UL;
>> @@ -3788,7 +3800,7 @@ static const struct opcode opcode_table[256] = {
>> DI(SrcAcc | DstReg, pause), X7(D(SrcAcc | DstReg)),
>> /* 0x98 - 0x9F */
>> D(DstAcc | SrcNone), I(ImplicitOps | SrcAcc, em_cwd),
>> - I(SrcImmFAddr | No64, em_call_far), N,
>> + I(SrcImmFAddr | No64, em_call_far), I(ImplicitOps, em_sahf),
>> II(ImplicitOps | Stack, em_pushf, pushf),
>> II(ImplicitOps | Stack, em_popf, popf), N, I(ImplicitOps, em_lahf),
>> /* 0xA0 - 0xA7 */
>> --
>> 1.8.3.1
>
> --
> Gleb.
>

2013-10-31 14:34:10

by Gleb Natapov

[permalink] [raw]
Subject: Re: [PATCH] KVM: x86: emulate SAHF instruction

On Thu, Oct 31, 2013 at 03:27:48PM +0100, Paolo Bonzini wrote:
> Il 31/10/2013 15:21, Gleb Natapov ha scritto:
> > On Thu, Oct 31, 2013 at 11:29:42AM +0100, Paolo Bonzini wrote:
> >> Yet another instruction that we fail to emulate, this time found
> >> in Windows 2008R2 32-bit.
> >>
> >> Cc: [email protected]
> >> Signed-off-by: Paolo Bonzini <[email protected]>
> >> ---
> >> Testcase on its way. BTW, lahf/sahf is another candidate for
> >> #UD emulation.
> >>
> >> arch/x86/kvm/emulate.c | 14 +++++++++++++-
> >> 1 file changed, 13 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> >> index 8e2a07bd8eac..ef750e75c930 100644
> >> --- a/arch/x86/kvm/emulate.c
> >> +++ b/arch/x86/kvm/emulate.c
> >> @@ -3296,6 +3296,18 @@ static int em_cpuid(struct x86_emulate_ctxt *ctxt)
> >> return X86EMUL_CONTINUE;
> >> }
> >>
> >> +static int em_sahf(struct x86_emulate_ctxt *ctxt)
> >> +{
> >> + u32 flags;
> >> +
> > Shouldn't we check CPUID.80000001H.ECX[0] = 1 in 64 bit mode?
>
> If we want to we should check for it in em_lahf too. But we don't
Right.

> usually check for CPUID bits. The recently added movbe is an exception,
> and syscall too, but we don't do that for SSE or MMX instructions.
>
> The way I understand it, either AMD was lazy, or they wanted to use
> lahf/sahf as prefixes later on. But it didn't work out that way, so I
> think it's fine to skip the check.
>
I haven't checked AMD doc, but if it is documented that lahf/sahf #UDs at 64
bit we should emulate it correctly. Who knows what code depends on it.
Of course I pretty much doubt we will ever emulate sahf in 64 bit mode
:)

> Paolo
>
> >> + flags = EFLG_CF | EFLG_PF | EFLG_AF | EFLG_ZF | EFLG_SF;
> >> + flags &= *reg_rmw(ctxt, VCPU_REGS_RAX) >> 8;
> >> +
> >> + ctxt->eflags &= ~0xffUL;
> >> + ctxt->eflags |= flags | X86_EFLAGS_FIXED;
> >> + return X86EMUL_CONTINUE;
> >> +}
> >> +
> >> static int em_lahf(struct x86_emulate_ctxt *ctxt)
> >> {
> >> *reg_rmw(ctxt, VCPU_REGS_RAX) &= ~0xff00UL;
> >> @@ -3788,7 +3800,7 @@ static const struct opcode opcode_table[256] = {
> >> DI(SrcAcc | DstReg, pause), X7(D(SrcAcc | DstReg)),
> >> /* 0x98 - 0x9F */
> >> D(DstAcc | SrcNone), I(ImplicitOps | SrcAcc, em_cwd),
> >> - I(SrcImmFAddr | No64, em_call_far), N,
> >> + I(SrcImmFAddr | No64, em_call_far), I(ImplicitOps, em_sahf),
> >> II(ImplicitOps | Stack, em_pushf, pushf),
> >> II(ImplicitOps | Stack, em_popf, popf), N, I(ImplicitOps, em_lahf),
> >> /* 0xA0 - 0xA7 */
> >> --
> >> 1.8.3.1
> >
> > --
> > Gleb.
> >

--
Gleb.

2013-10-31 14:49:10

by Paolo Bonzini

[permalink] [raw]
Subject: Re: [PATCH] KVM: x86: emulate SAHF instruction

Il 31/10/2013 15:34, Gleb Natapov ha scritto:
> I haven't checked AMD doc, but if it is documented that lahf/sahf #UDs at 64
> bit we should emulate it correctly.

It says "The LAHF instruction can only be executed in 64-bit mode if
supported by the processor implementation. Check the status of ECX bit 0
returned by CPUID function 8000_0001h to verify that the processor
supports LAHF in 64-bit mode". Same as Intel---in fact 80000001h is an
"AMD leaf" so to speak.

I found "AMD introduced support for the instructions with their Athlon
64, Opteron and Turion 64 revision D processors in March 2005 and Intel
introduced support for the instructions with the Pentium 4 G1 stepping
in December 2005". I think we can for all practical purposes ignore the
lahf_lm CPUID flag.

> Who knows what code depends on it.
> Of course I pretty much doubt we will ever emulate sahf in 64 bit mode

Yep.

Paolo

2013-10-31 15:14:26

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH] KVM: x86: emulate SAHF instruction

On Thu, Oct 31, 2013 at 03:49:04PM +0100, Paolo Bonzini wrote:
> Il 31/10/2013 15:34, Gleb Natapov ha scritto:
> > I haven't checked AMD doc, but if it is documented that lahf/sahf #UDs at 64
> > bit we should emulate it correctly.
>
> It says "The LAHF instruction can only be executed in 64-bit mode if
> supported by the processor implementation. Check the status of ECX bit 0
> returned by CPUID function 8000_0001h to verify that the processor
> supports LAHF in 64-bit mode". Same as Intel---in fact 80000001h is an
> "AMD leaf" so to speak.

Yes, we #UD if L/SAHF are not supported:

Invalid opcode, The LAHF instruction is not supported, as indicated by CPUID
#UD Fn8000_0001_ECX[LahfSahf] = 0.

> I found "AMD introduced support for the instructions with their Athlon
> 64, Opteron and Turion 64 revision D processors in March 2005 and Intel
> introduced support for the instructions with the Pentium 4 G1 stepping
> in December 2005". I think we can for all practical purposes ignore the
> lahf_lm CPUID flag.
>
> > Who knows what code depends on it.

I remember an issue where we had to turn off the LAHF_LM CPUID bit for
certain K8s because otherwise the flashplayer would SIGSEGV as it was
trying to execute LAHF but the CPU was not really supporting it although
CPUID said so :). See fbd8b1819e80a and 6b0f43ddfa358.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

2013-10-31 15:47:14

by Gleb Natapov

[permalink] [raw]
Subject: Re: [PATCH] KVM: x86: emulate SAHF instruction

On Thu, Oct 31, 2013 at 03:49:04PM +0100, Paolo Bonzini wrote:
> Il 31/10/2013 15:34, Gleb Natapov ha scritto:
> > I haven't checked AMD doc, but if it is documented that lahf/sahf #UDs at 64
> > bit we should emulate it correctly.
>
> It says "The LAHF instruction can only be executed in 64-bit mode if
> supported by the processor implementation. Check the status of ECX bit 0
> returned by CPUID function 8000_0001h to verify that the processor
> supports LAHF in 64-bit mode". Same as Intel---in fact 80000001h is an
> "AMD leaf" so to speak.
>
> I found "AMD introduced support for the instructions with their Athlon
> 64, Opteron and Turion 64 revision D processors in March 2005 and Intel
> introduced support for the instructions with the Pentium 4 G1 stepping
> in December 2005". I think we can for all practical purposes ignore the
> lahf_lm CPUID flag.
>
Yep,

Reviewed-by: Gleb Natapov <[email protected]>

--
Gleb.