From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56492) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alcGd-00009k-Mr for qemu-devel@nongnu.org; Thu, 31 Mar 2016 09:04:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1alcGX-0006xU-GM for qemu-devel@nongnu.org; Thu, 31 Mar 2016 09:04:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56703) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alcGX-0006xG-8q for qemu-devel@nongnu.org; Thu, 31 Mar 2016 09:04:01 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id E0C4065437 for ; Thu, 31 Mar 2016 13:04:00 +0000 (UTC) References: <1459371583-4824-2-git-send-email-pbonzini@redhat.com> <56FD1FA9.7070200@redhat.com> From: Paolo Bonzini Message-ID: <56FD203D.20806@redhat.com> Date: Thu, 31 Mar 2016 15:03:57 +0200 MIME-Version: 1.0 In-Reply-To: <56FD1FA9.7070200@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: Laszlo Ersek , qemu-devel@nongnu.org Cc: ehabkost@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 > ? 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 >> --- >> 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; >> >