From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: BUG with Win7 and user-return-notifier Date: Thu, 29 Oct 2009 18:05:24 +0200 Message-ID: <4AE9BD44.4080708@redhat.com> References: <4AE6ED18.9040901@siemens.com> <4AE6F17C.1070403@redhat.com> <4AE6F1EE.5090207@siemens.com> <4AE6F4A3.3050903@redhat.com> <4AE6F4C4.3000802@redhat.com> <4AE7FE3B.2070802@redhat.com> <4AE84EB4.1010603@siemens.com> <4AE86AA0.1060802@redhat.com> <4AE8AC20.50506@web.de> <4AE9462E.5050409@redhat.com> <4AE94C63.2070300@web.de> <4AE94D29.8030600@redhat.com> <4AE9530C.6080701@web.de> <4AE9B8AE.1000008@siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm-devel To: Jan Kiszka Return-path: Received: from mx1.redhat.com ([209.132.183.28]:9936 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753220AbZJ2QFX (ORCPT ); Thu, 29 Oct 2009 12:05:23 -0400 In-Reply-To: <4AE9B8AE.1000008@siemens.com> Sender: kvm-owner@vger.kernel.org List-ID: On 10/29/2009 05:45 PM, Jan Kiszka wrote: > >> static int vmx_vcpu_setup(struct vcpu_vmx *vmx) >> { >> ... >> for (i = 0; i< NR_VMX_MSR; ++i) { >> u32 index = vmx_msr_index[i]; >> u32 data_low, data_high; >> u64 data; >> int j = vmx->nmsrs; >> >> if (rdmsr_safe(index,&data_low,&data_high)< 0) >> continue; >> if (wrmsr_safe(index, data_low, data_high)< 0) >> continue; >> data = data_low | ((u64)data_high<< 32); >> vmx->guest_msrs[j].index = i; >> vmx->guest_msrs[j].data = 0; >> > ^^^^^ > Local 'data' drops on the floor. Is that correct (then it deserves a > cleanup)? Previous version did a "guest = host". > Arguably, it's more correct than what we used to have. This code dates back to the day when kvm started in 64-bit mode and so we copied important MSRs from the host. Shouldn't matter for our case. >> static void vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer) >> { >> struct vcpu_vmx *vmx = to_vmx(vcpu); >> struct shared_msr_entry *msr = find_msr_entry(vmx, MSR_EFER); >> >> if (!msr) >> return; >> vcpu->arch.shadow_efer = efer; >> if (!msr) >> return; >> > One "if (!msr)" too much - really the second one? > Yes. (Either really - if there is no host EFER, the only legal value for efer is 0, so whether we update it or not doesn't matter). Likely introduced by a bad merge. How code rots. -- error compiling committee.c: too many arguments to function