[PATCH 0/8] use jump labels to streamline common APIC configuration

[PATCH 0/8] use jump labels to streamline common APIC configuration

[Gleb Natapov]

APIC code has a lot of checks for apic presence and apic HW/SW enable
state.  Most common configuration is when each vcpu has in kernel apic
and it is fully enabled. This path series uses jump labels to turn checks
to nops in the common case. 

Gleb Natapov (8):
  KVM: clean up kvm_(set|get)_apic_base
  KVM: use kvm_lapic_set_base() to change apic_base
  KVM: mark apic enabled on start up.
  Export jump_label_rate_limit()
  KVM: use jump label to optimize checking for HW enabled APIC in
    APIC_BASE MSR.
  KVM: use jump label to optimize checking for SW enabled apic in
    spurious interrupt register
  KVM: use jump label to optimize checking for in kernel local apic
    presence.
  KVM: inline kvm_apic_present() and kvm_lapic_enabled()

 arch/x86/kvm/lapic.c |  211 +++++++++++++++++++++++++++-----------------------
 arch/x86/kvm/lapic.h |   46 ++++++++++-
 arch/x86/kvm/x86.c   |   18 ++---
 arch/x86/kvm/x86.h   |    1 +
 kernel/jump_label.c  |    1 +
 5 files changed, 170 insertions(+), 107 deletions(-)

-- 
1.7.10

[PATCH 1/8] KVM: clean up kvm_(set|get)_apic_base

[Gleb Natapov]

kvm_get_apic_base() needlessly checks irqchip_in_kernel although it does
the same no matter what result of the check is. kvm_set_apic_base() also
checks for irqchip_in_kernel, but kvm_lapic_set_base() can handle this
case.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/x86.c |   10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 530d800..f339b2c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -246,20 +246,14 @@ static void drop_user_return_notifiers(void *ignore)
 
 u64 kvm_get_apic_base(struct kvm_vcpu *vcpu)
 {
-	if (irqchip_in_kernel(vcpu->kvm))
-		return vcpu->arch.apic_base;
-	else
-		return vcpu->arch.apic_base;
+	return vcpu->arch.apic_base;
 }
 EXPORT_SYMBOL_GPL(kvm_get_apic_base);
 
 void kvm_set_apic_base(struct kvm_vcpu *vcpu, u64 data)
 {
 	/* TODO: reserve bits check */
-	if (irqchip_in_kernel(vcpu->kvm))
-		kvm_lapic_set_base(vcpu, data);
-	else
-		vcpu->arch.apic_base = data;
+	kvm_lapic_set_base(vcpu, data);
 }
 EXPORT_SYMBOL_GPL(kvm_set_apic_base);
 
-- 
1.7.10

[PATCH 2/8] KVM: use kvm_lapic_set_base() to change apic_base

[Gleb Natapov]

Do not change apic_base directly. Use kvm_lapic_set_base() instead.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/lapic.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 0cd431c..49f4ac0 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1185,7 +1185,8 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu)
 	update_divide_count(apic);
 	atomic_set(&apic->lapic_timer.pending, 0);
 	if (kvm_vcpu_is_bsp(vcpu))
-		vcpu->arch.apic_base |= MSR_IA32_APICBASE_BSP;
+		kvm_lapic_set_base(vcpu,
+				vcpu->arch.apic_base | MSR_IA32_APICBASE_BSP);
 	vcpu->arch.pv_eoi.msr_val = 0;
 	apic_update_ppr(apic);
 
@@ -1310,8 +1311,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
 		     HRTIMER_MODE_ABS);
 	apic->lapic_timer.timer.function = apic_timer_fn;
 
-	apic->base_address = APIC_DEFAULT_PHYS_BASE;
-	vcpu->arch.apic_base = APIC_DEFAULT_PHYS_BASE;
+	kvm_lapic_set_base(vcpu, APIC_DEFAULT_PHYS_BASE);
 
 	kvm_lapic_reset(vcpu);
 	kvm_iodevice_init(&apic->dev, &apic_mmio_ops);
@@ -1380,8 +1380,7 @@ void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
-	apic->base_address = vcpu->arch.apic_base &
-			     MSR_IA32_APICBASE_BASE;
+	kvm_lapic_set_base(vcpu, vcpu->arch.apic_base);
 	kvm_apic_set_version(vcpu);
 
 	apic_update_ppr(apic);
-- 
1.7.10

[PATCH 3/8] KVM: mark apic enabled on start up.

[Gleb Natapov]

According to SDM apic is enabled on start up.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/lapic.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 49f4ac0..c3f14fe 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1311,7 +1311,8 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
 		     HRTIMER_MODE_ABS);
 	apic->lapic_timer.timer.function = apic_timer_fn;
 
-	kvm_lapic_set_base(vcpu, APIC_DEFAULT_PHYS_BASE);
+	kvm_lapic_set_base(vcpu,
+			APIC_DEFAULT_PHYS_BASE | MSR_IA32_APICBASE_ENABLE);
 
 	kvm_lapic_reset(vcpu);
 	kvm_iodevice_init(&apic->dev, &apic_mmio_ops);
-- 
1.7.10

Re: [PATCH 3/8] KVM: mark apic enabled on start up.

[Avi Kivity]

On 08/05/2012 03:58 PM, Gleb Natapov wrote:
> According to SDM apic is enabled on start up.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>  arch/x86/kvm/lapic.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 49f4ac0..c3f14fe 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1311,7 +1311,8 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
>  		     HRTIMER_MODE_ABS);
>  	apic->lapic_timer.timer.function = apic_timer_fn;
>  
> -	kvm_lapic_set_base(vcpu, APIC_DEFAULT_PHYS_BASE);
> +	kvm_lapic_set_base(vcpu,
> +			APIC_DEFAULT_PHYS_BASE | MSR_IA32_APICBASE_ENABLE);
>  

I'm guessing that in practice this is a no-op since qemu reset involves
programming this MSR.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 3/8] KVM: mark apic enabled on start up.

[Gleb Natapov]

On Sun, Aug 05, 2012 at 05:14:59PM +0300, Avi Kivity wrote:
> On 08/05/2012 03:58 PM, Gleb Natapov wrote:
> > According to SDM apic is enabled on start up.
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > ---
> >  arch/x86/kvm/lapic.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 49f4ac0..c3f14fe 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -1311,7 +1311,8 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
> >  		     HRTIMER_MODE_ABS);
> >  	apic->lapic_timer.timer.function = apic_timer_fn;
> >  
> > -	kvm_lapic_set_base(vcpu, APIC_DEFAULT_PHYS_BASE);
> > +	kvm_lapic_set_base(vcpu,
> > +			APIC_DEFAULT_PHYS_BASE | MSR_IA32_APICBASE_ENABLE);
> >  
> 
> I'm guessing that in practice this is a no-op since qemu reset involves
> programming this MSR.
> 
Qemu is not the only userspace :) But this is here to avoid unneeded
jump label patching truth to be told.

--
			Gleb.

Re: [PATCH 3/8] KVM: mark apic enabled on start up.

[Avi Kivity]

On 08/05/2012 05:17 PM, Gleb Natapov wrote:
> On Sun, Aug 05, 2012 at 05:14:59PM +0300, Avi Kivity wrote:
>> On 08/05/2012 03:58 PM, Gleb Natapov wrote:
>> > According to SDM apic is enabled on start up.
>> > 
>> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
>> > ---
>> >  arch/x86/kvm/lapic.c |    3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> > index 49f4ac0..c3f14fe 100644
>> > --- a/arch/x86/kvm/lapic.c
>> > +++ b/arch/x86/kvm/lapic.c
>> > @@ -1311,7 +1311,8 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
>> >  		     HRTIMER_MODE_ABS);
>> >  	apic->lapic_timer.timer.function = apic_timer_fn;
>> >  
>> > -	kvm_lapic_set_base(vcpu, APIC_DEFAULT_PHYS_BASE);
>> > +	kvm_lapic_set_base(vcpu,
>> > +			APIC_DEFAULT_PHYS_BASE | MSR_IA32_APICBASE_ENABLE);
>> >  
>> 
>> I'm guessing that in practice this is a no-op since qemu reset involves
>> programming this MSR.
>> 
> Qemu is not the only userspace :) But this is here to avoid unneeded
> jump label patching truth to be told.

It's also correct, otherwise it wouldn't be here.

But won't the patching happen anyway? since it starts software disabled?


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 3/8] KVM: mark apic enabled on start up.

[Gleb Natapov]

On Sun, Aug 05, 2012 at 05:39:33PM +0300, Avi Kivity wrote:
> On 08/05/2012 05:17 PM, Gleb Natapov wrote:
> > On Sun, Aug 05, 2012 at 05:14:59PM +0300, Avi Kivity wrote:
> >> On 08/05/2012 03:58 PM, Gleb Natapov wrote:
> >> > According to SDM apic is enabled on start up.
> >> > 
> >> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> >> > ---
> >> >  arch/x86/kvm/lapic.c |    3 ++-
> >> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >> > 
> >> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> >> > index 49f4ac0..c3f14fe 100644
> >> > --- a/arch/x86/kvm/lapic.c
> >> > +++ b/arch/x86/kvm/lapic.c
> >> > @@ -1311,7 +1311,8 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
> >> >  		     HRTIMER_MODE_ABS);
> >> >  	apic->lapic_timer.timer.function = apic_timer_fn;
> >> >  
> >> > -	kvm_lapic_set_base(vcpu, APIC_DEFAULT_PHYS_BASE);
> >> > +	kvm_lapic_set_base(vcpu,
> >> > +			APIC_DEFAULT_PHYS_BASE | MSR_IA32_APICBASE_ENABLE);
> >> >  
> >> 
> >> I'm guessing that in practice this is a no-op since qemu reset involves
> >> programming this MSR.
> >> 
> > Qemu is not the only userspace :) But this is here to avoid unneeded
> > jump label patching truth to be told.
> 
> It's also correct, otherwise it wouldn't be here.
> 
Of course.

> But won't the patching happen anyway? since it starts software disabled?
> 
Yes, but different label, so less patching.

--
			Gleb.

[PATCH 4/8] Export jump_label_rate_limit()

[Gleb Natapov]

CC: Jason Baron <jbaron@redhat.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 kernel/jump_label.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 4304919..60f48fa 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -118,6 +118,7 @@ void jump_label_rate_limit(struct static_key_deferred *key,
 	key->timeout = rl;
 	INIT_DELAYED_WORK(&key->work, jump_label_update_timeout);
 }
+EXPORT_SYMBOL_GPL(jump_label_rate_limit);
 
 static int addr_conflict(struct jump_entry *entry, void *start, void *end)
 {
-- 
1.7.10

Re: [PATCH 4/8] Export jump_label_rate_limit()

[Avi Kivity]

On 08/05/2012 03:58 PM, Gleb Natapov wrote:
> CC: Jason Baron <jbaron@redhat.com>
> CC: Ingo Molnar <mingo@elte.hu>
> CC: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>  kernel/jump_label.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> index 4304919..60f48fa 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -118,6 +118,7 @@ void jump_label_rate_limit(struct static_key_deferred *key,
>  	key->timeout = rl;
>  	INIT_DELAYED_WORK(&key->work, jump_label_update_timeout);
>  }
> +EXPORT_SYMBOL_GPL(jump_label_rate_limit);
>  

Jason, please ack this if it is acceptable.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 4/8] Export jump_label_rate_limit()

[Jason Baron]

On Sun, Aug 05, 2012 at 05:16:24PM +0300, Avi Kivity wrote:
> On 08/05/2012 03:58 PM, Gleb Natapov wrote:
> > CC: Jason Baron <jbaron@redhat.com>
> > CC: Ingo Molnar <mingo@elte.hu>
> > CC: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > ---
> >  kernel/jump_label.c |    1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> > index 4304919..60f48fa 100644
> > --- a/kernel/jump_label.c
> > +++ b/kernel/jump_label.c
> > @@ -118,6 +118,7 @@ void jump_label_rate_limit(struct static_key_deferred *key,
> >  	key->timeout = rl;
> >  	INIT_DELAYED_WORK(&key->work, jump_label_update_timeout);
> >  }
> > +EXPORT_SYMBOL_GPL(jump_label_rate_limit);
> >  
> 
> Jason, please ack this if it is acceptable.
> 

Acked-by: Jason Baron <jbaron@redhat.com>

[PATCH 5/8] KVM: use jump label to optimize checking for HW enabled APIC in APIC_BASE MSR.

[Gleb Natapov]

Usually all APICs are HW enabled so the check can be optimized out.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/lapic.c |   29 ++++++++++++++++++++++++++++-
 arch/x86/kvm/lapic.h |    1 +
 arch/x86/kvm/x86.c   |    1 +
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index c3f14fe..1aa5528 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -34,6 +34,7 @@
 #include <asm/current.h>
 #include <asm/apicdef.h>
 #include <linux/atomic.h>
