From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sheng Yang Subject: Re: [PATCH 2/4] KVM: Extended shared_msr_global to per CPU Date: Thu, 17 Dec 2009 23:01:32 +0800 Message-ID: <200912172301.33033.sheng@linux.intel.com> References: <4B28A6A2.3090802@redhat.com> <1261042355-32381-1-git-send-email-sheng@linux.intel.com> <4B2A08A8.7080603@redhat.com> Mime-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , kvm@vger.kernel.org To: Avi Kivity Return-path: Received: from mga06.intel.com ([134.134.136.21]:51152 "EHLO orsmga101.jf.intel.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1762273AbZLQPBf (ORCPT ); Thu, 17 Dec 2009 10:01:35 -0500 In-Reply-To: <4B2A08A8.7080603@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Thursday 17 December 2009 18:32:08 Avi Kivity wrote: > On 12/17/2009 11:32 AM, Sheng Yang wrote: > > shared_msr_global saved host value of relevant MSRs, but it have an > > assumption that all MSRs it tracked shared the value across the different > > CPUs. It's not true with some MSRs, e.g. MSR_TSC_AUX. > > > > Extend it to per CPU to provide the support of MSR_TSC_AUX, and more > > alike MSRs. > > > > Notice now the shared_msr_global still have one assumption: it can only > > deal with the MSRs that won't change in host after KVM module loaded. > > > > Signed-off-by: Sheng Yang > > --- > > > > How about this? > > > > Move the all initialization to hardware_enable(). And only initialized > > once for each cpu. > > > > > > -void kvm_define_shared_msr(unsigned slot, u32 msr) > > +static void shared_msr_update(unsigned slot, u32 msr) > > { > > - int cpu; > > + struct kvm_shared_msrs *smsr; > > u64 value; > > > > + smsr =&__get_cpu_var(shared_msrs); > > + /* only read, and nobody should modify it at this time, > > + * so don't need lock */ > > + if (slot>= shared_msrs_global.nr) { > > + printk(KERN_ERR "kvm: invalid MSR slot!"); > > + return; > > + } > > + if (smsr->values[slot].initialized) > > + return; > > I don't think .initialized is worthwhile. shared_msr_update is run very > rarely. The reason is, after cpu hotplug, the MSR_TSC_AUX would be rewritten by vsyscall_init again. But in the hotplug notifier chain, KVM has higher priority(20 vs 0 for vsyscall_init), so maybe the rdmsr() here would get a bogus value... Then I think prevent it from initializing again should be safer. But I just think of another issue: if we hot plug in a cpu(without hot plug off), it would have a bogus value as well in the same path? Sound troublesome... > > > + smsr->values[slot].initialized = true; > > + put_cpu_var(shared_msrs); > > If you use __get_cpu_var(), you need to remove put_cpu_var(). > Sure... -- regards Yang, Sheng