On 21.12.2012, at 14:56, Jason J. Herne wrote:
> From: "Jason J. Herne" <jjhe...@us.ibm.com>
>
> 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 <jjhe...@us.ibm.com>
> Reviewed-by: Christian Borntraeger <borntrae...@de.ibm.com>
> ---
> 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
>
