From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:41044) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TqprY-0006ar-BW for qemu-devel@nongnu.org; Thu, 03 Jan 2013 13:49:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TqprX-0005z1-5x for qemu-devel@nongnu.org; Thu, 03 Jan 2013 13:49:56 -0500 Received: from e9.ny.us.ibm.com ([32.97.182.139]:41191) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TqprX-0005nC-29 for qemu-devel@nongnu.org; Thu, 03 Jan 2013 13:49:55 -0500 Received: from /spool/local by e9.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 3 Jan 2013 13:49:31 -0500 Received: from d01relay02.pok.ibm.com (d01relay02.pok.ibm.com [9.56.227.234]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id 334F138C8056 for ; Thu, 3 Jan 2013 13:49:23 -0500 (EST) Received: from d03av04.boulder.ibm.com (d03av04.boulder.ibm.com [9.17.195.170]) by d01relay02.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r03InMdH275722 for ; Thu, 3 Jan 2013 13:49:22 -0500 Received: from d03av04.boulder.ibm.com (loopback [127.0.0.1]) by d03av04.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r03In4Rw020644 for ; Thu, 3 Jan 2013 11:49:09 -0700 Message-ID: <50E5D29B.6060804@linux.vnet.ibm.com> Date: Thu, 03 Jan 2013 13:48:59 -0500 From: "Jason J. Herne" MIME-Version: 1.0 References: <1356098191-4998-1-git-send-email-jjherne@us.ibm.com> <133FEF92-3C4F-48C8-BF67-E50066EEEF45@suse.de> In-Reply-To: <133FEF92-3C4F-48C8-BF67-E50066EEEF45@suse.de> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 7/7] KVM regsync: Fix do_kvm_cpu_synchronize_state data integrity issue Reply-To: jjherne@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org, Alexander Graf , Christian Borntraeger On 01/03/2013 08:56 AM, Alexander Graf wrote: >> static void do_kvm_cpu_synchronize_state(void *_args) >> >{ >> > struct kvm_cpu_syncstate_args *args = _args; >> >+ CPUArchState *env = args->env; >> >+ int register_level = args->register_level; >> > > This probably becomes more readable if we explicitly revert back to unsynced state first: > > /* Write back local modifications at our current level */ > if (register_level > env->kvm_vcpu_dirty) { > kvm_arch_put_registers(...); > env->kvm_vcpu_dirty = 0; > } > > and then do the sync we are requested to do: > > if (!env->kvm_vcpu_dirty) { > ... > } I agree, but only if we add a second conditional to the if 1st statement as such: if (args->env->kvm_vcpu_dirty && register_level > env->kvm_vcpu_dirty) This is to cover the case where the caller is asking for register level "1" and we're already dirty at level "2". In this case, nothing should happen and we'll need the "args->env->kvm_vcpu_dirty" to ensure that is the case. static void do_kvm_cpu_synchronize_state(void *_args) { struct kvm_cpu_syncstate_args *args = _args; CPUArchState *env = args->env; int register_level = args->register_level; /* Write back local modifications at our current level */ if (args->env->kvm_vcpu_dirty && register_level > env->kvm_vcpu_dirty) { kvm_arch_put_registers(env, env->kvm_vcpu_dirty); env->kvm_vcpu_dirty = 0; } if (!args->env->kvm_vcpu_dirty) { kvm_arch_get_registers(env, register_level); env->kvm_vcpu_dirty = register_level; } } Do you agree? Thanks for your time. :) -- -- Jason J. Herne (jjherne@linux.vnet.ibm.com)