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

[Jan Kiszka <jan.kiszka@siemens.com>]

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;