Re: [PATCH v4 3/8] KVM Guest: Add VCPU running/pre-empted state for guest

[Marcelo Tosatti <mtosatti@redhat.com>]

On Fri, Aug 24, 2012 at 11:09:39AM +0530, Nikunj A Dadhania wrote:
> On Thu, 23 Aug 2012 06:36:43 -0300, Marcelo Tosatti <mtosatti@redhat.com> wrote:
> > On Tue, Aug 21, 2012 at 04:56:35PM +0530, Nikunj A. Dadhania wrote:
> > >  
> > > +void kvm_disable_vcpu_state(void)
> > > +{
> > > +	if (!has_vcpu_state)
> > > +		return;
> > > +
> > > +	wrmsr(MSR_KVM_VCPU_STATE, 0, 0);
> > 
> > wrmsrl (to be consistent).
> >
> Sure, will change
>  
> > > +}
> > > +
> > >  #ifdef CONFIG_SMP
> > >  static void __init kvm_smp_prepare_boot_cpu(void)
> > >  {
> > > @@ -410,6 +440,7 @@ static void __cpuinit kvm_guest_cpu_online(void *dummy)
> > >  
> > >  static void kvm_guest_cpu_offline(void *dummy)
> > >  {
> > > +	kvm_disable_vcpu_state();
> > 
> > Should disable MSR at kvm_pv_guest_cpu_reboot.
> > 
> Sure, can you explain the difference for my understanding?

These are different callbacks. One is used for CPU offline, the
other during reboot.

> > >  	kvm_disable_steal_time();
> > >  	if (kvm_para_has_feature(KVM_FEATURE_PV_EOI))
> > >  		wrmsrl(MSR_KVM_PV_EOI_EN, 0);
> > > @@ -469,6 +500,11 @@ void __init kvm_guest_init(void)
> > >  	if (kvm_para_has_feature(KVM_FEATURE_PV_EOI))
> > >  		apic_set_eoi_write(kvm_guest_apic_eoi_write);
> > >  
> > > +#ifdef CONFIG_PARAVIRT_TLB_FLUSH
> > > +	if (kvm_para_has_feature(KVM_FEATURE_VCPU_STATE))
> > > +		has_vcpu_state = 1;
> > > +#endif
> > 
> > Why only this hunk guarded by CONFIG_PARAVIRT_TLB_FLUSH and not
> > the rest of the code?
> > 
> The guest should have been compiled with CONFIG_PARAVIRT_TLB_FLUSH, as
> the config also brings in HAVE_RCU_TABLE_FREE code into picture. We
> should not enable this code without HAVE_RCU_TABLE_FREE.
> 
> Did not want to spray this across all the code, as the compiler will
> take care of throwing out the kvm_tlb_flush_others
> 
> > Is there a switch to enable/disable this feature on the kernel
> > command line? 
> >
> No, havent added it. 
> 
> > Grep for early_param in kvm.c.
> > 
> Let me know if that is required.

Yes, please add it. Its useful.