From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH] x86 emulator: Update EIP even with instructions with no writeback Date: Sun, 06 Jul 2008 16:34:54 +0300 Message-ID: <4870C9FE.5070606@qumranet.com> References: <20080705191421.GA28785@mohd-laptop> <48707989.1050003@qumranet.com> <52d4a3890807060626g4c231ccem3e40798e15d8a65a@mail.gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, riel@surriel.com To: Mohammed Gamal Return-path: Received: from il.qumranet.com ([212.179.150.194]:14492 "EHLO il.qumranet.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753915AbYGFNez (ORCPT ); Sun, 6 Jul 2008 09:34:55 -0400 In-Reply-To: <52d4a3890807060626g4c231ccem3e40798e15d8a65a@mail.gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: Mohammed Gamal wrote: > On Sun, Jul 6, 2008 at 10:51 AM, Avi Kivity 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 >>> >>> --- >>> >>> 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