* [PATCH] KVM: X86: Remove stale values from ctxt->memop before emulation
@ 2012-05-04 16:14 Joerg Roedel
2012-05-06 8:21 ` Avi Kivity
0 siblings, 1 reply; 6+ messages in thread
From: Joerg Roedel @ 2012-05-04 16:14 UTC (permalink / raw)
To: Avi Kivity, Marcelo Tosatti; +Cc: kvm, linux-kernel, Joerg Roedel
When instruction decoding begins there could be stale values
in the ctxt->memop structure. This causes problems when an
instruction is emulated with more op-bytes then the guest
wants (like the bsr instruction which is always emulated
with 4 or 8 op-bytes).
The stale value in this structure causes the unit-test for
the bsrw instruction to fail. Initialize the memop.val with
0 to prevent such bugs (an alternative fix could be to
always emulate instructions with the number of op-bytes
requested by the guest).
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
arch/x86/kvm/emulate.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index d4bf50c..1b516ec 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3937,6 +3937,7 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
struct opcode opcode;
ctxt->memop.type = OP_NONE;
+ ctxt->memop.val = 0;
ctxt->memopp = NULL;
ctxt->_eip = ctxt->eip;
ctxt->fetch.start = ctxt->_eip;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: X86: Remove stale values from ctxt->memop before emulation
2012-05-04 16:14 [PATCH] KVM: X86: Remove stale values from ctxt->memop before emulation Joerg Roedel
@ 2012-05-06 8:21 ` Avi Kivity
2012-05-07 10:12 ` Joerg Roedel
0 siblings, 1 reply; 6+ messages in thread
From: Avi Kivity @ 2012-05-06 8:21 UTC (permalink / raw)
To: Joerg Roedel; +Cc: Marcelo Tosatti, kvm, linux-kernel
On 05/04/2012 07:14 PM, Joerg Roedel wrote:
> When instruction decoding begins there could be stale values
> in the ctxt->memop structure. This causes problems when an
> instruction is emulated with more op-bytes then the guest
> wants (like the bsr instruction which is always emulated
> with 4 or 8 op-bytes).
>
> The stale value in this structure causes the unit-test for
> the bsrw instruction to fail. Initialize the memop.val with
> 0 to prevent such bugs (an alternative fix could be to
> always emulate instructions with the number of op-bytes
> requested by the guest).
>
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index d4bf50c..1b516ec 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -3937,6 +3937,7 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
> struct opcode opcode;
>
> ctxt->memop.type = OP_NONE;
> + ctxt->memop.val = 0;
> ctxt->memopp = NULL;
> ctxt->_eip = ctxt->eip;
> ctxt->fetch.start = ctxt->_eip;
This only works for long sized values - it doesn't initialize val64 on
i386, for example. So I think it's better to change bsr (and family) to
use emualte_2op_SrcV_nobyte() instead (which has the added benefit of
using the same values as the processor for the "undefined" bits).
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: X86: Remove stale values from ctxt->memop before emulation
2012-05-06 8:21 ` Avi Kivity
@ 2012-05-07 10:12 ` Joerg Roedel
2012-05-07 10:18 ` Avi Kivity
2012-05-14 8:32 ` Avi Kivity
0 siblings, 2 replies; 6+ messages in thread
From: Joerg Roedel @ 2012-05-07 10:12 UTC (permalink / raw)
To: Avi Kivity; +Cc: Marcelo Tosatti, kvm, linux-kernel
On Sun, May 06, 2012 at 11:21:52AM +0300, Avi Kivity wrote:
> > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> > index d4bf50c..1b516ec 100644
> > --- a/arch/x86/kvm/emulate.c
> > +++ b/arch/x86/kvm/emulate.c
> > @@ -3937,6 +3937,7 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
> > struct opcode opcode;
> >
> > ctxt->memop.type = OP_NONE;
> > + ctxt->memop.val = 0;
> > ctxt->memopp = NULL;
> > ctxt->_eip = ctxt->eip;
> > ctxt->fetch.start = ctxt->_eip;
>
> This only works for long sized values - it doesn't initialize val64 on
> i386, for example. So I think it's better to change bsr (and family) to
> use emualte_2op_SrcV_nobyte() instead (which has the added benefit of
> using the same values as the processor for the "undefined" bits).
Right, thats a better solution. How about the attached patch? The zf
check shouldn't be necessary anymore because the generated assembly uses
dst.val as input and output so writeback shouldn't do anything wrong.
The bsr and bsf unittests all pass again with this patch.
Joerg
>From e9262f18e90111d32b584084c0b5564cbd728d65 Mon Sep 17 00:00:00 2001
From: Joerg Roedel <joerg.roedel@amd.com>
Date: Mon, 7 May 2012 12:05:28 +0200
Subject: [PATCH] KVM: X86: convert bsf/bsr instructions to
emulate_2op_SrcV_nobyte()
The instruction emulation for bsrw is broken in KVM because
the code always uses bsr with 32 or 64 bit operand size for
emulation. Fix that by using emulate_2op_SrcV_nobyte() macro
to use guest operand size for emulation.
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
arch/x86/kvm/emulate.c | 26 ++------------------------
1 file changed, 2 insertions(+), 24 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 0d151e2..a6f8488 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3134,35 +3134,13 @@ static int em_btc(struct x86_emulate_ctxt *ctxt)
static int em_bsf(struct x86_emulate_ctxt *ctxt)
{
- u8 zf;
-
- __asm__ ("bsf %2, %0; setz %1"
- : "=r"(ctxt->dst.val), "=q"(zf)
- : "r"(ctxt->src.val));
-
- ctxt->eflags &= ~X86_EFLAGS_ZF;
- if (zf) {
- ctxt->eflags |= X86_EFLAGS_ZF;
- /* Disable writeback. */
- ctxt->dst.type = OP_NONE;
- }
+ emulate_2op_SrcV_nobyte(ctxt, "bsf");
return X86EMUL_CONTINUE;
}
static int em_bsr(struct x86_emulate_ctxt *ctxt)
{
- u8 zf;
-
- __asm__ ("bsr %2, %0; setz %1"
- : "=r"(ctxt->dst.val), "=q"(zf)
- : "r"(ctxt->src.val));
-
- ctxt->eflags &= ~X86_EFLAGS_ZF;
- if (zf) {
- ctxt->eflags |= X86_EFLAGS_ZF;
- /* Disable writeback. */
- ctxt->dst.type = OP_NONE;
- }
+ emulate_2op_SrcV_nobyte(ctxt, "bsr");
return X86EMUL_CONTINUE;
}
--
1.7.9.5
--
AMD Operating System Research Center
Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: X86: Remove stale values from ctxt->memop before emulation
2012-05-07 10:12 ` Joerg Roedel
@ 2012-05-07 10:18 ` Avi Kivity
2012-05-07 10:43 ` Joerg Roedel
2012-05-14 8:32 ` Avi Kivity
1 sibling, 1 reply; 6+ messages in thread
From: Avi Kivity @ 2012-05-07 10:18 UTC (permalink / raw)
To: Joerg Roedel; +Cc: Marcelo Tosatti, kvm, linux-kernel
On 05/07/2012 01:12 PM, Joerg Roedel wrote:
> On Sun, May 06, 2012 at 11:21:52AM +0300, Avi Kivity wrote:
> > > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> > > index d4bf50c..1b516ec 100644
> > > --- a/arch/x86/kvm/emulate.c
> > > +++ b/arch/x86/kvm/emulate.c
> > > @@ -3937,6 +3937,7 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
> > > struct opcode opcode;
> > >
> > > ctxt->memop.type = OP_NONE;
> > > + ctxt->memop.val = 0;
> > > ctxt->memopp = NULL;
> > > ctxt->_eip = ctxt->eip;
> > > ctxt->fetch.start = ctxt->_eip;
> >
> > This only works for long sized values - it doesn't initialize val64 on
> > i386, for example. So I think it's better to change bsr (and family) to
> > use emualte_2op_SrcV_nobyte() instead (which has the added benefit of
> > using the same values as the processor for the "undefined" bits).
>
> Right, thats a better solution. How about the attached patch? The zf
> check shouldn't be necessary anymore because the generated assembly uses
> dst.val as input and output so writeback shouldn't do anything wrong.
> The bsr and bsf unittests all pass again with this patch.
>
> Joerg
>
> From e9262f18e90111d32b584084c0b5564cbd728d65 Mon Sep 17 00:00:00 2001
> From: Joerg Roedel <joerg.roedel@amd.com>
> Date: Mon, 7 May 2012 12:05:28 +0200
> Subject: [PATCH] KVM: X86: convert bsf/bsr instructions to
> emulate_2op_SrcV_nobyte()
>
> The instruction emulation for bsrw is broken in KVM because
> the code always uses bsr with 32 or 64 bit operand size for
> emulation. Fix that by using emulate_2op_SrcV_nobyte() macro
> to use guest operand size for emulation.
>
It looks fine. Do you know what triggered this regression? (for
figuring out if it's 3.4 material)
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: X86: Remove stale values from ctxt->memop before emulation
2012-05-07 10:18 ` Avi Kivity
@ 2012-05-07 10:43 ` Joerg Roedel
0 siblings, 0 replies; 6+ messages in thread
From: Joerg Roedel @ 2012-05-07 10:43 UTC (permalink / raw)
To: Avi Kivity; +Cc: Marcelo Tosatti, kvm, linux-kernel
On Mon, May 07, 2012 at 01:18:01PM +0300, Avi Kivity wrote:
> On 05/07/2012 01:12 PM, Joerg Roedel wrote:
> >
> > The instruction emulation for bsrw is broken in KVM because
> > the code always uses bsr with 32 or 64 bit operand size for
> > emulation. Fix that by using emulate_2op_SrcV_nobyte() macro
> > to use guest operand size for emulation.
> >
>
> It looks fine. Do you know what triggered this regression? (for
> figuring out if it's 3.4 material)
Looks like it is 3.4 (and -stable) material. I tested a few older
kernels and the test passes on 3.0 but fails on 3.2 an later kernels
(I have not tested 3.1).
Joerg
--
AMD Operating System Research Center
Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: X86: Remove stale values from ctxt->memop before emulation
2012-05-07 10:12 ` Joerg Roedel
2012-05-07 10:18 ` Avi Kivity
@ 2012-05-14 8:32 ` Avi Kivity
1 sibling, 0 replies; 6+ messages in thread
From: Avi Kivity @ 2012-05-14 8:32 UTC (permalink / raw)
To: Joerg Roedel; +Cc: Marcelo Tosatti, kvm, linux-kernel
On 05/07/2012 01:12 PM, Joerg Roedel wrote:
> From e9262f18e90111d32b584084c0b5564cbd728d65 Mon Sep 17 00:00:00 2001
> From: Joerg Roedel <joerg.roedel@amd.com>
> Date: Mon, 7 May 2012 12:05:28 +0200
> Subject: [PATCH] KVM: X86: convert bsf/bsr instructions to
> emulate_2op_SrcV_nobyte()
>
> The instruction emulation for bsrw is broken in KVM because
> the code always uses bsr with 32 or 64 bit operand size for
> emulation. Fix that by using emulate_2op_SrcV_nobyte() macro
> to use guest operand size for emulation.
>
>
Applied, thanks.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-05-14 8:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-04 16:14 [PATCH] KVM: X86: Remove stale values from ctxt->memop before emulation Joerg Roedel
2012-05-06 8:21 ` Avi Kivity
2012-05-07 10:12 ` Joerg Roedel
2012-05-07 10:18 ` Avi Kivity
2012-05-07 10:43 ` Joerg Roedel
2012-05-14 8:32 ` Avi Kivity
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).