[PATCH] x86 emulator: Update EIP even with instructions with no writeback

[PATCH] x86 emulator: Update EIP even with instructions with no writeback

[Mohammed Gamal]

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:

 

Re: [PATCH] x86 emulator: Update EIP even with instructions with no writeback

[Avi Kivity]

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

Re: [PATCH] x86 emulator: Update EIP even with instructions with no writeback

[Mohammed Gamal]

[-- 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;

Re: [PATCH] x86 emulator: Update EIP even with instructions with no writeback

[Avi Kivity]

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

Re: [PATCH] x86 emulator: Update EIP even with instructions with no writeback

[Mohammed Gamal]

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.