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

[Paolo Bonzini <pbonzini@redhat.com>]

On 31/03/2016 15:01, 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>?

They need not, but indeed the commit message has to be adjusted (unless
I send both of them in the same pull request, and then they effectively
become 1/2 and 2/2).

Paolo

> 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 <pbonzini@redhat.com>
>> ---
>>  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.
> 
> 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 <lersek@redhat.com>
> 
> 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;
>> +    }
>>  
>>      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;
>>
>