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

Marcelo Tosatti
2012-08-24T15:13:18Z

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.