+#include <linux/jump_label.h>
 #include "kvm_cache_regs.h"
 #include "irq.h"
 #include "trace.h"
@@ -117,9 +118,13 @@ static inline int __apic_test_and_clear_vector(int vec, void *bitmap)
 	return __test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
 }
 
+struct static_key_deferred apic_hw_disabled __read_mostly;
+
 static inline int apic_hw_enabled(struct kvm_lapic *apic)
 {
-	return (apic)->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE;
+	if (static_key_false(&apic_hw_disabled.key))
+		return apic->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE;
+	return MSR_IA32_APICBASE_ENABLE;
 }
 
 static inline int  apic_sw_enabled(struct kvm_lapic *apic)
@@ -1055,6 +1060,9 @@ void kvm_free_lapic(struct kvm_vcpu *vcpu)
 
 	hrtimer_cancel(&vcpu->arch.apic->lapic_timer.timer);
 
+	if (!(vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE))
+		static_key_slow_dec_deferred(&apic_hw_disabled);
+
 	if (vcpu->arch.apic->regs)
 		free_page((unsigned long)vcpu->arch.apic->regs);
 
@@ -1125,6 +1133,14 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
 		return;
 	}
 
+	/* update jump label if enable bit changes */
+	if ((vcpu->arch.apic_base ^ value) & MSR_IA32_APICBASE_ENABLE) {
+		if (value & MSR_IA32_APICBASE_ENABLE)
+			static_key_slow_dec_deferred(&apic_hw_disabled);
+		else
+			static_key_slow_inc(&apic_hw_disabled.key);
+	}
+
 	if (!kvm_vcpu_is_bsp(apic->vcpu))
 		value &= ~MSR_IA32_APICBASE_BSP;
 
@@ -1311,6 +1327,11 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
 		     HRTIMER_MODE_ABS);
 	apic->lapic_timer.timer.function = apic_timer_fn;
 
+	/*
+	 * APIC is created enabled. This will prevent kvm_lapic_set_base from
+	 * thinking that APIC satet has changed.
+	 */ 
+	vcpu->arch.apic_base = MSR_IA32_APICBASE_ENABLE;
 	kvm_lapic_set_base(vcpu,
 			APIC_DEFAULT_PHYS_BASE | MSR_IA32_APICBASE_ENABLE);
 
@@ -1598,3 +1619,9 @@ int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data)
 	return kvm_gfn_to_hva_cache_init(vcpu->kvm, &vcpu->arch.pv_eoi.data,
 					 addr);
 }
+
+void kvm_lapic_init(void)
+{
+	/* do not patch jump label more than once per second */
+	jump_label_rate_limit(&apic_hw_disabled, HZ);
+}
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 166766f..73fa299 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -78,4 +78,5 @@ static inline bool kvm_hv_vapic_assist_page_enabled(struct kvm_vcpu *vcpu)
 }
 
 int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data);
+void kvm_lapic_init(void);
 #endif
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f339b2c..12812db 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4908,6 +4908,7 @@ int kvm_arch_init(void *opaque)
 	if (cpu_has_xsave)
 		host_xcr0 = xgetbv(XCR_XFEATURE_ENABLED_MASK);
 
+	kvm_lapic_init();
 	return 0;
 
 out:
-- 
1.7.10

Re: [PATCH 5/8] KVM: use jump label to optimize checking for HW enabled APIC in APIC_BASE MSR.

[Avi Kivity]

On 08/05/2012 03:58 PM, Gleb Natapov wrote:
> Usually all APICs are HW enabled so the check can be optimized out.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
>  arch/x86/kvm/lapic.c |   29 ++++++++++++++++++++++++++++-
>  arch/x86/kvm/lapic.h |    1 +
>  arch/x86/kvm/x86.c   |    1 +
>  3 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index c3f14fe..1aa5528 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -34,6 +34,7 @@
>  #include <asm/current.h>
>  #include <asm/apicdef.h>
>  #include <linux/atomic.h>
> +#include <linux/jump_label.h>
>  #include "kvm_cache_regs.h"
>  #include "irq.h"
>  #include "trace.h"
> @@ -117,9 +118,13 @@ static inline int __apic_test_and_clear_vector(int vec, void *bitmap)
>  	return __test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
>  }
>  
> +struct static_key_deferred apic_hw_disabled __read_mostly;

On top of file please.  Add all_ to the name to make it clear we're
talking about all apics.

> +
>  static inline int apic_hw_enabled(struct kvm_lapic *apic)
>  {
> -	return (apic)->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE;
> +	if (static_key_false(&apic_hw_disabled.key))
> +		return apic->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE;

Hm, for the test to be readable, it needs to be

   if (static_key_false(&all_apics_hw_enabled))




-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 5/8] KVM: use jump label to optimize checking for HW enabled APIC in APIC_BASE MSR.

[Gleb Natapov]

On Sun, Aug 05, 2012 at 05:35:21PM +0300, Avi Kivity wrote:
> On 08/05/2012 03:58 PM, Gleb Natapov wrote:
> > Usually all APICs are HW enabled so the check can be optimized out.
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > ---
> >  arch/x86/kvm/lapic.c |   29 ++++++++++++++++++++++++++++-
> >  arch/x86/kvm/lapic.h |    1 +
> >  arch/x86/kvm/x86.c   |    1 +
> >  3 files changed, 30 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index c3f14fe..1aa5528 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -34,6 +34,7 @@
> >  #include <asm/current.h>
> >  #include <asm/apicdef.h>
> >  #include <linux/atomic.h>
> > +#include <linux/jump_label.h>
> >  #include "kvm_cache_regs.h"
> >  #include "irq.h"
> >  #include "trace.h"
> > @@ -117,9 +118,13 @@ static inline int __apic_test_and_clear_vector(int vec, void *bitmap)
> >  	return __test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
> >  }
> >  
> > +struct static_key_deferred apic_hw_disabled __read_mostly;
> 
> On top of file please.  Add all_ to the name to make it clear we're
> talking about all apics.
> 
This is count of disabled apics really. So I think all_apic_hw_disabled
is misleading.

> > +
> >  static inline int apic_hw_enabled(struct kvm_lapic *apic)
> >  {
> > -	return (apic)->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE;
> > +	if (static_key_false(&apic_hw_disabled.key))
> > +		return apic->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE;
> 
> Hm, for the test to be readable, it needs to be
> 
>    if (static_key_false(&all_apics_hw_enabled))
> 
Exactly. all_ makes it so because apic_hw_disabled is a counter that
counts disabled apics. So may be call it global_hw_disabled_apic_counter?

--
			Gleb.

Re: [PATCH 5/8] KVM: use jump label to optimize checking for HW enabled APIC in APIC_BASE MSR.

[Avi Kivity]

On 08/05/2012 05:42 PM, Gleb Natapov wrote:
> On Sun, Aug 05, 2012 at 05:35:21PM +0300, Avi Kivity wrote:
>> On 08/05/2012 03:58 PM, Gleb Natapov wrote:
>> > Usually all APICs are HW enabled so the check can be optimized out.
>> > 
>> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
>> > ---
>> >  arch/x86/kvm/lapic.c |   29 ++++++++++++++++++++++++++++-
>> >  arch/x86/kvm/lapic.h |    1 +
>> >  arch/x86/kvm/x86.c   |    1 +
>> >  3 files changed, 30 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> > index c3f14fe..1aa5528 100644
>> > --- a/arch/x86/kvm/lapic.c
>> > +++ b/arch/x86/kvm/lapic.c
>> > @@ -34,6 +34,7 @@
>> >  #include <asm/current.h>
>> >  #include <asm/apicdef.h>
>> >  #include <linux/atomic.h>
>> > +#include <linux/jump_label.h>
>> >  #include "kvm_cache_regs.h"
>> >  #include "irq.h"
>> >  #include "trace.h"
>> > @@ -117,9 +118,13 @@ static inline int __apic_test_and_clear_vector(int vec, void *bitmap)
>> >  	return __test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
>> >  }
>> >  
>> > +struct static_key_deferred apic_hw_disabled __read_mostly;
>> 
>> On top of file please.  Add all_ to the name to make it clear we're
>> talking about all apics.
>> 
> This is count of disabled apics really. So I think all_apic_hw_disabled
> is misleading.
> 
>> > +
>> >  static inline int apic_hw_enabled(struct kvm_lapic *apic)
>> >  {
>> > -	return (apic)->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE;
>> > +	if (static_key_false(&apic_hw_disabled.key))
>> > +		return apic->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE;
>> 
>> Hm, for the test to be readable, it needs to be
>> 
>>    if (static_key_false(&all_apics_hw_enabled))
>> 
> Exactly. all_ makes it so because apic_hw_disabled is a counter that
> counts disabled apics. So may be call it global_hw_disabled_apic_counter?
> 

The problem is how static_key_false() is defined.  It returns true if
the count > 0, opposite from what I'd expect.  So anything with counter
semantics will be confusing.  I guess we need to pick a neutral name
(apic_disabled_key or apic_disabled_slowpath or such) to force the
reader to look at the definitions.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 5/8] KVM: use jump label to optimize checking for HW enabled APIC in APIC_BASE MSR.

[Gleb Natapov]

