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

[Laszlo Ersek]

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.

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