Re: [Qemu-devel] [PATCH] target-i386: assert that KVM_GET/SET_MSRS can set all requested MSRs

[Eduardo Habkost]

On Thu, Mar 31, 2016 at 03:01:29PM +0200, Laszlo Ersek wrote:
> On 03/30/16 22:59, Paolo Bonzini wrote:
> > This would have caught the bug in the previous patch.
> 
> Should this patch share a series with
> <http://thread.gmane.org/gmane.comp.emulators.qemu/404245>? Otherwise
> they could be separated by other patches in the commit history, and then
> "previous patch" would be misleading.
> 
> (Alternatively, the reference to "previous patch" could be made by subject.)
> 
> > Signed-off-by: Paolo Bonzini <address@hidden>
> > ---
> >  target-i386/kvm.c | 34 ++++++++++++++++++++++++++++++----
> >  1 file changed, 30 insertions(+), 4 deletions(-)
> > 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 19e2d94..799fdfa 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -141,6 +141,7 @@ static int kvm_get_tsc(CPUState *cs)
> >          return ret;
> >      }
> >  
> > +    assert(ret == 1);
> >      env->tsc = msr_data.entries[0].data;
> >      return 0;
> >  }
> > @@ -1446,6 +1447,7 @@ static int kvm_put_tscdeadline_msr(X86CPU *cpu)
> >          struct kvm_msr_entry entries[1];
> >      } msr_data;
> >      struct kvm_msr_entry *msrs = msr_data.entries;
> > +    int ret;
> >  
> >      if (!has_msr_tsc_deadline) {
> >          return 0;
> > @@ -1457,7 +1459,13 @@ static int kvm_put_tscdeadline_msr(X86CPU *cpu)
> >          .nmsrs = 1,
> >      };
> >  
> > -    return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
> > +    ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> > +
> > +    assert(ret == 1);
> > +    return 0;
> >  }
> 
> This changes the return value of kvm_put_tscdeadline_msr() -- and
> friends below -- for successful invocations. I guess that's fine, but a
> note about it in the commit message would be nice.

All these functions have only one caller each, that only checks
if ret < 0.

(As they are all static functions with a single caller in
target-i386/kvm.c, I don't mind if this is not mentioned in the
commit message.)

> 
> Anyway, I'm not an "expert" in this area, so the best I can offer for
> this two-part (almost-) series, with the commit message nits fixed, is
> 
> Acked-by: Laszlo Ersek <address@hidden>
> 
> Thanks
> Laszlo
> 
> >  
> >  /*
> > @@ -1472,6 +1480,11 @@ static int kvm_put_msr_feature_control(X86CPU *cpu)
> >          struct kvm_msrs info;
> >          struct kvm_msr_entry entry;
> >      } msr_data;
> > +    int ret;
> > +
> > +    if (!has_msr_feature_control) {
> > +        return 0;
> > +    }

This is not strictly needed to implement what's described in the
commit message, but it makes kvm_put_msr_feature_control() safer
and harder to break.

Reviewed-by: Eduardo Habkost <address@hidden>

> >  
> >      kvm_msr_entry_set(&msr_data.entry, MSR_IA32_FEATURE_CONTROL,
> >                        cpu->env.msr_ia32_feature_control);
> > @@ -1480,7 +1493,13 @@ static int kvm_put_msr_feature_control(X86CPU *cpu)
> >          .nmsrs = 1,
> >      };
> >  
> > -    return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
> > +    ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> > +
> > +    assert(ret == 1);
> > +    return 0;
> >  }
> >  
> >  static int kvm_put_msrs(X86CPU *cpu, int level)
> > @@ -1492,6 +1511,7 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> >      } msr_data;
> >      struct kvm_msr_entry *msrs = msr_data.entries;
> >      int n = 0, i;
> > +    int ret;
> >  
> >      kvm_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_CS, env->sysenter_cs);
> >      kvm_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_ESP, 
> > env->sysenter_esp);
> > @@ -1685,8 +1705,13 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> >          .nmsrs = n,
> >      };
> >  
> > -    return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
> > +    ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, &msr_data);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> >  
> > +    assert(ret == n);
> > +    return 0;
> >  }
> >  
> >  
> > @@ -2055,6 +2080,7 @@ static int kvm_get_msrs(X86CPU *cpu)
> >          return ret;
> >      }
> >  
> > +    assert(ret == n);
> >      for (i = 0; i < ret; i++) {
> >          uint32_t index = msrs[i].index;
> >          switch (index) {
> > @@ -2511,7 +2537,7 @@ int kvm_arch_put_registers(CPUState *cpu, int level)
> >  
> >      assert(cpu_is_stopped(cpu) || qemu_cpu_is_self(cpu));
> >  
> > -    if (level >= KVM_PUT_RESET_STATE && has_msr_feature_control) {
> > +    if (level >= KVM_PUT_RESET_STATE) {
> >          ret = kvm_put_msr_feature_control(x86_cpu);
> >          if (ret < 0) {
> >              return ret;
> > 
> 

-- 
Eduardo