Re: [PATCH 2/4] KVM: Extended shared_msr_global to per CPU

[Sheng Yang <sheng@linux.intel.com>]

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<sheng@linux.intel.com>
> > ---
> >
> > 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