From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [PATCH v4 3/8] KVM Guest: Add VCPU running/pre-empted state for
 guest
Date: Fri, 24 Aug 2012 12:02:27 -0300
Message-ID: <20120824150227.GE13206@amt.cnet>
References: <20120821112346.3512.99814.stgit@abhimanyu.in.ibm.com>
 <20120821112630.3512.5109.stgit@abhimanyu>
 <20120823093643.GC32710@amt.cnet>
 <87vcg8q25g.fsf@linux.vnet.ibm.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: avi@redhat.com, raghukt@linux.vnet.ibm.com, alex.shi@intel.com,
	kvm@vger.kernel.org, stefano.stabellini@eu.citrix.com,
	peterz@infradead.org, hpa@zytor.com, vsrivatsa@gmail.com,
	mingo@elte.hu
To: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:10516 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1759330Ab2HXPNS (ORCPT <rfc822;kvm@vger.kernel.org>);
	Fri, 24 Aug 2012 11:13:18 -0400
Content-Disposition: inline
In-Reply-To: <87vcg8q25g.fsf@linux.vnet.ibm.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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.