* [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).