On Sun, Aug 05, 2012 at 05:48:42PM +0300, Avi Kivity wrote:
> On 08/05/2012 05:42 PM, Gleb Natapov wrote:
> > On Sun, Aug 05, 2012 at 05:35:21PM +0300, Avi Kivity wrote:
> >> On 08/05/2012 03:58 PM, Gleb Natapov wrote:
> >> > Usually all APICs are HW enabled so the check can be optimized out.
> >> > 
> >> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> >> > ---
> >> >  arch/x86/kvm/lapic.c |   29 ++++++++++++++++++++++++++++-
> >> >  arch/x86/kvm/lapic.h |    1 +
> >> >  arch/x86/kvm/x86.c   |    1 +
> >> >  3 files changed, 30 insertions(+), 1 deletion(-)
> >> > 
> >> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> >> > index c3f14fe..1aa5528 100644
> >> > --- a/arch/x86/kvm/lapic.c
> >> > +++ b/arch/x86/kvm/lapic.c
> >> > @@ -34,6 +34,7 @@
> >> >  #include <asm/current.h>
> >> >  #include <asm/apicdef.h>
> >> >  #include <linux/atomic.h>
> >> > +#include <linux/jump_label.h>
> >> >  #include "kvm_cache_regs.h"
> >> >  #include "irq.h"
> >> >  #include "trace.h"
> >> > @@ -117,9 +118,13 @@ static inline int __apic_test_and_clear_vector(int vec, void *bitmap)
> >> >  	return __test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
> >> >  }
> >> >  
> >> > +struct static_key_deferred apic_hw_disabled __read_mostly;
> >> 
> >> On top of file please.  Add all_ to the name to make it clear we're
> >> talking about all apics.
> >> 
> > This is count of disabled apics really. So I think all_apic_hw_disabled
> > is misleading.
> > 
> >> > +
> >> >  static inline int apic_hw_enabled(struct kvm_lapic *apic)
> >> >  {
> >> > -	return (apic)->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE;
> >> > +	if (static_key_false(&apic_hw_disabled.key))
> >> > +		return apic->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE;
> >> 
> >> Hm, for the test to be readable, it needs to be
> >> 
> >>    if (static_key_false(&all_apics_hw_enabled))
> >> 
> > Exactly. all_ makes it so because apic_hw_disabled is a counter that
> > counts disabled apics. So may be call it global_hw_disabled_apic_counter?
> > 
> 
> The problem is how static_key_false() is defined.  It returns true if
> the count > 0, opposite from what I'd expect.  So anything with counter
> semantics will be confusing.  I guess we need to pick a neutral name
> (apic_disabled_key or apic_disabled_slowpath or such) to force the
> reader to look at the definitions.
> 
There was a long thread about readability of static_key_true/false and
this is the result of it :( I read it this way: drop static_key_(true|false)
and read the if() to understand its meaning. Now look at static_key_(true|false)
to see what fast path will be in asm code.

--
			Gleb.

[PATCH 6/8] KVM: use jump label to optimize checking for SW enabled apic in spurious interrupt register

[Gleb Natapov]

Usually all APICs are SW enabled so the check can be optimized out.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/lapic.c |   39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 1aa5528..952774c 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -127,9 +127,24 @@ static inline int apic_hw_enabled(struct kvm_lapic *apic)
 	return MSR_IA32_APICBASE_ENABLE;
 }
 
-static inline int  apic_sw_enabled(struct kvm_lapic *apic)
+struct static_key_deferred apic_sw_disabled __read_mostly;
+
+static inline void apic_set_spiv(struct kvm_lapic *apic, u32 val)
+{
+	if ((apic_get_reg(apic, APIC_SPIV) ^ val) & APIC_SPIV_APIC_ENABLED) {
+		if (val & APIC_SPIV_APIC_ENABLED)
+			static_key_slow_dec_deferred(&apic_sw_disabled);
+		else
+			static_key_slow_inc(&apic_sw_disabled.key);
+	}
+	apic_set_reg(apic, APIC_SPIV, val);
+}
+
+static inline int apic_sw_enabled(struct kvm_lapic *apic)
 {
-	return apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_APIC_ENABLED;
+	if (static_key_false(&apic_sw_disabled.key))
+		return apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_APIC_ENABLED;
+	return APIC_SPIV_APIC_ENABLED;
 }
 
 static inline int apic_enabled(struct kvm_lapic *apic)
@@ -918,7 +933,7 @@ static int apic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 		u32 mask = 0x3ff;
 		if (apic_get_reg(apic, APIC_LVR) & APIC_LVR_DIRECTED_EOI)
 			mask |= APIC_SPIV_DIRECTED_EOI;
-		apic_set_reg(apic, APIC_SPIV, val & mask);
+		apic_set_spiv(apic, val & mask);
 		if (!(val & APIC_SPIV_APIC_ENABLED)) {
 			int i;
 			u32 lvt_val;
@@ -1055,18 +1070,23 @@ EXPORT_SYMBOL_GPL(kvm_lapic_set_eoi);
 
 void kvm_free_lapic(struct kvm_vcpu *vcpu)
 {
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
 	if (!vcpu->arch.apic)
 		return;
 
-	hrtimer_cancel(&vcpu->arch.apic->lapic_timer.timer);
+	hrtimer_cancel(&apic->lapic_timer.timer);
 
 	if (!(vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE))
 		static_key_slow_dec_deferred(&apic_hw_disabled);
 
-	if (vcpu->arch.apic->regs)
-		free_page((unsigned long)vcpu->arch.apic->regs);
+	if (!(apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_APIC_ENABLED))
+		static_key_slow_dec_deferred(&apic_sw_disabled);
 
-	kfree(vcpu->arch.apic);
+	if (apic->regs)
+		free_page((unsigned long)apic->regs);
+
+	kfree(apic);
 }
 
 /*
@@ -1182,7 +1202,7 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu)
 		     SET_APIC_DELIVERY_MODE(0, APIC_MODE_EXTINT));
 
 	apic_set_reg(apic, APIC_DFR, 0xffffffffU);
-	apic_set_reg(apic, APIC_SPIV, 0xff);
+	apic_set_spiv(apic, 0xff);
 	apic_set_reg(apic, APIC_TASKPRI, 0);
 	apic_set_reg(apic, APIC_LDR, 0);
 	apic_set_reg(apic, APIC_ESR, 0);
@@ -1335,6 +1355,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
 	kvm_lapic_set_base(vcpu,
 			APIC_DEFAULT_PHYS_BASE | MSR_IA32_APICBASE_ENABLE);
 
+	static_key_slow_inc(&apic_sw_disabled.key); /* sw disabled at reset */
 	kvm_lapic_reset(vcpu);
 	kvm_iodevice_init(&apic->dev, &apic_mmio_ops);
 
@@ -1404,6 +1425,7 @@ void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu)
 
 	kvm_lapic_set_base(vcpu, vcpu->arch.apic_base);
 	kvm_apic_set_version(vcpu);
+	apic_set_spiv(apic, apic_get_reg(apic, APIC_SPIV));
 
 	apic_update_ppr(apic);
 	hrtimer_cancel(&apic->lapic_timer.timer);
@@ -1624,4 +1646,5 @@ void kvm_lapic_init(void)
 {
 	/* do not patch jump label more than once per second */
 	jump_label_rate_limit(&apic_hw_disabled, HZ);
+	jump_label_rate_limit(&apic_sw_disabled, HZ);
 }
-- 
1.7.10

[PATCH 7/8] KVM: use jump label to optimize checking for in kernel local apic presence.

[Gleb Natapov]

Usually all vcpus have local apic pointer initialized, so the check may
be completely skipped.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/lapic.c |   62 +++++++++++++++++++++++++++-----------------------
 arch/x86/kvm/x86.c   |    7 +++++-
 arch/x86/kvm/x86.h   |    1 +
 3 files changed, 41 insertions(+), 29 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 952774c..c97284b 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -152,6 +152,13 @@ static inline int apic_enabled(struct kvm_lapic *apic)
 	return apic_sw_enabled(apic) &&	apic_hw_enabled(apic);
 }
 
+static inline bool vcpu_has_lapic(struct kvm_vcpu *vcpu)
+{
+	if (static_key_false(&kvm_no_apic_vcpu))
+		return vcpu->arch.apic;
+	return true;
+}
+
 #define LVT_MASK	\
 	(APIC_LVT_MASKED | APIC_SEND_PENDING | APIC_VECTOR_MASK)
 
@@ -204,7 +211,7 @@ void kvm_apic_set_version(struct kvm_vcpu *vcpu)
 	struct kvm_cpuid_entry2 *feat;
 	u32 v = APIC_VERSION;
 
-	if (!irqchip_in_kernel(vcpu->kvm))
+	if (!vcpu_has_lapic(vcpu))
 		return;
 
 	feat = kvm_find_cpuid_entry(apic->vcpu, 0x1, 0);
@@ -305,7 +312,6 @@ static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
 
 int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
 {
-	struct kvm_lapic *apic = vcpu->arch.apic;
 	int highest_irr;
 
 	/* This may race with setting of irr in __apic_accept_irq() and
@@ -313,9 +319,9 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
 	 * will cause vmexit immediately and the value will be recalculated
 	 * on the next vmentry.
 	 */
-	if (!apic)
+	if (!vcpu_has_lapic(vcpu))
 		return 0;
-	highest_irr = apic_find_highest_irr(apic);
+	highest_irr = apic_find_highest_irr(vcpu->arch.apic);
 
 	return highest_irr;
 }
@@ -1061,9 +1067,7 @@ static int apic_mmio_write(struct kvm_io_device *this,
 
 void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu)
 {
-	struct kvm_lapic *apic = vcpu->arch.apic;
-
-	if (apic)
+	if (vcpu_has_lapic(vcpu))
 		apic_reg_write(vcpu->arch.apic, APIC_EOI, 0);
 }
 EXPORT_SYMBOL_GPL(kvm_lapic_set_eoi);
@@ -1098,10 +1102,9 @@ void kvm_free_lapic(struct kvm_vcpu *vcpu)
 u64 kvm_get_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
-	if (!apic)
-		return 0;
 
-	if (apic_lvtt_oneshot(apic) || apic_lvtt_period(apic))
+	if (!vcpu_has_lapic(vcpu) || apic_lvtt_oneshot(apic) ||
+			apic_lvtt_period(apic))
 		return 0;
 
 	return apic->lapic_timer.tscdeadline;
@@ -1110,10 +1113,9 @@ u64 kvm_get_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu)
 void kvm_set_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu, u64 data)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
-	if (!apic)
-		return;
 
-	if (apic_lvtt_oneshot(apic) || apic_lvtt_period(apic))
+	if (!vcpu_has_lapic(vcpu) || apic_lvtt_oneshot(apic) ||
+			apic_lvtt_period(apic))
 		return;
 
 	hrtimer_cancel(&apic->lapic_timer.timer);
@@ -1125,20 +1127,21 @@ void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
-	if (!apic)
+	if (!vcpu_has_lapic(vcpu))
 		return;
+
 	apic_set_tpr(apic, ((cr8 & 0x0f) << 4)
 		     | (apic_get_reg(apic, APIC_TASKPRI) & 4));
 }
 
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu)
 {
-	struct kvm_lapic *apic = vcpu->arch.apic;
 	u64 tpr;
 
-	if (!apic)
+	if (!vcpu_has_lapic(vcpu))
 		return 0;
-	tpr = (u64) apic_get_reg(apic, APIC_TASKPRI);
+
+	tpr = (u64) apic_get_reg(vcpu->arch.apic, APIC_TASKPRI);
 
 	return (tpr & 0xf0) >> 4;
 }
@@ -1237,7 +1240,7 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu)
 
 bool kvm_apic_present(struct kvm_vcpu *vcpu)
 {
-	return vcpu->arch.apic && apic_hw_enabled(vcpu->arch.apic);
+	return vcpu_has_lapic(vcpu) && apic_hw_enabled(vcpu->arch.apic);
 }
 
 int kvm_lapic_enabled(struct kvm_vcpu *vcpu)
@@ -1258,10 +1261,11 @@ static bool lapic_is_periodic(struct kvm_lapic *apic)
 
 int apic_has_pending_timer(struct kvm_vcpu *vcpu)
 {
-	struct kvm_lapic *lapic = vcpu->arch.apic;
+	struct kvm_lapic *apic = vcpu->arch.apic;
 
-	if (lapic && apic_enabled(lapic) && apic_lvt_enabled(lapic, APIC_LVTT))
-		return atomic_read(&lapic->lapic_timer.pending);
+	if (vcpu_has_lapic(vcpu) && apic_enabled(apic) &&
+			apic_lvt_enabled(apic, APIC_LVTT))
+		return atomic_read(&apic->lapic_timer.pending);
 
 	return 0;
 }
@@ -1371,7 +1375,7 @@ int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu)
 	struct kvm_lapic *apic = vcpu->arch.apic;
 	int highest_irr;
 
-	if (!apic || !apic_enabled(apic))
+	if (!vcpu_has_lapic(vcpu) || !apic_enabled(apic))
 		return -1;
 
 	apic_update_ppr(apic);
@@ -1399,7 +1403,10 @@ void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
-	if (apic && atomic_read(&apic->lapic_timer.pending) > 0) {
+	if (!vcpu_has_lapic(vcpu))
+		return;
+
+	if (atomic_read(&apic->lapic_timer.pending) > 0) {
 		if (kvm_apic_local_deliver(apic, APIC_LVTT))
 			atomic_dec(&apic->lapic_timer.pending);
 	}
@@ -1439,13 +1446,12 @@ void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu)
 
 void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
 {
-	struct kvm_lapic *apic = vcpu->arch.apic;
 	struct hrtimer *timer;
 
-	if (!apic)
+	if (!vcpu_has_lapic(vcpu))
 		return;
 
-	timer = &apic->lapic_timer.timer;
+	timer = &vcpu->arch.apic->lapic_timer.timer;
 	if (hrtimer_cancel(timer))
 		hrtimer_start_expires(timer, HRTIMER_MODE_ABS);
 }
@@ -1602,7 +1608,7 @@ int kvm_hv_vapic_msr_write(struct kvm_vcpu *vcpu, u32 reg, u64 data)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
-	if (!irqchip_in_kernel(vcpu->kvm))
+	if (!vcpu_has_lapic(vcpu))
 		return 1;
 
 	/* if this is ICR write vector before command */
@@ -1616,7 +1622,7 @@ int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 reg, u64 *data)
 	struct kvm_lapic *apic = vcpu->arch.apic;
 	u32 low, high = 0;
 
-	if (!irqchip_in_kernel(vcpu->kvm))
+	if (!vcpu_has_lapic(vcpu))
 		return 1;
 
 	if (apic_reg_read(apic, reg, 4, &low))
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 12812db..042754b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6169,6 +6169,8 @@ bool kvm_vcpu_compatible(struct kvm_vcpu *vcpu)
 	return irqchip_in_kernel(vcpu->kvm) == (vcpu->arch.apic != NULL);
 }
 
+struct static_key kvm_no_apic_vcpu __read_mostly;
+
 int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 {
 	struct page *page;
@@ -6201,7 +6203,8 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 		r = kvm_create_lapic(vcpu);
 		if (r < 0)
 			goto fail_mmu_destroy;
-	}
+	} else
+		static_key_slow_inc(&kvm_no_apic_vcpu);
 
 	vcpu->arch.mce_banks = kzalloc(KVM_MAX_MCE_BANKS * sizeof(u64) * 4,
 				       GFP_KERNEL);
@@ -6241,6 +6244,8 @@ void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu)
 	kvm_mmu_destroy(vcpu);
 	srcu_read_unlock(&vcpu->kvm->srcu, idx);
 	free_page((unsigned long)vcpu->arch.pio_data);
