[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] target-i386: assert that KVM_GET/SET_MSRS can s
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH] target-i386: assert that KVM_GET/SET_MSRS can set all requested MSRs |
Date: |
Thu, 31 Mar 2016 15:03:57 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 |
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 <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;
>>
>