From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55929) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alcEE-0006M5-0p for qemu-devel@nongnu.org; Thu, 31 Mar 2016 09:01:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1alcE8-0006dM-Vh for qemu-devel@nongnu.org; Thu, 31 Mar 2016 09:01:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57749) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alcE8-0006dH-PM for qemu-devel@nongnu.org; Thu, 31 Mar 2016 09:01:32 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 5EDDAC05005E for ; Thu, 31 Mar 2016 13:01:32 +0000 (UTC) References: <1459371583-4824-2-git-send-email-pbonzini@redhat.com> From: Laszlo Ersek Message-ID: <56FD1FA9.7070200@redhat.com> Date: Thu, 31 Mar 2016 15:01:29 +0200 MIME-Version: 1.0 In-Reply-To: <1459371583-4824-2-git-send-email-pbonzini@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] target-i386: assert that KVM_GET/SET_MSRS can set all requested MSRs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , qemu-devel@nongnu.org Cc: ehabkost@redhat.com 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 ? 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 > --- > 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 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; >