x86: Corrupted eflags in qemu's CPU state

x86: Corrupted eflags in qemu's CPU state

[Jan Kiszka]

Hi,

this line almost ruined my afternoon:

diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
index 01748ed..4ad386b 100644
--- a/qemu/qemu-kvm-x86.c
+++ b/qemu/qemu-kvm-x86.c
@@ -429,7 +429,6 @@ void kvm_arch_save_regs(CPUState *env)
     env->cc_src = env->eflags & (CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
     env->df = 1 - (2 * ((env->eflags >> 10) & 1));
     env->cc_op = CC_OP_EFLAGS;
-    env->eflags &= ~(DF_MASK | CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
 
     /* msrs */
     n = 0;

The guest flags reported via gdb or monitor were garbage and I first
didn't realized this...

git logs revealed that commit 6eecdc3eea74ead3c11b8b43d825d2cabe7a2456
once introduced it mid of 2006, but maybe under different boundary
conditions. At least today it appears to be plain wrong, eflags must
always contain to full state, cc_src, df & cc_op are just supplementary
states. Please correct me if I'm wrong, otherwise I will send out a
proper patch, also upstream as QEMU's kvm suffers from the same issue.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: x86: Corrupted eflags in qemu's CPU state

[Glauber Costa]

On Mon, Jan 26, 2009 at 1:49 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> Hi,
>
> this line almost ruined my afternoon:
There's a lesson for us to learn here: Always hack at night.

>
> diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
> index 01748ed..4ad386b 100644
> --- a/qemu/qemu-kvm-x86.c
> +++ b/qemu/qemu-kvm-x86.c
> @@ -429,7 +429,6 @@ void kvm_arch_save_regs(CPUState *env)
>     env->cc_src = env->eflags & (CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
>     env->df = 1 - (2 * ((env->eflags >> 10) & 1));
>     env->cc_op = CC_OP_EFLAGS;
> -    env->eflags &= ~(DF_MASK | CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
>
>     /* msrs */
>     n = 0;
>
> The guest flags reported via gdb or monitor were garbage and I first
> didn't realized this...
>
> git logs revealed that commit 6eecdc3eea74ead3c11b8b43d825d2cabe7a2456
> once introduced it mid of 2006, but maybe under different boundary
> conditions. At least today it appears to be plain wrong, eflags must
> always contain to full state, cc_src, df & cc_op are just supplementary
> states. Please correct me if I'm wrong, otherwise I will send out a
> proper patch, also upstream as QEMU's kvm suffers from the same issue.
>
> Jan

Have you tested this removing the other parts of it too?
I'd say there are unnecessary, and might well be harming other loads too.

There were once a time in which we executed kvm from inside qemu's cpu_exec
loop. Back then, it made some sense to mess with qemu flags. Right now,
I don't see any reason for us to ever touch it.

-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

[PATCH] x86-userspace: Remove eflags conversion into emulator format

[Jan Kiszka]

Glauber Costa wrote:
> On Mon, Jan 26, 2009 at 1:49 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> Hi,
>>
>> this line almost ruined my afternoon:
> There's a lesson for us to learn here: Always hack at night.

If the night wasn't that short...

> 
>> diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
>> index 01748ed..4ad386b 100644
>> --- a/qemu/qemu-kvm-x86.c
>> +++ b/qemu/qemu-kvm-x86.c
>> @@ -429,7 +429,6 @@ void kvm_arch_save_regs(CPUState *env)
>>     env->cc_src = env->eflags & (CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
>>     env->df = 1 - (2 * ((env->eflags >> 10) & 1));
>>     env->cc_op = CC_OP_EFLAGS;
>> -    env->eflags &= ~(DF_MASK | CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
>>
>>     /* msrs */
>>     n = 0;
>>
>> The guest flags reported via gdb or monitor were garbage and I first
>> didn't realized this...
>>
>> git logs revealed that commit 6eecdc3eea74ead3c11b8b43d825d2cabe7a2456
>> once introduced it mid of 2006, but maybe under different boundary
>> conditions. At least today it appears to be plain wrong, eflags must
>> always contain to full state, cc_src, df & cc_op are just supplementary
>> states. Please correct me if I'm wrong, otherwise I will send out a
>> proper patch, also upstream as QEMU's kvm suffers from the same issue.
>>
>> Jan
> 
> Have you tested this removing the other parts of it too?

I did now, and the effect is as suspected - no effect.

> I'd say there are unnecessary, and might well be harming other loads too.
> 
> There were once a time in which we executed kvm from inside qemu's cpu_exec
> loop. Back then, it made some sense to mess with qemu flags. Right now,
> I don't see any reason for us to ever touch it.

Well, upstream kvm also runs from cpu_exec but is fine with
corresponding surgery, too. I guess the point is that register read-back
now only takes place outside that context, and there we have the normal
representation.

But could someone explain /me why migration and suspend/resume didn't
crash regularly due to this bug?

However, let's start with getting rid of it here:

------>

It seems that the conversion of the kernel-delivered eflags state into
qemu's internal splitted representation was once needed in an older kvm
design (register read-back may have taken place from inside cpu_exec).
Today it is plain wrong and causes incorrect cpu state reporting (gdb,
monitor) and should also corrupt its saving (savevm, migration). Drop
the corresponding lines.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 qemu/qemu-kvm-x86.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
index 01748ed..645dc23 100644
--- a/qemu/qemu-kvm-x86.c
+++ b/qemu/qemu-kvm-x86.c
@@ -426,10 +426,6 @@ void kvm_arch_save_regs(CPUState *env)
             }
     }
     env->hflags = (env->hflags & HFLAG_COPY_MASK) | hflags;
-    env->cc_src = env->eflags & (CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
-    env->df = 1 - (2 * ((env->eflags >> 10) & 1));
-    env->cc_op = CC_OP_EFLAGS;
-    env->eflags &= ~(DF_MASK | CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
 
     /* msrs */
     n = 0;

Re: [PATCH] x86-userspace: Remove eflags conversion into emulator format

[Marcelo Tosatti]

On Mon, Jan 26, 2009 at 06:52:44PM +0100, Jan Kiszka wrote:
> Glauber Costa wrote:
> > On Mon, Jan 26, 2009 at 1:49 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >> Hi,
> >>
> >> this line almost ruined my afternoon:
> > There's a lesson for us to learn here: Always hack at night.
> 
> If the night wasn't that short...
> 
> > 
> >> diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
> >> index 01748ed..4ad386b 100644
> >> --- a/qemu/qemu-kvm-x86.c
> >> +++ b/qemu/qemu-kvm-x86.c
> >> @@ -429,7 +429,6 @@ void kvm_arch_save_regs(CPUState *env)
> >>     env->cc_src = env->eflags & (CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
> >>     env->df = 1 - (2 * ((env->eflags >> 10) & 1));
> >>     env->cc_op = CC_OP_EFLAGS;
> >> -    env->eflags &= ~(DF_MASK | CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
> >>
> >>     /* msrs */
> >>     n = 0;
> >>
> >> The guest flags reported via gdb or monitor were garbage and I first
> >> didn't realized this...
> >>
> >> git logs revealed that commit 6eecdc3eea74ead3c11b8b43d825d2cabe7a2456
> >> once introduced it mid of 2006, but maybe under different boundary
> >> conditions. At least today it appears to be plain wrong, eflags must
> >> always contain to full state, cc_src, df & cc_op are just supplementary
> >> states. Please correct me if I'm wrong, otherwise I will send out a
> >> proper patch, also upstream as QEMU's kvm suffers from the same issue.
> >>
> >> Jan
> > 
> > Have you tested this removing the other parts of it too?
> 
> I did now, and the effect is as suspected - no effect.
> 
> > I'd say there are unnecessary, and might well be harming other loads too.
> > 
> > There were once a time in which we executed kvm from inside qemu's cpu_exec
> > loop. Back then, it made some sense to mess with qemu flags. Right now,
> > I don't see any reason for us to ever touch it.
> 
> Well, upstream kvm also runs from cpu_exec but is fine with
> corresponding surgery, too. I guess the point is that register read-back
> now only takes place outside that context, and there we have the normal
> representation.
> 
> But could someone explain /me why migration and suspend/resume didn't
> crash regularly due to this bug?

Just luck probably, in HLT most of the time. 

> However, let's start with getting rid of it here:

Applied.

> ------>
> 
> It seems that the conversion of the kernel-delivered eflags state into
> qemu's internal splitted representation was once needed in an older kvm
> design (register read-back may have taken place from inside cpu_exec).
> Today it is plain wrong and causes incorrect cpu state reporting (gdb,
> monitor) and should also corrupt its saving (savevm, migration). Drop
> the corresponding lines.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
>  qemu/qemu-kvm-x86.c |    4 ----
>  1 files changed, 0 insertions(+), 4 deletions(-)
> 
> diff --git a/qemu/qemu-kvm-x86.c b/qemu/qemu-kvm-x86.c
> index 01748ed..645dc23 100644
> --- a/qemu/qemu-kvm-x86.c
> +++ b/qemu/qemu-kvm-x86.c
> @@ -426,10 +426,6 @@ void kvm_arch_save_regs(CPUState *env)
>              }
>      }
>      env->hflags = (env->hflags & HFLAG_COPY_MASK) | hflags;
> -    env->cc_src = env->eflags & (CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
> -    env->df = 1 - (2 * ((env->eflags >> 10) & 1));
> -    env->cc_op = CC_OP_EFLAGS;
> -    env->eflags &= ~(DF_MASK | CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C);
>  
>      /* msrs */
>      n = 0;