+	if (!irqchip_in_kernel(vcpu->kvm))
+		static_key_slow_dec(&kvm_no_apic_vcpu);
 }
 
 int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 3d1134d..2b5219c 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -124,4 +124,5 @@ int kvm_write_guest_virt_system(struct x86_emulate_ctxt *ctxt,
 
 extern u64 host_xcr0;
 
+extern struct static_key kvm_no_apic_vcpu;
 #endif
-- 
1.7.10

[PATCH 8/8] KVM: inline kvm_apic_present() and kvm_lapic_enabled()

[Gleb Natapov]

Those functions are used during interrupt injection. When inlined they
become nops on the fast path.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
---
 arch/x86/kvm/lapic.c |  143 +++++++++++++++++++-------------------------------
 arch/x86/kvm/lapic.h |   45 +++++++++++++++-
 2 files changed, 96 insertions(+), 92 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index c97284b..f019ca2 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -73,11 +73,6 @@
 static unsigned int min_timer_period_us = 500;
 module_param(min_timer_period_us, uint, S_IRUGO | S_IWUSR);
 
-static inline u32 apic_get_reg(struct kvm_lapic *apic, int reg_off)
-{
-	return *((u32 *) (apic->regs + reg_off));
-}
-
 static inline void apic_set_reg(struct kvm_lapic *apic, int reg_off, u32 val)
 {
 	*((u32 *) (apic->regs + reg_off)) = val;
@@ -119,19 +114,11 @@ static inline int __apic_test_and_clear_vector(int vec, void *bitmap)
 }
 
 struct static_key_deferred apic_hw_disabled __read_mostly;
-
-static inline int apic_hw_enabled(struct kvm_lapic *apic)
-{
-	if (static_key_false(&apic_hw_disabled.key))
-		return apic->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE;
-	return MSR_IA32_APICBASE_ENABLE;
-}
-
 struct static_key_deferred apic_sw_disabled __read_mostly;
 
 static inline void apic_set_spiv(struct kvm_lapic *apic, u32 val)
 {
-	if ((apic_get_reg(apic, APIC_SPIV) ^ val) & APIC_SPIV_APIC_ENABLED) {
+	if ((kvm_apic_get_reg(apic, APIC_SPIV) ^ val) & APIC_SPIV_APIC_ENABLED) {
 		if (val & APIC_SPIV_APIC_ENABLED)
 			static_key_slow_dec_deferred(&apic_sw_disabled);
 		else
@@ -140,23 +127,9 @@ static inline void apic_set_spiv(struct kvm_lapic *apic, u32 val)
 	apic_set_reg(apic, APIC_SPIV, val);
 }
 
-static inline int apic_sw_enabled(struct kvm_lapic *apic)
-{
-	if (static_key_false(&apic_sw_disabled.key))
-		return apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_APIC_ENABLED;
-	return APIC_SPIV_APIC_ENABLED;
-}
-
 static inline int apic_enabled(struct kvm_lapic *apic)
 {
-	return apic_sw_enabled(apic) &&	apic_hw_enabled(apic);
-}
-
-static inline bool vcpu_has_lapic(struct kvm_vcpu *vcpu)
-{
-	if (static_key_false(&kvm_no_apic_vcpu))
-		return vcpu->arch.apic;
-	return true;
+	return kvm_apic_sw_enabled(apic) &&	kvm_apic_hw_enabled(apic);
 }
 
 #define LVT_MASK	\
@@ -168,34 +141,34 @@ static inline bool vcpu_has_lapic(struct kvm_vcpu *vcpu)
 
 static inline int kvm_apic_id(struct kvm_lapic *apic)
 {
-	return (apic_get_reg(apic, APIC_ID) >> 24) & 0xff;
+	return (kvm_apic_get_reg(apic, APIC_ID) >> 24) & 0xff;
 }
 
 static inline int apic_lvt_enabled(struct kvm_lapic *apic, int lvt_type)
 {
-	return !(apic_get_reg(apic, lvt_type) & APIC_LVT_MASKED);
+	return !(kvm_apic_get_reg(apic, lvt_type) & APIC_LVT_MASKED);
 }
 
 static inline int apic_lvt_vector(struct kvm_lapic *apic, int lvt_type)
 {
-	return apic_get_reg(apic, lvt_type) & APIC_VECTOR_MASK;
+	return kvm_apic_get_reg(apic, lvt_type) & APIC_VECTOR_MASK;
 }
 
 static inline int apic_lvtt_oneshot(struct kvm_lapic *apic)
 {
-	return ((apic_get_reg(apic, APIC_LVTT) &
+	return ((kvm_apic_get_reg(apic, APIC_LVTT) &
 		apic->lapic_timer.timer_mode_mask) == APIC_LVT_TIMER_ONESHOT);
 }
 
 static inline int apic_lvtt_period(struct kvm_lapic *apic)
 {
-	return ((apic_get_reg(apic, APIC_LVTT) &
+	return ((kvm_apic_get_reg(apic, APIC_LVTT) &
 		apic->lapic_timer.timer_mode_mask) == APIC_LVT_TIMER_PERIODIC);
 }
 
 static inline int apic_lvtt_tscdeadline(struct kvm_lapic *apic)
 {
-	return ((apic_get_reg(apic, APIC_LVTT) &
+	return ((kvm_apic_get_reg(apic, APIC_LVTT) &
 		apic->lapic_timer.timer_mode_mask) ==
 			APIC_LVT_TIMER_TSCDEADLINE);
 }
@@ -211,7 +184,7 @@ void kvm_apic_set_version(struct kvm_vcpu *vcpu)
 	struct kvm_cpuid_entry2 *feat;
 	u32 v = APIC_VERSION;
 
-	if (!vcpu_has_lapic(vcpu))
+	if (!kvm_vcpu_has_lapic(vcpu))
 		return;
 
 	feat = kvm_find_cpuid_entry(apic->vcpu, 0x1, 0);
@@ -319,7 +292,7 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
 	 * will cause vmexit immediately and the value will be recalculated
 	 * on the next vmentry.
 	 */
-	if (!vcpu_has_lapic(vcpu))
+	if (!kvm_vcpu_has_lapic(vcpu))
 		return 0;
 	highest_irr = apic_find_highest_irr(vcpu->arch.apic);
 
@@ -404,8 +377,8 @@ static void apic_update_ppr(struct kvm_lapic *apic)
 	u32 tpr, isrv, ppr, old_ppr;
 	int isr;
 
-	old_ppr = apic_get_reg(apic, APIC_PROCPRI);
-	tpr = apic_get_reg(apic, APIC_TASKPRI);
+	old_ppr = kvm_apic_get_reg(apic, APIC_PROCPRI);
+	tpr = kvm_apic_get_reg(apic, APIC_TASKPRI);
 	isr = apic_find_highest_isr(apic);
 	isrv = (isr != -1) ? isr : 0;
 
@@ -441,13 +414,13 @@ int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda)
 	u32 logical_id;
 
 	if (apic_x2apic_mode(apic)) {
-		logical_id = apic_get_reg(apic, APIC_LDR);
+		logical_id = kvm_apic_get_reg(apic, APIC_LDR);
 		return logical_id & mda;
 	}
 
-	logical_id = GET_APIC_LOGICAL_ID(apic_get_reg(apic, APIC_LDR));
+	logical_id = GET_APIC_LOGICAL_ID(kvm_apic_get_reg(apic, APIC_LDR));
 
-	switch (apic_get_reg(apic, APIC_DFR)) {
+	switch (kvm_apic_get_reg(apic, APIC_DFR)) {
 	case APIC_DFR_FLAT:
 		if (logical_id & mda)
 			result = 1;
@@ -459,7 +432,7 @@ int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda)
 		break;
 	default:
 		apic_debug("Bad DFR vcpu %d: %08x\n",
-			   apic->vcpu->vcpu_id, apic_get_reg(apic, APIC_DFR));
+			   apic->vcpu->vcpu_id, kvm_apic_get_reg(apic, APIC_DFR));
 		break;
 	}
 
@@ -617,7 +590,7 @@ static int apic_set_eoi(struct kvm_lapic *apic)
 	apic_clear_isr(vector, apic);
 	apic_update_ppr(apic);
 
-	if (!(apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI) &&
+	if (!(kvm_apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI) &&
 	    kvm_ioapic_handles_vector(apic->vcpu->kvm, vector)) {
 		int trigger_mode;
 		if (apic_test_vector(vector, apic->regs + APIC_TMR))
@@ -632,8 +605,8 @@ static int apic_set_eoi(struct kvm_lapic *apic)
 
 static void apic_send_ipi(struct kvm_lapic *apic)
 {
-	u32 icr_low = apic_get_reg(apic, APIC_ICR);
-	u32 icr_high = apic_get_reg(apic, APIC_ICR2);
+	u32 icr_low = kvm_apic_get_reg(apic, APIC_ICR);
+	u32 icr_high = kvm_apic_get_reg(apic, APIC_ICR2);
 	struct kvm_lapic_irq irq;
 
 	irq.vector = icr_low & APIC_VECTOR_MASK;
@@ -668,7 +641,7 @@ static u32 apic_get_tmcct(struct kvm_lapic *apic)
 	ASSERT(apic != NULL);
 
 	/* if initial count is 0, current count should also be 0 */
-	if (apic_get_reg(apic, APIC_TMICT) == 0)
+	if (kvm_apic_get_reg(apic, APIC_TMICT) == 0)
 		return 0;
 
 	remaining = hrtimer_get_remaining(&apic->lapic_timer.timer);
@@ -724,13 +697,13 @@ static u32 __apic_read(struct kvm_lapic *apic, unsigned int offset)
 		break;
 	case APIC_PROCPRI:
 		apic_update_ppr(apic);
-		val = apic_get_reg(apic, offset);
+		val = kvm_apic_get_reg(apic, offset);
 		break;
 	case APIC_TASKPRI:
 		report_tpr_access(apic, false);
 		/* fall thru */
 	default:
-		val = apic_get_reg(apic, offset);
+		val = kvm_apic_get_reg(apic, offset);
 		break;
 	}
 
@@ -782,7 +755,7 @@ static int apic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
 
 static int apic_mmio_in_range(struct kvm_lapic *apic, gpa_t addr)
 {
-	return apic_hw_enabled(apic) &&
+	return kvm_apic_hw_enabled(apic) &&
 	    addr >= apic->base_address &&
 	    addr < apic->base_address + LAPIC_MMIO_LENGTH;
 }
@@ -805,7 +778,7 @@ static void update_divide_count(struct kvm_lapic *apic)
 {
 	u32 tmp1, tmp2, tdcr;
 
-	tdcr = apic_get_reg(apic, APIC_TDCR);
+	tdcr = kvm_apic_get_reg(apic, APIC_TDCR);
 	tmp1 = tdcr & 0xf;
 	tmp2 = ((tmp1 & 0x3) | ((tmp1 & 0x8) >> 1)) + 1;
 	apic->divide_count = 0x1 << (tmp2 & 0x7);
@@ -822,7 +795,7 @@ static void start_apic_timer(struct kvm_lapic *apic)
 	if (apic_lvtt_period(apic) || apic_lvtt_oneshot(apic)) {
 		/* lapic timer in oneshot or periodic mode */
 		now = apic->lapic_timer.timer.base->get_time();
-		apic->lapic_timer.period = (u64)apic_get_reg(apic, APIC_TMICT)
+		apic->lapic_timer.period = (u64)kvm_apic_get_reg(apic, APIC_TMICT)
 			    * APIC_BUS_CYCLE_NS * apic->divide_count;
 
 		if (!apic->lapic_timer.period)
@@ -854,7 +827,7 @@ static void start_apic_timer(struct kvm_lapic *apic)
 			   "timer initial count 0x%x, period %lldns, "
 			   "expire @ 0x%016" PRIx64 ".\n", __func__,
 			   APIC_BUS_CYCLE_NS, ktime_to_ns(now),
-			   apic_get_reg(apic, APIC_TMICT),
+			   kvm_apic_get_reg(apic, APIC_TMICT),
 			   apic->lapic_timer.period,
 			   ktime_to_ns(ktime_add_ns(now,
 					apic->lapic_timer.period)));
@@ -886,7 +859,7 @@ static void start_apic_timer(struct kvm_lapic *apic)
 
 static void apic_manage_nmi_watchdog(struct kvm_lapic *apic, u32 lvt0_val)
 {
-	int nmi_wd_enabled = apic_lvt_nmi_mode(apic_get_reg(apic, APIC_LVT0));
+	int nmi_wd_enabled = apic_lvt_nmi_mode(kvm_apic_get_reg(apic, APIC_LVT0));
 
 	if (apic_lvt_nmi_mode(lvt0_val)) {
 		if (!nmi_wd_enabled) {
@@ -937,7 +910,7 @@ static int apic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 
 	case APIC_SPIV: {
 		u32 mask = 0x3ff;
-		if (apic_get_reg(apic, APIC_LVR) & APIC_LVR_DIRECTED_EOI)
+		if (kvm_apic_get_reg(apic, APIC_LVR) & APIC_LVR_DIRECTED_EOI)
 			mask |= APIC_SPIV_DIRECTED_EOI;
 		apic_set_spiv(apic, val & mask);
 		if (!(val & APIC_SPIV_APIC_ENABLED)) {
@@ -945,7 +918,7 @@ static int apic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 			u32 lvt_val;
 
 			for (i = 0; i < APIC_LVT_NUM; i++) {
-				lvt_val = apic_get_reg(apic,
+				lvt_val = kvm_apic_get_reg(apic,
 						       APIC_LVTT + 0x10 * i);
 				apic_set_reg(apic, APIC_LVTT + 0x10 * i,
 					     lvt_val | APIC_LVT_MASKED);
@@ -974,7 +947,7 @@ static int apic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 	case APIC_LVT1:
 	case APIC_LVTERR:
 		/* TODO: Check vector */
-		if (!apic_sw_enabled(apic))
+		if (!kvm_apic_sw_enabled(apic))
 			val |= APIC_LVT_MASKED;
 
 		val &= apic_lvt_mask[(reg - APIC_LVTT) >> 4];
@@ -983,12 +956,12 @@ static int apic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 		break;
 
 	case APIC_LVTT:
-		if ((apic_get_reg(apic, APIC_LVTT) &
+		if ((kvm_apic_get_reg(apic, APIC_LVTT) &
 		    apic->lapic_timer.timer_mode_mask) !=
 		   (val & apic->lapic_timer.timer_mode_mask))
 			hrtimer_cancel(&apic->lapic_timer.timer);
 
-		if (!apic_sw_enabled(apic))
+		if (!kvm_apic_sw_enabled(apic))
 			val |= APIC_LVT_MASKED;
 		val &= (apic_lvt_mask[0] | apic->lapic_timer.timer_mode_mask);
 		apic_set_reg(apic, APIC_LVTT, val);
@@ -1067,7 +1040,7 @@ static int apic_mmio_write(struct kvm_io_device *this,
 
 void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu)
 {
-	if (vcpu_has_lapic(vcpu))
+	if (kvm_vcpu_has_lapic(vcpu))
 		apic_reg_write(vcpu->arch.apic, APIC_EOI, 0);
 }
 EXPORT_SYMBOL_GPL(kvm_lapic_set_eoi);
@@ -1084,7 +1057,7 @@ void kvm_free_lapic(struct kvm_vcpu *vcpu)
 	if (!(vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE))
 		static_key_slow_dec_deferred(&apic_hw_disabled);
 
-	if (!(apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_APIC_ENABLED))
+	if (!(kvm_apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_APIC_ENABLED))
 		static_key_slow_dec_deferred(&apic_sw_disabled);
 
 	if (apic->regs)
@@ -1103,7 +1076,7 @@ u64 kvm_get_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
-	if (!vcpu_has_lapic(vcpu) || apic_lvtt_oneshot(apic) ||
+	if (!kvm_vcpu_has_lapic(vcpu) || apic_lvtt_oneshot(apic) ||
 			apic_lvtt_period(apic))
 		return 0;
 
@@ -1114,7 +1087,7 @@ void kvm_set_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu, u64 data)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
-	if (!vcpu_has_lapic(vcpu) || apic_lvtt_oneshot(apic) ||
+	if (!kvm_vcpu_has_lapic(vcpu) || apic_lvtt_oneshot(apic) ||
 			apic_lvtt_period(apic))
 		return;
 
@@ -1127,21 +1100,21 @@ void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
-	if (!vcpu_has_lapic(vcpu))
+	if (!kvm_vcpu_has_lapic(vcpu))
 		return;
 
 	apic_set_tpr(apic, ((cr8 & 0x0f) << 4)
-		     | (apic_get_reg(apic, APIC_TASKPRI) & 4));
+		     | (kvm_apic_get_reg(apic, APIC_TASKPRI) & 4));
 }
 
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu)
 {
 	u64 tpr;
 
-	if (!vcpu_has_lapic(vcpu))
+	if (!kvm_vcpu_has_lapic(vcpu))
 		return 0;
 
-	tpr = (u64) apic_get_reg(vcpu->arch.apic, APIC_TASKPRI);
+	tpr = (u64) kvm_apic_get_reg(vcpu->arch.apic, APIC_TASKPRI);
 
 	return (tpr & 0xf0) >> 4;
 }
@@ -1238,16 +1211,6 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu)
 		   vcpu->arch.apic_base, apic->base_address);
 }
 
-bool kvm_apic_present(struct kvm_vcpu *vcpu)
-{
-	return vcpu_has_lapic(vcpu) && apic_hw_enabled(vcpu->arch.apic);
-}
-
-int kvm_lapic_enabled(struct kvm_vcpu *vcpu)
-{
-	return kvm_apic_present(vcpu) && apic_sw_enabled(vcpu->arch.apic);
-}
-
 /*
  *----------------------------------------------------------------------
  * timer interface
@@ -1263,7 +1226,7 @@ int apic_has_pending_timer(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
-	if (vcpu_has_lapic(vcpu) && apic_enabled(apic) &&
+	if (kvm_vcpu_has_lapic(vcpu) && apic_enabled(apic) &&
 			apic_lvt_enabled(apic, APIC_LVTT))
 		return atomic_read(&apic->lapic_timer.pending);
 
@@ -1272,10 +1235,10 @@ int apic_has_pending_timer(struct kvm_vcpu *vcpu)
 
 int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type)
 {
-	u32 reg = apic_get_reg(apic, lvt_type);
+	u32 reg = kvm_apic_get_reg(apic, lvt_type);
 	int vector, mode, trig_mode;
 
-	if (apic_hw_enabled(apic) && !(reg & APIC_LVT_MASKED)) {
+	if (kvm_apic_hw_enabled(apic) && !(reg & APIC_LVT_MASKED)) {
 		vector = reg & APIC_VECTOR_MASK;
 		mode = reg & APIC_MODE_MASK;
 		trig_mode = reg & APIC_LVT_LEVEL_TRIGGER;
@@ -1375,23 +1338,23 @@ int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu)
 	struct kvm_lapic *apic = vcpu->arch.apic;
 	int highest_irr;
 
-	if (!vcpu_has_lapic(vcpu) || !apic_enabled(apic))
+	if (!kvm_vcpu_has_lapic(vcpu) || !apic_enabled(apic))
 		return -1;
 
 	apic_update_ppr(apic);
 	highest_irr = apic_find_highest_irr(apic);
 	if ((highest_irr == -1) ||
-	    ((highest_irr & 0xF0) <= apic_get_reg(apic, APIC_PROCPRI)))
+	    ((highest_irr & 0xF0) <= kvm_apic_get_reg(apic, APIC_PROCPRI)))
 		return -1;
 	return highest_irr;
 }
 
 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu)
 {
-	u32 lvt0 = apic_get_reg(vcpu->arch.apic, APIC_LVT0);
+	u32 lvt0 = kvm_apic_get_reg(vcpu->arch.apic, APIC_LVT0);
 	int r = 0;
 
-	if (!apic_hw_enabled(vcpu->arch.apic))
+	if (!kvm_apic_hw_enabled(vcpu->arch.apic))
 		r = 1;
 	if ((lvt0 & APIC_LVT_MASKED) == 0 &&
 	    GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT)
@@ -1403,7 +1366,7 @@ void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
-	if (!vcpu_has_lapic(vcpu))
+	if (!kvm_vcpu_has_lapic(vcpu))
 		return;
 
 	if (atomic_read(&apic->lapic_timer.pending) > 0) {
@@ -1432,7 +1395,7 @@ void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu)
 
 	kvm_lapic_set_base(vcpu, vcpu->arch.apic_base);
 	kvm_apic_set_version(vcpu);
-	apic_set_spiv(apic, apic_get_reg(apic, APIC_SPIV));
+	apic_set_spiv(apic, kvm_apic_get_reg(apic, APIC_SPIV));
 
 	apic_update_ppr(apic);
 	hrtimer_cancel(&apic->lapic_timer.timer);
@@ -1448,7 +1411,7 @@ void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
 {
 	struct hrtimer *timer;
 
-	if (!vcpu_has_lapic(vcpu))
+	if (!kvm_vcpu_has_lapic(vcpu))
 		return;
 
 	timer = &vcpu->arch.apic->lapic_timer.timer;
@@ -1549,7 +1512,7 @@ void kvm_lapic_sync_to_vapic(struct kvm_vcpu *vcpu)
 	if (!test_bit(KVM_APIC_CHECK_VAPIC, &vcpu->arch.apic_attention))
 		return;
 
-	tpr = apic_get_reg(apic, APIC_TASKPRI) & 0xff;
+	tpr = kvm_apic_get_reg(apic, APIC_TASKPRI) & 0xff;
 	max_irr = apic_find_highest_irr(apic);
 	if (max_irr < 0)
 		max_irr = 0;
@@ -1608,7 +1571,7 @@ int kvm_hv_vapic_msr_write(struct kvm_vcpu *vcpu, u32 reg, u64 data)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
-	if (!vcpu_has_lapic(vcpu))
+	if (!kvm_vcpu_has_lapic(vcpu))
 		return 1;
 
 	/* if this is ICR write vector before command */
@@ -1622,7 +1585,7 @@ int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 reg, u64 *data)
 	struct kvm_lapic *apic = vcpu->arch.apic;
 	u32 low, high = 0;
 
-	if (!vcpu_has_lapic(vcpu))
+	if (!kvm_vcpu_has_lapic(vcpu))
 		return 1;
 
 	if (apic_reg_read(apic, reg, 4, &low))
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 73fa299..2ad9caa 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -55,8 +55,6 @@ int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type);
 u64 kvm_get_apic_base(struct kvm_vcpu *vcpu);
 void kvm_set_apic_base(struct kvm_vcpu *vcpu, u64 data);
 void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu);
-int kvm_lapic_enabled(struct kvm_vcpu *vcpu);
-bool kvm_apic_present(struct kvm_vcpu *vcpu);
 int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu);
 
 u64 kvm_get_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu);
@@ -79,4 +77,47 @@ static inline bool kvm_hv_vapic_assist_page_enabled(struct kvm_vcpu *vcpu)
 
 int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data);
 void kvm_lapic_init(void);
+
+static inline u32 kvm_apic_get_reg(struct kvm_lapic *apic, int reg_off)
+{
+	        return *((u32 *) (apic->regs + reg_off));
+}
+
+extern struct static_key kvm_no_apic_vcpu;
+
+static inline bool kvm_vcpu_has_lapic(struct kvm_vcpu *vcpu)
+{
+	if (static_key_false(&kvm_no_apic_vcpu))
+		return vcpu->arch.apic;
+	return true;
+}
+
+extern struct static_key_deferred apic_hw_disabled;
+
+static inline int kvm_apic_hw_enabled(struct kvm_lapic *apic)
+{
+	if (static_key_false(&apic_hw_disabled.key))
+		return apic->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE;
+	return MSR_IA32_APICBASE_ENABLE;
+}
+
+extern struct static_key_deferred apic_sw_disabled;
+
+static inline int kvm_apic_sw_enabled(struct kvm_lapic *apic)
+{
+	if (static_key_false(&apic_sw_disabled.key))
+		return kvm_apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_APIC_ENABLED;
+	return APIC_SPIV_APIC_ENABLED;
+}
+
+static inline bool kvm_apic_present(struct kvm_vcpu *vcpu)
+{
+	return kvm_vcpu_has_lapic(vcpu) && kvm_apic_hw_enabled(vcpu->arch.apic);
+}
+
+static inline int kvm_lapic_enabled(struct kvm_vcpu *vcpu)
+{
+	return kvm_apic_present(vcpu) && kvm_apic_sw_enabled(vcpu->arch.apic);
+}
+
 #endif
-- 
1.7.10

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Avi Kivity]

On 08/05/2012 03:58 PM, Gleb Natapov wrote:
> APIC code has a lot of checks for apic presence and apic HW/SW enable
> state.  Most common configuration is when each vcpu has in kernel apic
> and it is fully enabled. This path series uses jump labels to turn checks
> to nops in the common case. 
> 
> Gleb Natapov (8):
>   KVM: clean up kvm_(set|get)_apic_base
>   KVM: use kvm_lapic_set_base() to change apic_base
>   KVM: mark apic enabled on start up.
>   Export jump_label_rate_limit()
>   KVM: use jump label to optimize checking for HW enabled APIC in
>     APIC_BASE MSR.
>   KVM: use jump label to optimize checking for SW enabled apic in
>     spurious interrupt register
>   KVM: use jump label to optimize checking for in kernel local apic
>     presence.
>   KVM: inline kvm_apic_present() and kvm_lapic_enabled()

Neat.

During guest boot up, some of these jump keys will change, no?  Does
this mean a stop_machine() or equivalent?  I'm worried about real-time
response or one guest being affected by another.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Gleb Natapov]

On Sun, Aug 05, 2012 at 04:33:02PM +0300, Avi Kivity wrote:
> On 08/05/2012 03:58 PM, Gleb Natapov wrote:
> > APIC code has a lot of checks for apic presence and apic HW/SW enable
> > state.  Most common configuration is when each vcpu has in kernel apic
> > and it is fully enabled. This path series uses jump labels to turn checks
> > to nops in the common case. 
> > 
> > Gleb Natapov (8):
> >   KVM: clean up kvm_(set|get)_apic_base
> >   KVM: use kvm_lapic_set_base() to change apic_base
> >   KVM: mark apic enabled on start up.
> >   Export jump_label_rate_limit()
> >   KVM: use jump label to optimize checking for HW enabled APIC in
> >     APIC_BASE MSR.
> >   KVM: use jump label to optimize checking for SW enabled apic in
> >     spurious interrupt register
> >   KVM: use jump label to optimize checking for in kernel local apic
> >     presence.
> >   KVM: inline kvm_apic_present() and kvm_lapic_enabled()
> 
> Neat.
> 
> During guest boot up, some of these jump keys will change, no?  Does
> this mean a stop_machine() or equivalent?  I'm worried about real-time
> response or one guest being affected by another.
> 
Yes, SW enable bit changes during boot. The jump label triggerable by a
guest are rate limited though. So stop machine will not happen more then
once per second even with malicious guests.

--
			Gleb.

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Avi Kivity]

On 08/05/2012 04:35 PM, Gleb Natapov wrote:
> On Sun, Aug 05, 2012 at 04:33:02PM +0300, Avi Kivity wrote:
>> On 08/05/2012 03:58 PM, Gleb Natapov wrote:
>> > APIC code has a lot of checks for apic presence and apic HW/SW enable
>> > state.  Most common configuration is when each vcpu has in kernel apic
>> > and it is fully enabled. This path series uses jump labels to turn checks
>> > to nops in the common case. 
>> > 
>> > Gleb Natapov (8):
>> >   KVM: clean up kvm_(set|get)_apic_base
>> >   KVM: use kvm_lapic_set_base() to change apic_base
>> >   KVM: mark apic enabled on start up.
>> >   Export jump_label_rate_limit()
>> >   KVM: use jump label to optimize checking for HW enabled APIC in
>> >     APIC_BASE MSR.
>> >   KVM: use jump label to optimize checking for SW enabled apic in
>> >     spurious interrupt register
>> >   KVM: use jump label to optimize checking for in kernel local apic
>> >     presence.
>> >   KVM: inline kvm_apic_present() and kvm_lapic_enabled()
>> 
>> Neat.
>> 
>> During guest boot up, some of these jump keys will change, no?  Does
>> this mean a stop_machine() or equivalent?  I'm worried about real-time
>> response or one guest being affected by another.
>> 
> Yes, SW enable bit changes during boot. The jump label triggerable by a
> guest are rate limited though. So stop machine will not happen more then
> once per second even with malicious guests.

I'm not talking about a malicious guest, just a guest that is booting up
normally but kills real-time response for another guest (or just induces
a large hiccup in a non-real-time guest, but we don't guarantee anything
for those).

We don't support real-time guests now, but Jan has plans.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Gleb Natapov]

On Sun, Aug 05, 2012 at 04:42:17PM +0300, Avi Kivity wrote:
> On 08/05/2012 04:35 PM, Gleb Natapov wrote:
> > On Sun, Aug 05, 2012 at 04:33:02PM +0300, Avi Kivity wrote:
> >> On 08/05/2012 03:58 PM, Gleb Natapov wrote:
> >> > APIC code has a lot of checks for apic presence and apic HW/SW enable
> >> > state.  Most common configuration is when each vcpu has in kernel apic
> >> > and it is fully enabled. This path series uses jump labels to turn checks
> >> > to nops in the common case. 
> >> > 
> >> > Gleb Natapov (8):
> >> >   KVM: clean up kvm_(set|get)_apic_base
> >> >   KVM: use kvm_lapic_set_base() to change apic_base
> >> >   KVM: mark apic enabled on start up.
> >> >   Export jump_label_rate_limit()
> >> >   KVM: use jump label to optimize checking for HW enabled APIC in
> >> >     APIC_BASE MSR.
> >> >   KVM: use jump label to optimize checking for SW enabled apic in
> >> >     spurious interrupt register
> >> >   KVM: use jump label to optimize checking for in kernel local apic
> >> >     presence.
> >> >   KVM: inline kvm_apic_present() and kvm_lapic_enabled()
> >> 
> >> Neat.
> >> 
> >> During guest boot up, some of these jump keys will change, no?  Does
> >> this mean a stop_machine() or equivalent?  I'm worried about real-time
> >> response or one guest being affected by another.
> >> 
> > Yes, SW enable bit changes during boot. The jump label triggerable by a
> > guest are rate limited though. So stop machine will not happen more then
> > once per second even with malicious guests.
> 
> I'm not talking about a malicious guest, just a guest that is booting up
> normally but kills real-time response for another guest (or just induces
> a large hiccup in a non-real-time guest, but we don't guarantee anything
> for those).
> 
> We don't support real-time guests now, but Jan has plans.
> 
For such setup jump labels have to be compiled out from the kernel
completely. Anything that calls stop_machine does not play well with
real time.

Guest can cause stop machine on boot today already by detecting PMU and
configuring NMI watchdog.

--
			Gleb.

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Avi Kivity]

On 08/05/2012 04:48 PM, Gleb Natapov wrote:
 >>
>> >> During guest boot up, some of these jump keys will change, no?  Does
>> >> this mean a stop_machine() or equivalent?  I'm worried about real-time
>> >> response or one guest being affected by another.
>> >> 
>> > Yes, SW enable bit changes during boot. The jump label triggerable by a
>> > guest are rate limited though. So stop machine will not happen more then
>> > once per second even with malicious guests.
>> 
>> I'm not talking about a malicious guest, just a guest that is booting up
>> normally but kills real-time response for another guest (or just induces
>> a large hiccup in a non-real-time guest, but we don't guarantee anything
>> for those).
>> 
>> We don't support real-time guests now, but Jan has plans.
>> 
> For such setup jump labels have to be compiled out from the kernel
> completely. Anything that calls stop_machine does not play well with
> real time.
> 
> Guest can cause stop machine on boot today already by detecting PMU and
> configuring NMI watchdog.

The host can prevent this by leaving disabling the guest pmu.  But
disabling jump labels for real-time kernels may be acceptable too.  We
can probably to it at run time by forcing the slow path at all times.
-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Gleb Natapov]

On Sun, Aug 05, 2012 at 05:00:37PM +0300, Avi Kivity wrote:
> On 08/05/2012 04:48 PM, Gleb Natapov wrote:
>  >>
> >> >> During guest boot up, some of these jump keys will change, no?  Does
> >> >> this mean a stop_machine() or equivalent?  I'm worried about real-time
> >> >> response or one guest being affected by another.
> >> >> 
> >> > Yes, SW enable bit changes during boot. The jump label triggerable by a
> >> > guest are rate limited though. So stop machine will not happen more then
> >> > once per second even with malicious guests.
> >> 
> >> I'm not talking about a malicious guest, just a guest that is booting up
> >> normally but kills real-time response for another guest (or just induces
> >> a large hiccup in a non-real-time guest, but we don't guarantee anything
> >> for those).
> >> 
> >> We don't support real-time guests now, but Jan has plans.
> >> 
> > For such setup jump labels have to be compiled out from the kernel
> > completely. Anything that calls stop_machine does not play well with
> > real time.
> > 
> > Guest can cause stop machine on boot today already by detecting PMU and
> > configuring NMI watchdog.
> 
> The host can prevent this by leaving disabling the guest pmu.  But
> disabling jump labels for real-time kernels may be acceptable too.  We
> can probably to it at run time by forcing the slow path at all times.
Yes, it is possible to add module option that will force slow path if
needed.

--
			Gleb.

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Jan Kiszka]

On 2012-08-05 16:03, Gleb Natapov wrote:
> On Sun, Aug 05, 2012 at 05:00:37PM +0300, Avi Kivity wrote:
>> On 08/05/2012 04:48 PM, Gleb Natapov wrote:
>>  >>
>>>>>> During guest boot up, some of these jump keys will change, no?  Does
>>>>>> this mean a stop_machine() or equivalent?  I'm worried about real-time
>>>>>> response or one guest being affected by another.
>>>>>>
>>>>> Yes, SW enable bit changes during boot. The jump label triggerable by a
>>>>> guest are rate limited though. So stop machine will not happen more then
>>>>> once per second even with malicious guests.
>>>>
>>>> I'm not talking about a malicious guest, just a guest that is booting up
>>>> normally but kills real-time response for another guest (or just induces
>>>> a large hiccup in a non-real-time guest, but we don't guarantee anything
>>>> for those).
>>>>
>>>> We don't support real-time guests now, but Jan has plans.
>>>>
>>> For such setup jump labels have to be compiled out from the kernel
>>> completely. Anything that calls stop_machine does not play well with
>>> real time.
>>>
>>> Guest can cause stop machine on boot today already by detecting PMU and
>>> configuring NMI watchdog.
>>
>> The host can prevent this by leaving disabling the guest pmu.  But
>> disabling jump labels for real-time kernels may be acceptable too.  We
>> can probably to it at run time by forcing the slow path at all times.
> Yes, it is possible to add module option that will force slow path if
> needed.

Should I write a patch or will you? Having host-side stop_machine due to
such common guest operations is indeed a no-go for RT.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Gleb Natapov]

On Tue, Aug 14, 2012 at 04:00:54PM +0200, Jan Kiszka wrote:
> On 2012-08-05 16:03, Gleb Natapov wrote:
> > On Sun, Aug 05, 2012 at 05:00:37PM +0300, Avi Kivity wrote:
> >> On 08/05/2012 04:48 PM, Gleb Natapov wrote:
> >>  >>
> >>>>>> During guest boot up, some of these jump keys will change, no?  Does
> >>>>>> this mean a stop_machine() or equivalent?  I'm worried about real-time
> >>>>>> response or one guest being affected by another.
> >>>>>>
> >>>>> Yes, SW enable bit changes during boot. The jump label triggerable by a
> >>>>> guest are rate limited though. So stop machine will not happen more then
> >>>>> once per second even with malicious guests.
> >>>>
> >>>> I'm not talking about a malicious guest, just a guest that is booting up
> >>>> normally but kills real-time response for another guest (or just induces
> >>>> a large hiccup in a non-real-time guest, but we don't guarantee anything
> >>>> for those).
> >>>>
> >>>> We don't support real-time guests now, but Jan has plans.
> >>>>
> >>> For such setup jump labels have to be compiled out from the kernel
> >>> completely. Anything that calls stop_machine does not play well with
> >>> real time.
> >>>
> >>> Guest can cause stop machine on boot today already by detecting PMU and
> >>> configuring NMI watchdog.
> >>
> >> The host can prevent this by leaving disabling the guest pmu.  But
> >> disabling jump labels for real-time kernels may be acceptable too.  We
> >> can probably to it at run time by forcing the slow path at all times.
> > Yes, it is possible to add module option that will force slow path if
> > needed.
> 
> Should I write a patch or will you? Having host-side stop_machine due to
> such common guest operations is indeed a no-go for RT.
> 
Do we support RT now? The operation are definitely not common.

--
			Gleb.

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Jan Kiszka]

