Glauber Costa wrote:
> On Mon, Jan 26, 2009 at 1:49 PM, Jan Kiszka <jan.kiszka@xxxxxxxxxxx> 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@xxxxxxxxxxx>
---
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;
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
