Re: [Qemu-devel] [PATCH 7/7] KVM regsync: Fix do_kvm_cpu_synchronize_state data integrity issue

["Jason J. Herne" <jjherne@linux.vnet.ibm.com>]

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)