On 2012-08-14 16:03, Gleb Natapov wrote:
> On Tue, Aug 14, 2012 at 04:00:54PM +0200, Jan Kiszka wrote:
>> On 2012-08-05 16:03, Gleb Natapov wrote:
>>> On Sun, Aug 05, 2012 at 05:00:37PM +0300, Avi Kivity wrote:
>>>> On 08/05/2012 04:48 PM, Gleb Natapov wrote:
>>>>  >>
>>>>>>>> During guest boot up, some of these jump keys will change, no?  Does
>>>>>>>> this mean a stop_machine() or equivalent?  I'm worried about real-time
>>>>>>>> response or one guest being affected by another.
>>>>>>>>
>>>>>>> Yes, SW enable bit changes during boot. The jump label triggerable by a
>>>>>>> guest are rate limited though. So stop machine will not happen more then
>>>>>>> once per second even with malicious guests.
>>>>>>
>>>>>> I'm not talking about a malicious guest, just a guest that is booting up
>>>>>> normally but kills real-time response for another guest (or just induces
>>>>>> a large hiccup in a non-real-time guest, but we don't guarantee anything
>>>>>> for those).
>>>>>>
>>>>>> We don't support real-time guests now, but Jan has plans.
>>>>>>
>>>>> For such setup jump labels have to be compiled out from the kernel
>>>>> completely. Anything that calls stop_machine does not play well with
>>>>> real time.
>>>>>
>>>>> Guest can cause stop machine on boot today already by detecting PMU and
>>>>> configuring NMI watchdog.
>>>>
>>>> The host can prevent this by leaving disabling the guest pmu.  But
>>>> disabling jump labels for real-time kernels may be acceptable too.  We
>>>> can probably to it at run time by forcing the slow path at all times.
>>> Yes, it is possible to add module option that will force slow path if
>>> needed.
>>
>> Should I write a patch or will you? Having host-side stop_machine due to
>> such common guest operations is indeed a no-go for RT.
>>
> Do we support RT now? The operation are definitely not common.

