* [PATCH] x86 emulator: Update EIP even with instructions with no writeback @ 2008-07-05 19:14 Mohammed Gamal 2008-07-06 7:51 ` Avi Kivity 0 siblings, 1 reply; 5+ messages in thread From: Mohammed Gamal @ 2008-07-05 19:14 UTC (permalink / raw) To: kvm; +Cc: avi, riel This patch resolves the problem encountered with HLT emulation with FreeDOS's HIMEM XMS Driver. HLT is the only instruction that goes to the done label unconditionally, causing the EIP value not to be updated which leads to the guest looping forever on the same instruction. Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com> --- arch/x86/kvm/x86_emulate.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c index dd4efe1..04d7f02 100644 --- a/arch/x86/kvm/x86_emulate.c +++ b/arch/x86/kvm/x86_emulate.c @@ -1769,13 +1769,15 @@ writeback: /* Commit shadow register state. */ memcpy(ctxt->vcpu->arch.regs, c->regs, sizeof c->regs); - kvm_rip_write(ctxt->vcpu, c->eip); done: if (rc == X86EMUL_UNHANDLEABLE) { c->eip = saved_eip; return -1; } + else + kvm_rip_write(ctxt->vcpu, c->eip); + return 0; twobyte_insn: ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] x86 emulator: Update EIP even with instructions with no writeback 2008-07-05 19:14 [PATCH] x86 emulator: Update EIP even with instructions with no writeback Mohammed Gamal @ 2008-07-06 7:51 ` Avi Kivity 2008-07-06 13:26 ` Mohammed Gamal 0 siblings, 1 reply; 5+ messages in thread From: Avi Kivity @ 2008-07-06 7:51 UTC (permalink / raw) To: Mohammed Gamal; +Cc: kvm, riel Mohammed Gamal wrote: > This patch resolves the problem encountered with HLT emulation with FreeDOS's HIMEM XMS Driver. > > HLT is the only instruction that goes to the done label unconditionally, > causing the EIP value not to be updated which leads to the guest looping > forever on the same instruction. > > Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com> > > --- > > arch/x86/kvm/x86_emulate.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c > index dd4efe1..04d7f02 100644 > --- a/arch/x86/kvm/x86_emulate.c > +++ b/arch/x86/kvm/x86_emulate.c > @@ -1769,13 +1769,15 @@ writeback: > > /* Commit shadow register state. */ > memcpy(ctxt->vcpu->arch.regs, c->regs, sizeof c->regs); > - kvm_rip_write(ctxt->vcpu, c->eip); > > done: > if (rc == X86EMUL_UNHANDLEABLE) { > c->eip = saved_eip; > return -1; > } > + else > + kvm_rip_write(ctxt->vcpu, c->eip); > + > return 0; Why not change hlt to writeback like all other instructions? -- error compiling committee.c: too many arguments to function ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86 emulator: Update EIP even with instructions with no writeback 2008-07-06 7:51 ` Avi Kivity @ 2008-07-06 13:26 ` Mohammed Gamal 2008-07-06 13:34 ` Avi Kivity 0 siblings, 1 reply; 5+ messages in thread From: Mohammed Gamal @ 2008-07-06 13:26 UTC (permalink / raw) To: Avi Kivity; +Cc: kvm, riel [-- Attachment #1: Type: text/plain, Size: 1578 bytes --] On Sun, Jul 6, 2008 at 10:51 AM, Avi Kivity <avi@qumranet.com> wrote: > Mohammed Gamal wrote: >> >> This patch resolves the problem encountered with HLT emulation with >> FreeDOS's HIMEM XMS Driver. >> HLT is the only instruction that goes to the done label unconditionally, >> causing the EIP value not to be updated which leads to the guest looping >> forever on the same instruction. >> >> Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com> >> >> --- >> >> arch/x86/kvm/x86_emulate.c | 4 +++- >> 1 files changed, 3 insertions(+), 1 deletions(-) >> >> diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c >> index dd4efe1..04d7f02 100644 >> --- a/arch/x86/kvm/x86_emulate.c >> +++ b/arch/x86/kvm/x86_emulate.c >> @@ -1769,13 +1769,15 @@ writeback: >> /* Commit shadow register state. */ >> memcpy(ctxt->vcpu->arch.regs, c->regs, sizeof c->regs); >> - kvm_rip_write(ctxt->vcpu, c->eip); >> done: >> if (rc == X86EMUL_UNHANDLEABLE) { >> c->eip = saved_eip; >> return -1; >> } >> + else >> + kvm_rip_write(ctxt->vcpu, c->eip); >> + >> return 0; > > Why not change hlt to writeback like all other instructions? > IIRC hlt doesn't do writebacks. So, instead of changing hlt to go for a bogus writeback, I thought it'd be more logical that since we're going to the done label anyway we check first if the instruction is unhandleable, in which case we write the saved EIP, otherwise we update the EIP value. Anyway, here is a patch that changes hlt to writeback. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: hlt_bugfix.patch --] [-- Type: text/x-diff; name=hlt_bugfix.patch, Size: 496 bytes --] arch/x86/kvm/x86_emulate.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c index dd4efe1..62e71b6 100644 --- a/arch/x86/kvm/x86_emulate.c +++ b/arch/x86/kvm/x86_emulate.c @@ -1732,7 +1732,7 @@ special_insn: break; case 0xf4: /* hlt */ ctxt->vcpu->arch.halt_request = 1; - goto done; + break; case 0xf5: /* cmc */ /* complement carry flag from eflags reg */ ctxt->eflags ^= EFLG_CF; ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] x86 emulator: Update EIP even with instructions with no writeback 2008-07-06 13:26 ` Mohammed Gamal @ 2008-07-06 13:34 ` Avi Kivity 2008-07-06 13:45 ` Mohammed Gamal 0 siblings, 1 reply; 5+ messages in thread From: Avi Kivity @ 2008-07-06 13:34 UTC (permalink / raw) To: Mohammed Gamal; +Cc: kvm, riel Mohammed Gamal wrote: > On Sun, Jul 6, 2008 at 10:51 AM, Avi Kivity <avi@qumranet.com> wrote: > >> Mohammed Gamal wrote: >> >>> This patch resolves the problem encountered with HLT emulation with >>> FreeDOS's HIMEM XMS Driver. >>> HLT is the only instruction that goes to the done label unconditionally, >>> causing the EIP value not to be updated which leads to the guest looping >>> forever on the same instruction. >>> >>> Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com> >>> >>> --- >>> >>> arch/x86/kvm/x86_emulate.c | 4 +++- >>> 1 files changed, 3 insertions(+), 1 deletions(-) >>> >>> diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c >>> index dd4efe1..04d7f02 100644 >>> --- a/arch/x86/kvm/x86_emulate.c >>> +++ b/arch/x86/kvm/x86_emulate.c >>> @@ -1769,13 +1769,15 @@ writeback: >>> /* Commit shadow register state. */ >>> memcpy(ctxt->vcpu->arch.regs, c->regs, sizeof c->regs); >>> - kvm_rip_write(ctxt->vcpu, c->eip); >>> done: >>> if (rc == X86EMUL_UNHANDLEABLE) { >>> c->eip = saved_eip; >>> return -1; >>> } >>> + else >>> + kvm_rip_write(ctxt->vcpu, c->eip); >>> + >>> return 0; >>> >> Why not change hlt to writeback like all other instructions? >> >> > > IIRC hlt doesn't do writebacks. So, instead of changing hlt to go for > a bogus writeback, I thought it'd be more logical that since we're > going to the done label anyway we check first if the instruction is > unhandleable, in which case we write the saved EIP, otherwise we > update the EIP value. > It's not bogus, you have to write back the instruction pointer at least. It also helps having less code paths. > Anyway, here is a patch that changes hlt to writeback. > Does it solve the problem? If so, please provide an updated changelog entry and a signoff. -- error compiling committee.c: too many arguments to function ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86 emulator: Update EIP even with instructions with no writeback 2008-07-06 13:34 ` Avi Kivity @ 2008-07-06 13:45 ` Mohammed Gamal 0 siblings, 0 replies; 5+ messages in thread From: Mohammed Gamal @ 2008-07-06 13:45 UTC (permalink / raw) To: Avi Kivity; +Cc: kvm, riel On Sun, Jul 6, 2008 at 4:34 PM, Avi Kivity <avi@qumranet.com> wrote: > Mohammed Gamal wrote: >> >> On Sun, Jul 6, 2008 at 10:51 AM, Avi Kivity <avi@qumranet.com> wrote: >> >>> >>> Mohammed Gamal wrote: >>> >>>> >>>> This patch resolves the problem encountered with HLT emulation with >>>> FreeDOS's HIMEM XMS Driver. >>>> HLT is the only instruction that goes to the done label unconditionally, >>>> causing the EIP value not to be updated which leads to the guest looping >>>> forever on the same instruction. >>>> >>>> Signed-off-by: Mohammed Gamal <m.gamal005@gmail.com> >>>> >>>> --- >>>> >>>> arch/x86/kvm/x86_emulate.c | 4 +++- >>>> 1 files changed, 3 insertions(+), 1 deletions(-) >>>> >>>> diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c >>>> index dd4efe1..04d7f02 100644 >>>> --- a/arch/x86/kvm/x86_emulate.c >>>> +++ b/arch/x86/kvm/x86_emulate.c >>>> @@ -1769,13 +1769,15 @@ writeback: >>>> /* Commit shadow register state. */ >>>> memcpy(ctxt->vcpu->arch.regs, c->regs, sizeof c->regs); >>>> - kvm_rip_write(ctxt->vcpu, c->eip); >>>> done: >>>> if (rc == X86EMUL_UNHANDLEABLE) { >>>> c->eip = saved_eip; >>>> return -1; >>>> } >>>> + else >>>> + kvm_rip_write(ctxt->vcpu, c->eip); >>>> + >>>> return 0; >>>> >>> >>> Why not change hlt to writeback like all other instructions? >>> >>> >> >> IIRC hlt doesn't do writebacks. So, instead of changing hlt to go for >> a bogus writeback, I thought it'd be more logical that since we're >> going to the done label anyway we check first if the instruction is >> unhandleable, in which case we write the saved EIP, otherwise we >> update the EIP value. >> > > It's not bogus, you have to write back the instruction pointer at least. It > also helps having less code paths. > The instruction pointer would haven been written back anyway as we do go to the done label anyway. But I am convinced with your agrument. >> Anyway, here is a patch that changes hlt to writeback. >> > > Does it solve the problem? If so, please provide an updated changelog entry > and a signoff. Yes it does, I'll send the updated patch right now. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2008-07-06 13:45 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-07-05 19:14 [PATCH] x86 emulator: Update EIP even with instructions with no writeback Mohammed Gamal 2008-07-06 7:51 ` Avi Kivity 2008-07-06 13:26 ` Mohammed Gamal 2008-07-06 13:34 ` Avi Kivity 2008-07-06 13:45 ` Mohammed Gamal
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.