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

[Marcelo Tosatti <mtosatti@redhat.com>]

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;