We also have min_timer_period_us to control the APIC timer - for the
same use case.

And regarding how common they are: Do standard OSes trigger any
jump-label optimized switch during at least their boot-up? I thought so.
In that case, if you co-locate RT and standard OSes on a shared host,
you would have a conflict.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Gleb Natapov]

On Tue, Aug 14, 2012 at 04:20:06PM +0200, Jan Kiszka wrote:
> On 2012-08-14 16:03, Gleb Natapov wrote:
> > On Tue, Aug 14, 2012 at 04:00:54PM +0200, Jan Kiszka wrote:
> >> On 2012-08-05 16:03, Gleb Natapov wrote:
> >>> On Sun, Aug 05, 2012 at 05:00:37PM +0300, Avi Kivity wrote:
> >>>> On 08/05/2012 04:48 PM, Gleb Natapov wrote:
> >>>>  >>
> >>>>>>>> During guest boot up, some of these jump keys will change, no?  Does
> >>>>>>>> this mean a stop_machine() or equivalent?  I'm worried about real-time
> >>>>>>>> response or one guest being affected by another.
> >>>>>>>>
> >>>>>>> Yes, SW enable bit changes during boot. The jump label triggerable by a
> >>>>>>> guest are rate limited though. So stop machine will not happen more then
> >>>>>>> once per second even with malicious guests.
> >>>>>>
> >>>>>> I'm not talking about a malicious guest, just a guest that is booting up
> >>>>>> normally but kills real-time response for another guest (or just induces
> >>>>>> a large hiccup in a non-real-time guest, but we don't guarantee anything
> >>>>>> for those).
> >>>>>>
> >>>>>> We don't support real-time guests now, but Jan has plans.
> >>>>>>
> >>>>> For such setup jump labels have to be compiled out from the kernel
> >>>>> completely. Anything that calls stop_machine does not play well with
> >>>>> real time.
> >>>>>
> >>>>> Guest can cause stop machine on boot today already by detecting PMU and
> >>>>> configuring NMI watchdog.
> >>>>
> >>>> The host can prevent this by leaving disabling the guest pmu.  But
> >>>> disabling jump labels for real-time kernels may be acceptable too.  We
> >>>> can probably to it at run time by forcing the slow path at all times.
> >>> Yes, it is possible to add module option that will force slow path if
> >>> needed.
> >>
> >> Should I write a patch or will you? Having host-side stop_machine due to
> >> such common guest operations is indeed a no-go for RT.
> >>
> > Do we support RT now? The operation are definitely not common.
> 
> We also have min_timer_period_us to control the APIC timer - for the
> same use case.
> 
> And regarding how common they are: Do standard OSes trigger any
> jump-label optimized switch during at least their boot-up? I thought so.
> In that case, if you co-locate RT and standard OSes on a shared host,
> you would have a conflict.
> 
Yes, during boot up it happens. But it is rate limited to happen not
more than once per second. But I genuinely curious does RT guest have
any RT guaranties from QEMU/kvm combination today (with of without
jump-labels)?

