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

[Alexander Graf]

On 21.12.2012, at 14:56, Jason J. Herne wrote:

> From: "Jason J. Herne" <address@hidden>
> 
> Modify syncing algorithm in do_kvm_cpu_synchronize_state to avoid
> overwriting previously synced register data by calling
> do_kvm_cpu_synchronize_state twice.
> 
> The problem occurs if the following sequence of events occurs:
> 1. kvm_arch_get_registers(env, KVM_REGSYNC_RUNTIME_STATE)
> 2. Use the runtime state
> 3. kvm_arch_get_registers(env, KVM_REGSYNC_FULL_STATE) (ignored)
> 4. Use the full state.
> 
> In step 4 the call to kvm_arch_get_registers() does nothing (to avoid 
> squashing
> local changes to the runtime registers), but the caller assumes the full
> register state is now available.
> 
> This is fixed by encoding which registers are synced in env->kvm_vcpu_dirty 
> and
> calling kvm_arch_put_registers() to sync local changes back to KVM before
> calling kvm_arch_get_registers() if we are expanding the set of synced
> registers.
> 
> Signed-off-by: Jason J. Herne <address@hidden>
> Reviewed-by: Christian Borntraeger <address@hidden>
> ---
> include/exec/cpu-defs.h |    6 ++++++
> kvm-all.c               |   14 ++++++++++----
> 2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
> index aea0ece..af3b6aa 100644
> --- a/include/exec/cpu-defs.h
> +++ b/include/exec/cpu-defs.h
> @@ -208,6 +208,12 @@ typedef struct CPUWatchpoint {
>     struct KVMState *kvm_state;                                         \
>     struct kvm_run *kvm_run;                                            \
>     int kvm_fd;                                                         \
> +                                                                        \
> +    /* Register level indicating which vcpu registers have been synced  \
> +       from KVM, are potentially dirty due to local modifications, and  \
> +       will need to be written back to KVM.  Valid values are 0, which  \
> +       indicates no registers are dirty, or any of the KVM_REGSYNC_*    \
> +       constants defined in kvm.h */                                    \
>     int kvm_vcpu_dirty;
> 
> #endif
> diff --git a/kvm-all.c b/kvm-all.c
> index aee5bdd..858a636 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -230,7 +230,7 @@ int kvm_init_vcpu(CPUArchState *env)
> 
>     env->kvm_fd = ret;
>     env->kvm_state = s;
> -    env->kvm_vcpu_dirty = 1;
> +    env->kvm_vcpu_dirty = KVM_REGSYNC_FULL_STATE;
> 
>     mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
>     if (mmap_size < 0) {
> @@ -1489,10 +1489,16 @@ void kvm_flush_coalesced_mmio_buffer(void)
> 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) {
    ...
}


Alex

>     if (!args->env->kvm_vcpu_dirty) {
> -        kvm_arch_get_registers(args->env, args->register_level);
> -        args->env->kvm_vcpu_dirty = 1;
> +        kvm_arch_get_registers(env, register_level);
> +        env->kvm_vcpu_dirty = register_level;
> +    } else if (register_level > env->kvm_vcpu_dirty) {
> +        kvm_arch_put_registers(env, env->kvm_vcpu_dirty);
> +        kvm_arch_get_registers(env, register_level);
> +        env->kvm_vcpu_dirty = register_level;
>     }
> }
> 
> @@ -1535,7 +1541,7 @@ int kvm_cpu_exec(CPUArchState *env)
> 
>     do {
>         if (env->kvm_vcpu_dirty) {
> -            kvm_arch_put_registers(env, KVM_REGSYNC_RUNTIME_STATE);
> +            kvm_arch_put_registers(env, env->kvm_vcpu_dirty);
>             env->kvm_vcpu_dirty = 0;
>         }
> 
> -- 
> 1.7.9.5
>