x86: Corrupted eflags in qemu's CPU state
Hi,
this line almost ruined my afternoon:
qemu/qemu-kvm-x86.c +0 -1
@@ -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
Signature · 2 lines
Siemens AG, Corporate Technology, CT SE 2 Corporate Competence Center Embedded Linux
Quoted 3 lines · Jan Kiszka
Hi,
this line almost ruined my afternoon:
There's a lesson for us to learn here: Always hack at night.
Quoted 17 lines
qemu/qemu-kvm-x86.c +0
-1@@ -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...
Quoted 8 lines
git logs revealed that commit
6eecdc3eea74ead3c11b8b43d825d2cabe7a2456once 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_opare 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.
Signature · 5 lines
Glauber Costa. "Free as in Freedom" http://glommer.net "The less confident you are, the more serious you have to act."
PATCH 1/1 x86-userspace: Remove eflags conversion into emulator format
Quoted 5 lines · Glauber Costa
Quoted 3 lines · Jan Kiszka
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...
Quoted 24 lines
Quoted 24 lines
qemu/qemu-kvm-x86.c +0
-1@@ -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
6eecdc3eea74ead3c11b8b43d825d2cabe7a2456once 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_opare 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.
Quoted 3 lines
Quoted 1 line
Jan
Have you tested this removing the other parts of it too?
I did now, and the effect is as suspected - no effect.
Quoted 5 lines
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_execloop. 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.
---
qemu/qemu-kvm-x86.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-)
qemu/qemu-kvm-x86.c +0 -4
@@ -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 ;
Marcelo Tosatti
AppliedRe: [PATCH] x86-userspace: Remove eflags conversion into emulator format
Quoted 45 lines · Jan Kiszka
Quoted 5 lines · Glauber Costa
Quoted 3 lines · Jan Kiszka
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...
Quoted 24 lines
Quoted 24 lines
qemu/qemu-kvm-x86.c +0
-1@@ -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
6eecdc3eea74ead3c11b8b43d825d2cabe7a2456once 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_opare 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.Quoted 3 lines
Quoted 1 line
Jan
Have you tested this removing the other parts of it too?
I did now, and the effect is as suspected - no effect.
Quoted 5 lines
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_execloop. 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.
Quoted 7 lines
Well, upstream kvm also runs from
cpu_execbut 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.
Quoted 1 line
However, let's start with getting rid of it here:
Applied.
Quoted 30 lines
------>
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 <[email protected]>---
qemu/qemu-kvm-x86.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-)qemu/qemu-kvm-x86.c +0
-4@@ -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 ;