--
			Gleb.

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Jan Kiszka]

On 2012-08-14 16:37, Gleb Natapov wrote:
> On Tue, Aug 14, 2012 at 04:20:06PM +0200, Jan Kiszka wrote:
>> On 2012-08-14 16:03, Gleb Natapov wrote:
>>> On Tue, Aug 14, 2012 at 04:00:54PM +0200, Jan Kiszka wrote:
>>>> On 2012-08-05 16:03, Gleb Natapov wrote:
>>>>> On Sun, Aug 05, 2012 at 05:00:37PM +0300, Avi Kivity wrote:
>>>>>> On 08/05/2012 04:48 PM, Gleb Natapov wrote:
>>>>>>  >>
>>>>>>>>>> During guest boot up, some of these jump keys will change, no?  Does
>>>>>>>>>> this mean a stop_machine() or equivalent?  I'm worried about real-time
>>>>>>>>>> response or one guest being affected by another.
>>>>>>>>>>
>>>>>>>>> Yes, SW enable bit changes during boot. The jump label triggerable by a
>>>>>>>>> guest are rate limited though. So stop machine will not happen more then
>>>>>>>>> once per second even with malicious guests.
>>>>>>>>
>>>>>>>> I'm not talking about a malicious guest, just a guest that is booting up
>>>>>>>> normally but kills real-time response for another guest (or just induces
>>>>>>>> a large hiccup in a non-real-time guest, but we don't guarantee anything
>>>>>>>> for those).
>>>>>>>>
>>>>>>>> We don't support real-time guests now, but Jan has plans.
>>>>>>>>
>>>>>>> For such setup jump labels have to be compiled out from the kernel
>>>>>>> completely. Anything that calls stop_machine does not play well with
>>>>>>> real time.
>>>>>>>
>>>>>>> Guest can cause stop machine on boot today already by detecting PMU and
>>>>>>> configuring NMI watchdog.
>>>>>>
>>>>>> The host can prevent this by leaving disabling the guest pmu.  But
>>>>>> disabling jump labels for real-time kernels may be acceptable too.  We
>>>>>> can probably to it at run time by forcing the slow path at all times.
>>>>> Yes, it is possible to add module option that will force slow path if
>>>>> needed.
>>>>
>>>> Should I write a patch or will you? Having host-side stop_machine due to
>>>> such common guest operations is indeed a no-go for RT.
>>>>
>>> Do we support RT now? The operation are definitely not common.
>>
>> We also have min_timer_period_us to control the APIC timer - for the
>> same use case.
>>
>> And regarding how common they are: Do standard OSes trigger any
>> jump-label optimized switch during at least their boot-up? I thought so.
>> In that case, if you co-locate RT and standard OSes on a shared host,
>> you would have a conflict.
>>
> Yes, during boot up it happens. But it is rate limited to happen not
> more than once per second. But I genuinely curious does RT guest have
> any RT guaranties from QEMU/kvm combination today (with of without
> jump-labels)?

Yes, when avoiding userspace exits. If you have a customized RTOS guest
or are lucky with some existing one, that works pretty well for periodic
processing in the 1 ms range.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Avi Kivity]

On 08/14/2012 05:58 PM, Jan Kiszka wrote:
>>>
>>> And regarding how common they are: Do standard OSes trigger any
>>> jump-label optimized switch during at least their boot-up? I thought so.
>>> In that case, if you co-locate RT and standard OSes on a shared host,
>>> you would have a conflict.
>>>
>> Yes, during boot up it happens. But it is rate limited to happen not
>> more than once per second. But I genuinely curious does RT guest have
>> any RT guaranties from QEMU/kvm combination today (with of without
>> jump-labels)?
> 
> Yes, when avoiding userspace exits. If you have a customized RTOS guest
> or are lucky with some existing one, that works pretty well for periodic
> processing in the 1 ms range.

I'd have expected better.

For more formal support, we need some ioctl() to pre-populate mappings,
or perhaps extend mmu notifiers to report mlock un munlock operations
and take them as a hint to premap.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Jan Kiszka]

On 2012-08-14 18:21, Avi Kivity wrote:
> On 08/14/2012 05:58 PM, Jan Kiszka wrote:
>>>>
>>>> And regarding how common they are: Do standard OSes trigger any
>>>> jump-label optimized switch during at least their boot-up? I thought so.
>>>> In that case, if you co-locate RT and standard OSes on a shared host,
>>>> you would have a conflict.
>>>>
>>> Yes, during boot up it happens. But it is rate limited to happen not
>>> more than once per second. But I genuinely curious does RT guest have
>>> any RT guaranties from QEMU/kvm combination today (with of without
>>> jump-labels)?
>>
>> Yes, when avoiding userspace exits. If you have a customized RTOS guest
>> or are lucky with some existing one, that works pretty well for periodic
>> processing in the 1 ms range.
> 
> I'd have expected better.

It can be but I would not yet count on it - therefore this conservative
number. Also, the number of VM exists per period is, of course, an
increasingly relevant factor when going down with the cycle time. So,
actually, you can only define the latency as function of various
workload parameters.

> 
> For more formal support, we need some ioctl() to pre-populate mappings,
> or perhaps extend mmu notifiers to report mlock un munlock operations
> and take them as a hint to premap.

Of course, we are doing mlockall(CURRENT|FUTURE) for RT. As unpopulated
mappings it didn't show up as latency source so far, I haven't looked at
this closer. Which mappings could still be unpopulated?

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Avi Kivity]

On 08/14/2012 07:38 PM, Jan Kiszka wrote:
> On 2012-08-14 18:21, Avi Kivity wrote:
>> On 08/14/2012 05:58 PM, Jan Kiszka wrote:
>>>>>
>>>>> And regarding how common they are: Do standard OSes trigger any
>>>>> jump-label optimized switch during at least their boot-up? I thought so.
>>>>> In that case, if you co-locate RT and standard OSes on a shared host,
>>>>> you would have a conflict.
>>>>>
>>>> Yes, during boot up it happens. But it is rate limited to happen not
>>>> more than once per second. But I genuinely curious does RT guest have
>>>> any RT guaranties from QEMU/kvm combination today (with of without
>>>> jump-labels)?
>>>
>>> Yes, when avoiding userspace exits. If you have a customized RTOS guest
>>> or are lucky with some existing one, that works pretty well for periodic
>>> processing in the 1 ms range.
>> 
>> I'd have expected better.
> 
> It can be but I would not yet count on it - therefore this conservative
> number. Also, the number of VM exists per period is, of course, an
> increasingly relevant factor when going down with the cycle time. So,
> actually, you can only define the latency as function of various
> workload parameters.

Right.  These can be divided into guest induced exits (like APIC writes)
and host induced exits (like external interrupts).  The former would be
more predictable, while the latter can be part of the background load,
so I expect them to be more problematic.  Does this match your experience?

There is ongoing work to reduce unneeded IPIs, or confine them to
affected processors, this should help us in the long run.

> 
>> 
>> For more formal support, we need some ioctl() to pre-populate mappings,
>> or perhaps extend mmu notifiers to report mlock un munlock operations
>> and take them as a hint to premap.
> 
> Of course, we are doing mlockall(CURRENT|FUTURE) for RT. As unpopulated
> mappings it didn't show up as latency source so far, I haven't looked at
> this closer. Which mappings could still be unpopulated?

I expect none, since the guest application probably touches all memory
as part of initialization.  That's what I meant by "formal": it's a
latency source that doesn't occur in practice, but the host cannot
guarantee that.

We need to examine anything that can synchronize_rcu().  Luckily I don't
think we have anything like that in normal paths anymore.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Jan Kiszka]

On 2012-08-14 19:00, Avi Kivity wrote:
> On 08/14/2012 07:38 PM, Jan Kiszka wrote:
>> On 2012-08-14 18:21, Avi Kivity wrote:
>>> On 08/14/2012 05:58 PM, Jan Kiszka wrote:
>>>>>>
>>>>>> And regarding how common they are: Do standard OSes trigger any
>>>>>> jump-label optimized switch during at least their boot-up? I thought so.
>>>>>> In that case, if you co-locate RT and standard OSes on a shared host,
>>>>>> you would have a conflict.
>>>>>>
>>>>> Yes, during boot up it happens. But it is rate limited to happen not
>>>>> more than once per second. But I genuinely curious does RT guest have
>>>>> any RT guaranties from QEMU/kvm combination today (with of without
>>>>> jump-labels)?
>>>>
>>>> Yes, when avoiding userspace exits. If you have a customized RTOS guest
>>>> or are lucky with some existing one, that works pretty well for periodic
>>>> processing in the 1 ms range.
>>>
>>> I'd have expected better.
>>
>> It can be but I would not yet count on it - therefore this conservative
>> number. Also, the number of VM exists per period is, of course, an
>> increasingly relevant factor when going down with the cycle time. So,
>> actually, you can only define the latency as function of various
>> workload parameters.
> 
> Right.  These can be divided into guest induced exits (like APIC writes)
> and host induced exits (like external interrupts).  The former would be
> more predictable, while the latter can be part of the background load,
> so I expect them to be more problematic.  Does this match your experience?

Yep. Thanks to CPU isolation, we can control the latter to some degree
as well.

> 
> There is ongoing work to reduce unneeded IPIs, or confine them to
> affected processors, this should help us in the long run.

Indeed. The vision is 100% guest/KVM load on a dedicated core.

