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

[Marcelo Tosatti]

On Thu, Jan 03, 2013 at 08:09:22PM +0100, Alexander Graf wrote:
> 
> 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

Implement read/write accessors for individual registers, similarly to
how its done in the kernel. See kvm_cache_regs.h.

This also avoids the programmer from mapping "register X,Y,Z" to "level M"
manually (which is quite bug prone).

Can use KVM_GET/SET_ONEREG, then.