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@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? 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@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
