From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59582) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WlEXQ-0007pz-CW for qemu-devel@nongnu.org; Fri, 16 May 2014 05:34:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WlEXH-0000Dn-Ay for qemu-devel@nongnu.org; Fri, 16 May 2014 05:34:48 -0400 Received: from mail-ee0-x22c.google.com ([2a00:1450:4013:c00::22c]:35886) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WlEXH-0000Di-4D for qemu-devel@nongnu.org; Fri, 16 May 2014 05:34:39 -0400 Received: by mail-ee0-f44.google.com with SMTP id c41so1343310eek.31 for ; Fri, 16 May 2014 02:34:38 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <5375DBA8.9050701@redhat.com> Date: Fri, 16 May 2014 11:34:32 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1400173453-27705-1-git-send-email-pbonzini@redhat.com> <5374F998.3020203@twiddle.net> In-Reply-To: <5374F998.3020203@twiddle.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] target-i386: set CC_OP to CC_OP_EFLAGS in cpu_load_eflags List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson , qemu-devel@nongnu.org Il 15/05/2014 19:30, Richard Henderson ha scritto: > On 05/15/2014 10:04 AM, Paolo Bonzini wrote: >> There is no reason to keep that out of the function. The comment refers >> to the disassembler's cc_op state rather than the CPUState field. > > Yes, but... > >> -/* NOTE: CC_OP must be modified manually to CC_OP_EFLAGS */ >> +/* NOTE: set CC_OP to CC_OP_EFLAGS after calling a helper that uses this! */ >> static inline void cpu_load_eflags(CPUX86State *env, int eflags, >> int update_mask) >> { >> CC_SRC = eflags & (CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C); >> + CC_OP = CC_OP_EFLAGS; >> env->df = 1 - (2 * ((eflags >> 10) & 1)); > > ... we'd do well to reflect that in the comment here, rather than > in the commit message. Because by itself it looks like the comment > is out of date. That's what I tried to do, but my wording was awful. > /* NOTE: the translator must set DisasContext.cc_op to CC_OP_EFLAGS > after generating a call to a helper that uses this. */ Much better. >> --- a/target-i386/svm_helper.c >> +++ b/target-i386/svm_helper.c >> @@ -260,7 +260,6 @@ void helper_vmrun(CPUX86State *env, int aflag, int next_eip_addend) >> env->vm_vmcb + offsetof(struct vmcb, >> save.rflags)), >> ~(CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C | DF_MASK)); >> - CC_OP = CC_OP_EFLAGS; >> >> svm_load_seg_cache(env, env->vm_vmcb + offsetof(struct vmcb, save.es), >> R_ES); >> -- > > I see two sets of CC_OP in svm_helper.c. Missed one? Yes. Paolo