> 
>>
>>>
>>> For more formal support, we need some ioctl() to pre-populate mappings,
>>> or perhaps extend mmu notifiers to report mlock un munlock operations
>>> and take them as a hint to premap.
>>
>> Of course, we are doing mlockall(CURRENT|FUTURE) for RT. As unpopulated
>> mappings it didn't show up as latency source so far, I haven't looked at
>> this closer. Which mappings could still be unpopulated?
> 
> I expect none, since the guest application probably touches all memory
> as part of initialization.  That's what I meant by "formal": it's a
> latency source that doesn't occur in practice, but the host cannot
> guarantee that.
> 
> We need to examine anything that can synchronize_rcu().  Luckily I don't
> think we have anything like that in normal paths anymore.

Yep, that matches our observation - in a particular setup at least.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Avi Kivity]

On 08/14/2012 05:00 PM, Jan Kiszka wrote:

>>> The host can prevent this by leaving disabling the guest pmu.  But
>>> disabling jump labels for real-time kernels may be acceptable too.  We
>>> can probably to it at run time by forcing the slow path at all times.
>> Yes, it is possible to add module option that will force slow path if
>> needed.
> 
> Should I write a patch or will you? Having host-side stop_machine due to
> such common guest operations is indeed a no-go for RT.
> 

Note that an additional patch is needed for perf, otherwise the guest
(or a user, but that's less of a concern for realtime) can easily invoke
stop_machine by configuring and unconfiguring its pmu.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Jan Kiszka]

On 2012-08-14 16:04, Avi Kivity wrote:
> On 08/14/2012 05:00 PM, Jan Kiszka wrote:
> 
>>>> The host can prevent this by leaving disabling the guest pmu.  But
>>>> disabling jump labels for real-time kernels may be acceptable too.  We
>>>> can probably to it at run time by forcing the slow path at all times.
>>> Yes, it is possible to add module option that will force slow path if
>>> needed.
>>
>> Should I write a patch or will you? Having host-side stop_machine due to
>> such common guest operations is indeed a no-go for RT.
>>
> 
> Note that an additional patch is needed for perf, otherwise the guest
> (or a user, but that's less of a concern for realtime) can easily invoke
> stop_machine by configuring and unconfiguring its pmu.

Hmm, can't the PMU be blocked by taking away some CPU features?

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Gleb Natapov]

On Tue, Aug 14, 2012 at 04:05:54PM +0200, Jan Kiszka wrote:
> On 2012-08-14 16:04, Avi Kivity wrote:
> > On 08/14/2012 05:00 PM, Jan Kiszka wrote:
> > 
> >>>> The host can prevent this by leaving disabling the guest pmu.  But
> >>>> disabling jump labels for real-time kernels may be acceptable too.  We
> >>>> can probably to it at run time by forcing the slow path at all times.
> >>> Yes, it is possible to add module option that will force slow path if
> >>> needed.
> >>
> >> Should I write a patch or will you? Having host-side stop_machine due to
> >> such common guest operations is indeed a no-go for RT.
> >>
> > 
> > Note that an additional patch is needed for perf, otherwise the guest
> > (or a user, but that's less of a concern for realtime) can easily invoke
> > stop_machine by configuring and unconfiguring its pmu.
> 
> Hmm, can't the PMU be blocked by taking away some CPU features?
> 
It can.

--
			Gleb.

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Gleb Natapov]

On Tue, Aug 14, 2012 at 05:04:16PM +0300, Avi Kivity wrote:
> On 08/14/2012 05:00 PM, Jan Kiszka wrote:
> 
> >>> The host can prevent this by leaving disabling the guest pmu.  But
> >>> disabling jump labels for real-time kernels may be acceptable too.  We
> >>> can probably to it at run time by forcing the slow path at all times.
> >> Yes, it is possible to add module option that will force slow path if
> >> needed.
> > 
> > Should I write a patch or will you? Having host-side stop_machine due to
> > such common guest operations is indeed a no-go for RT.
> > 
> 
> Note that an additional patch is needed for perf, otherwise the guest
> (or a user, but that's less of a concern for realtime) can easily invoke
> stop_machine by configuring and unconfiguring its pmu.
> 
> 
Are we talking about malicious guests? Why not compile kernel with jump
label disabled if this is serious concern?

--
			Gleb.

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Jan Kiszka]

On 2012-08-14 16:07, Gleb Natapov wrote:
> On Tue, Aug 14, 2012 at 05:04:16PM +0300, Avi Kivity wrote:
>> On 08/14/2012 05:00 PM, Jan Kiszka wrote:
>>
>>>>> The host can prevent this by leaving disabling the guest pmu.  But
>>>>> disabling jump labels for real-time kernels may be acceptable too.  We
>>>>> can probably to it at run time by forcing the slow path at all times.
>>>> Yes, it is possible to add module option that will force slow path if
>>>> needed.
>>>
>>> Should I write a patch or will you? Having host-side stop_machine due to
>>> such common guest operations is indeed a no-go for RT.
>>>
>>
>> Note that an additional patch is needed for perf, otherwise the guest
>> (or a user, but that's less of a concern for realtime) can easily invoke
>> stop_machine by configuring and unconfiguring its pmu.
>>
>>
> Are we talking about malicious guests? Why not compile kernel with jump
> label disabled if this is serious concern?

Because jump labels are still useful for other purposes (e.g. tracing) -
provided you don't use them while a critical operation is running. It's
cumbersome to require static configuration, specifically given that we
could easily control dynamically it at KVM level.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Gleb Natapov]

On Tue, Aug 14, 2012 at 04:13:17PM +0200, Jan Kiszka wrote:
> On 2012-08-14 16:07, Gleb Natapov wrote:
> > On Tue, Aug 14, 2012 at 05:04:16PM +0300, Avi Kivity wrote:
> >> On 08/14/2012 05:00 PM, Jan Kiszka wrote:
> >>
> >>>>> The host can prevent this by leaving disabling the guest pmu.  But
> >>>>> disabling jump labels for real-time kernels may be acceptable too.  We
> >>>>> can probably to it at run time by forcing the slow path at all times.
> >>>> Yes, it is possible to add module option that will force slow path if
> >>>> needed.
> >>>
> >>> Should I write a patch or will you? Having host-side stop_machine due to
> >>> such common guest operations is indeed a no-go for RT.
> >>>
> >>
> >> Note that an additional patch is needed for perf, otherwise the guest
> >> (or a user, but that's less of a concern for realtime) can easily invoke
> >> stop_machine by configuring and unconfiguring its pmu.
> >>
> >>
> > Are we talking about malicious guests? Why not compile kernel with jump
> > label disabled if this is serious concern?
> 
> Because jump labels are still useful for other purposes (e.g. tracing) -
> provided you don't use them while a critical operation is running. It's
> cumbersome to require static configuration, specifically given that we
> could easily control dynamically it at KVM level.
> 
I am not against parameter. I just think you are fighting uphill battle.
jump-label is not trace only thing. Their use expect to grow in kernel
and those that this patchset adds are not the first that can be triggered
by regular user. BTW there is work to make patching without calling
stop_machine().

--
			Gleb.

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Jan Kiszka]

On 2012-08-14 16:44, Gleb Natapov wrote:
> On Tue, Aug 14, 2012 at 04:13:17PM +0200, Jan Kiszka wrote:
>> On 2012-08-14 16:07, Gleb Natapov wrote:
>>> On Tue, Aug 14, 2012 at 05:04:16PM +0300, Avi Kivity wrote:
>>>> On 08/14/2012 05:00 PM, Jan Kiszka wrote:
>>>>
>>>>>>> The host can prevent this by leaving disabling the guest pmu.  But
>>>>>>> disabling jump labels for real-time kernels may be acceptable too.  We
>>>>>>> can probably to it at run time by forcing the slow path at all times.
>>>>>> Yes, it is possible to add module option that will force slow path if
>>>>>> needed.
>>>>>
>>>>> Should I write a patch or will you? Having host-side stop_machine due to
>>>>> such common guest operations is indeed a no-go for RT.
>>>>>
>>>>
>>>> Note that an additional patch is needed for perf, otherwise the guest
>>>> (or a user, but that's less of a concern for realtime) can easily invoke
>>>> stop_machine by configuring and unconfiguring its pmu.
>>>>
>>>>
>>> Are we talking about malicious guests? Why not compile kernel with jump
>>> label disabled if this is serious concern?
>>
>> Because jump labels are still useful for other purposes (e.g. tracing) -
>> provided you don't use them while a critical operation is running. It's
>> cumbersome to require static configuration, specifically given that we
>> could easily control dynamically it at KVM level.
>>
> I am not against parameter. I just think you are fighting uphill battle.
> jump-label is not trace only thing. Their use expect to grow in kernel
> and those that this patchset adds are not the first that can be triggered
> by regular user. BTW there is work to make patching without calling
> stop_machine().

Yes, there might be more, but not under direct control of the guests,
typically the workload that you don't have under the same control as
your host processes.

Will the stop_machine-free version hit the kernel together or shortly
after your changes? I didn't follow that work.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Eric Northup]

On Sun, Aug 5, 2012 at 5:58 AM, Gleb Natapov <gleb@redhat.com> wrote:
> APIC code has a lot of checks for apic presence and apic HW/SW enable
> state.  Most common configuration is when each vcpu has in kernel apic
> and it is fully enabled. This path series uses jump labels to turn checks
> to nops in the common case.

What is the target workload and how does the performance compare?  As
a naive question, how different is it than just using gcc branch
hints?

[...]
-- 
Typing one-handed, please don't mistake brevity for rudeness.

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Avi Kivity]

On 08/05/2012 10:30 PM, Eric Northup wrote:
> On Sun, Aug 5, 2012 at 5:58 AM, Gleb Natapov <gleb@redhat.com> wrote:
>> APIC code has a lot of checks for apic presence and apic HW/SW enable
>> state.  Most common configuration is when each vcpu has in kernel apic
>> and it is fully enabled. This path series uses jump labels to turn checks
>> to nops in the common case.
> 
> What is the target workload and how does the performance compare?  As
> a naive question, how different is it than just using gcc branch
> hints?

We saw about 1.3% spent in kvm_apic_present() in some OLTP benchmark.

I imagine most of it would be gone by just inlining.  An exception would
be the call from kvm_irq_delivery_to_apic() which happens from a
different cpu, possibly causing a cache miss if something in the same
cache line as vcpu->arch.apic is changed frequently.

Come to think of it, if we implemented the irq routing cache (which
eliminates the loop in kvm_irq_delivery_to_apic()) we might improve
things quite a lot as well.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Gleb Natapov]

On Sun, Aug 05, 2012 at 12:30:49PM -0700, Eric Northup wrote:
> On Sun, Aug 5, 2012 at 5:58 AM, Gleb Natapov <gleb@redhat.com> wrote:
> > APIC code has a lot of checks for apic presence and apic HW/SW enable
> > state.  Most common configuration is when each vcpu has in kernel apic
> > and it is fully enabled. This path series uses jump labels to turn checks
> > to nops in the common case.
> 
> What is the target workload and how does the performance compare?
Since those patches micro optimize interrupt injection path workload
with a lot of interrupts and a lot of cpus is the target workload. I
ran a simple test that transfers big file from gust (with 16 vcpus)
to a host. The workload generates ~40000 interrupt per second (this is
with userspace networking). Using perf I checked how functions affected
by the change perform with and without the patches:

Without:
0.85% kvm_apic_present
0.45% __apic_accept_irq
0.30% kvm_apic_has_interrupt
0.20% kvm_irq_delivery_to_apic
0.16% apic_has_pending_timer
0.09% apic_mmio_write
0.04% kvm_apic_accept_pic_intr
0.04% kvm_lapic_get_cr8
0.04% kvm_lapic_enabled

With:
0.61%   kvm_irq_delivery_to_apic
0.49%   __apic_accept_irq
0.12%   apic_has_pending_timer
0.11%   kvm_apic_has_interrupt
0.04%   kvm_apic_accept_pic_intr
0.03%   apic_mmio_write
0.02%   kvm_lapic_get_cr8

>                                                                    As
> a naive question, how different is it than just using gcc branch
> hints?
> 
If I add likely() annotations to kvm_apic_present() gcc generates
exactly same code.

--
			Gleb.

Re: [PATCH 0/8] use jump labels to streamline common APIC configuration

[Avi Kivity]

On 08/05/2012 03:58 PM, Gleb Natapov wrote:
> APIC code has a lot of checks for apic presence and apic HW/SW enable
> state.  Most common configuration is when each vcpu has in kernel apic
> and it is fully enabled. This path series uses jump labels to turn checks
> to nops in the common case. 

Okay, given the perf results, the fact that we can disable this at
runtime if needed, and the fact that userspace can already toggle static
keys without privileges, I applied the patchset.  I don't think we can
improve much on the naming issue, so I just let it be.


-- 
error compiling committee.c: too many arguments to function