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

[Alexander Graf]

On 03.01.2013, at 19:48, Jason J. Herne wrote:

> 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.

As before, I'd prefer to make this explicit:

> 
> 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;

if (register_level > env->kvm_vcpu_dirty) {
    /* We are more dirty than we need to - all is well */
    return;
}

> 
>    /* 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. :)

Please also check out the discussions I've had with Bharat about his watchdog 
patches. There we need a mechanism to synchronize registers only when we 
actually need to, in order to avoid potential race conditions with a kernel 
timer.

That particular case doesn't work well with levels. We can have multiple 
different potential race producers in the kernel that we need to avoid 
individually, so we can't always synchronize all of them when only one of them 
needs to be synchronized.

The big question is what we should be doing about this. We basically have 3 
options:

  * implement levels, treat racy registers as "manually synchronized", as 
Bharat's latest patch set does
  * implement levels, add a bitmap for additional special synchronization bits
  * replace levels by bitmap

I'm quite frankly not sure which one of the 3 would be the best way forward.


Alex