[PATCHv2] kvm: optimize ISR lookups

[PATCHv2] kvm: optimize ISR lookups

[Michael S. Tsirkin]

We perform ISR lookups twice: during interrupt
injection and on EOI. Typical workloads only have
a single bit set there. So we can avoid ISR scans by
1. counting bits as we set/clear them in ISR
2. if count is 1, caching the vector number
3. if count != 1, invalidating the cache

The real purpose of this is enabling PV EOI
which needs to quickly validate the vector.
But non PV guests might also benefit.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

I am well aware of Thomas and Peter's suggestion of reworking APIC
register handling in kvm instead of adding a cache like this patch does.

This revision does *not* address that comment yet: it only corrects a
bug in the original patch.

Posting in this form for ease of testing.

Changes from v1:
	replace ASSERT by BUG_ON, correcting inverted logic

 arch/x86/kvm/lapic.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++++-
 arch/x86/kvm/lapic.h |    2 +
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 93c1574..0d2985d 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -107,6 +107,16 @@ static inline void apic_clear_vector(int vec, void *bitmap)
 	clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
 }
 
+static inline int __apic_test_and_set_vector(int vec, void *bitmap)
+{
+	return __test_and_set_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
+}
+
+static inline int __apic_test_and_clear_vector(int vec, void *bitmap)
+{
+	return __test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
+}
+
 static inline int apic_hw_enabled(struct kvm_lapic *apic)
 {
 	return (apic)->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE;
@@ -210,6 +220,16 @@ static int find_highest_vector(void *bitmap)
 		return fls(word[word_offset << 2]) - 1 + (word_offset << 5);
 }
 
+static u8 count_vectors(void *bitmap)
+{
+	u32 *word = bitmap;
+	int word_offset;
+	u8 count = 0;
+	for (word_offset = 0; word_offset < MAX_APIC_VECTOR >> 5; ++word_offset)
+		count += hweight32(word[word_offset << 2]);
+	return count;
+}
+
 static inline int apic_test_and_set_irr(int vec, struct kvm_lapic *apic)
 {
 	apic->irr_pending = true;
@@ -242,6 +262,25 @@ static inline void apic_clear_irr(int vec, struct kvm_lapic *apic)
 		apic->irr_pending = true;
 }
 
+static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
+{
+	if (!__apic_test_and_set_vector(vec, apic->regs + APIC_ISR))
+		++apic->isr_count;
+	BUG_ON(apic->isr_count > MAX_APIC_VECTOR);
+	if (likely(apic->isr_count == 1))
+		apic->isr_cache = vec;
+	else
+		apic->isr_cache = -1;
+}
+
+static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
+{
+	if (__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
+		--apic->isr_count;
+	BUG_ON(apic->isr_count < 0);
+	apic->isr_cache = -1;
+}
+
 int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
@@ -273,6 +312,10 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq)
 static inline int apic_find_highest_isr(struct kvm_lapic *apic)
 {
 	int result;
+	if (!apic->isr_count)
+		return -1;
+	if (likely(apic->isr_cache != -1))
+		return apic->isr_cache;
 
 	result = find_highest_vector(apic->regs + APIC_ISR);
 	ASSERT(result == -1 || result >= 16);
@@ -492,7 +535,7 @@ static void apic_set_eoi(struct kvm_lapic *apic)
 	if (vector == -1)
 		return;
 
-	apic_clear_vector(vector, apic->regs + APIC_ISR);
+	apic_clear_isr(vector, apic);
 	apic_update_ppr(apic);
 
 	if (!(apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI) &&
@@ -1081,6 +1124,8 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu)
 		apic_set_reg(apic, APIC_TMR + 0x10 * i, 0);
 	}
 	apic->irr_pending = false;
+	apic->isr_count = 0;
+	apic->isr_cache = -1;
 	update_divide_count(apic);
 	atomic_set(&apic->lapic_timer.pending, 0);
 	if (kvm_vcpu_is_bsp(vcpu))
@@ -1248,7 +1293,7 @@ int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu)
 	if (vector == -1)
 		return -1;
 
-	apic_set_vector(vector, apic->regs + APIC_ISR);
+	apic_set_isr(vector, apic);
 	apic_update_ppr(apic);
 	apic_clear_irr(vector, apic);
 	return vector;
@@ -1267,6 +1312,8 @@ void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu)
 	update_divide_count(apic);
 	start_apic_timer(apic);
 	apic->irr_pending = true;
+	apic->isr_count = count_vectors(apic->regs + APIC_ISR);
+	apic->isr_cache = -1;
 	kvm_make_request(KVM_REQ_EVENT, vcpu);
 }
 
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 6f4ce25..9f8deff 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -13,6 +13,8 @@ struct kvm_lapic {
 	u32 divide_count;
 	struct kvm_vcpu *vcpu;
 	bool irr_pending;
+	s16 isr_count;
+	int isr_cache;
 	void *regs;
 	gpa_t vapic_addr;
 	struct page *vapic_page;
-- 
MST

Re: [PATCHv2] kvm: optimize ISR lookups

[Marcelo Tosatti]

On Tue, May 22, 2012 at 03:54:56PM +0300, Michael S. Tsirkin wrote:
> We perform ISR lookups twice: during interrupt
> injection and on EOI. Typical workloads only have
> a single bit set there. So we can avoid ISR scans by
> 1. counting bits as we set/clear them in ISR
> 2. if count is 1, caching the vector number
> 3. if count != 1, invalidating the cache
> 
> The real purpose of this is enabling PV EOI
> which needs to quickly validate the vector.
> But non PV guests might also benefit.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> I am well aware of Thomas and Peter's suggestion of reworking APIC
> register handling in kvm instead of adding a cache like this patch does.
> 
> This revision does *not* address that comment yet: it only corrects a
> bug in the original patch.
> 
> Posting in this form for ease of testing.
> 
> Changes from v1:
> 	replace ASSERT by BUG_ON, correcting inverted logic
> 
>  arch/x86/kvm/lapic.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  arch/x86/kvm/lapic.h |    2 +
>  2 files changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 93c1574..0d2985d 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -107,6 +107,16 @@ static inline void apic_clear_vector(int vec, void *bitmap)
>  	clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
>  }
>  
> +static inline int __apic_test_and_set_vector(int vec, void *bitmap)
> +{
> +	return __test_and_set_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
> +}
> +
> +static inline int __apic_test_and_clear_vector(int vec, void *bitmap)
> +{
> +	return __test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
> +}
> +
>  static inline int apic_hw_enabled(struct kvm_lapic *apic)
>  {
>  	return (apic)->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE;
> @@ -210,6 +220,16 @@ static int find_highest_vector(void *bitmap)
>  		return fls(word[word_offset << 2]) - 1 + (word_offset << 5);
>  }
>  
> +static u8 count_vectors(void *bitmap)
> +{
> +	u32 *word = bitmap;
> +	int word_offset;
> +	u8 count = 0;
> +	for (word_offset = 0; word_offset < MAX_APIC_VECTOR >> 5; ++word_offset)
> +		count += hweight32(word[word_offset << 2]);
> +	return count;
> +}
> +
>  static inline int apic_test_and_set_irr(int vec, struct kvm_lapic *apic)
>  {
>  	apic->irr_pending = true;
> @@ -242,6 +262,25 @@ static inline void apic_clear_irr(int vec, struct kvm_lapic *apic)
>  		apic->irr_pending = true;
>  }
>  
> +static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
> +{
> +	if (!__apic_test_and_set_vector(vec, apic->regs + APIC_ISR))
> +		++apic->isr_count;
> +	BUG_ON(apic->isr_count > MAX_APIC_VECTOR);
> +	if (likely(apic->isr_count == 1))
> +		apic->isr_cache = vec;
> +	else
> +		apic->isr_cache = -1;
> +}
> +
> +static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
> +{
> +	if (__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
> +		--apic->isr_count;
> +	BUG_ON(apic->isr_count < 0);
> +	apic->isr_cache = -1;
> +}
> +
>  int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_lapic *apic = vcpu->arch.apic;
> @@ -273,6 +312,10 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq)
>  static inline int apic_find_highest_isr(struct kvm_lapic *apic)
>  {
>  	int result;
> +	if (!apic->isr_count)
> +		return -1;
> +	if (likely(apic->isr_cache != -1))

assert(isr_count == 1).

Looks fine otherwise. Gleb can you review please?

Re: [PATCHv2] kvm: optimize ISR lookups

[Gleb Natapov]

On Wed, May 30, 2012 at 05:54:42PM -0300, Marcelo Tosatti wrote:
> On Tue, May 22, 2012 at 03:54:56PM +0300, Michael S. Tsirkin wrote:
> > We perform ISR lookups twice: during interrupt
> > injection and on EOI. Typical workloads only have
> > a single bit set there. So we can avoid ISR scans by
> > 1. counting bits as we set/clear them in ISR
> > 2. if count is 1, caching the vector number
> > 3. if count != 1, invalidating the cache
> > 
> > The real purpose of this is enabling PV EOI
> > which needs to quickly validate the vector.
> > But non PV guests might also benefit.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > 
> > I am well aware of Thomas and Peter's suggestion of reworking APIC
> > register handling in kvm instead of adding a cache like this patch does.
> > 
> > This revision does *not* address that comment yet: it only corrects a
> > bug in the original patch.
> > 
> > Posting in this form for ease of testing.
> > 
> > Changes from v1:
> > 	replace ASSERT by BUG_ON, correcting inverted logic
> > 
> >  arch/x86/kvm/lapic.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >  arch/x86/kvm/lapic.h |    2 +
> >  2 files changed, 51 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 93c1574..0d2985d 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -107,6 +107,16 @@ static inline void apic_clear_vector(int vec, void *bitmap)
> >  	clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
> >  }
> >  
> > +static inline int __apic_test_and_set_vector(int vec, void *bitmap)
> > +{
> > +	return __test_and_set_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
> > +}
> > +
> > +static inline int __apic_test_and_clear_vector(int vec, void *bitmap)
> > +{
> > +	return __test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
> > +}
> > +
> >  static inline int apic_hw_enabled(struct kvm_lapic *apic)
> >  {
> >  	return (apic)->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE;
> > @@ -210,6 +220,16 @@ static int find_highest_vector(void *bitmap)
> >  		return fls(word[word_offset << 2]) - 1 + (word_offset << 5);
> >  }
> >  
> > +static u8 count_vectors(void *bitmap)
> > +{
> > +	u32 *word = bitmap;
> > +	int word_offset;
> > +	u8 count = 0;
> > +	for (word_offset = 0; word_offset < MAX_APIC_VECTOR >> 5; ++word_offset)
> > +		count += hweight32(word[word_offset << 2]);
> > +	return count;
> > +}
> > +
> >  static inline int apic_test_and_set_irr(int vec, struct kvm_lapic *apic)
> >  {
> >  	apic->irr_pending = true;
> > @@ -242,6 +262,25 @@ static inline void apic_clear_irr(int vec, struct kvm_lapic *apic)
> >  		apic->irr_pending = true;
> >  }
> >  
> > +static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
> > +{
> > +	if (!__apic_test_and_set_vector(vec, apic->regs + APIC_ISR))
> > +		++apic->isr_count;
> > +	BUG_ON(apic->isr_count > MAX_APIC_VECTOR);
> > +	if (likely(apic->isr_count == 1))
> > +		apic->isr_cache = vec;
> > +	else
> > +		apic->isr_cache = -1;
> > +}
> > +
> > +static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
> > +{
> > +	if (__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
> > +		--apic->isr_count;
> > +	BUG_ON(apic->isr_count < 0);
> > +	apic->isr_cache = -1;
> > +}
> > +
> >  int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
> >  {
> >  	struct kvm_lapic *apic = vcpu->arch.apic;
> > @@ -273,6 +312,10 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq)
> >  static inline int apic_find_highest_isr(struct kvm_lapic *apic)
> >  {
> >  	int result;
> > +	if (!apic->isr_count)
> > +		return -1;
> > +	if (likely(apic->isr_cache != -1))
> 
> assert(isr_count == 1).
> 
> Looks fine otherwise. Gleb can you review please?
I am not convinced we need to keep track of isr_count. isr_cache should be enough.
Why setting isr_cache to -1 if isr_count > 1? Just overwrite isr_cache
values with newly injected vector and use it on next EOI.

--
			Gleb.

Re: [PATCHv2] kvm: optimize ISR lookups

[Michael S. Tsirkin]

On Thu, May 31, 2012 at 01:06:57PM +0300, Gleb Natapov wrote:
> On Wed, May 30, 2012 at 05:54:42PM -0300, Marcelo Tosatti wrote:
> > On Tue, May 22, 2012 at 03:54:56PM +0300, Michael S. Tsirkin wrote:
> > > We perform ISR lookups twice: during interrupt
> > > injection and on EOI. Typical workloads only have
> > > a single bit set there. So we can avoid ISR scans by
> > > 1. counting bits as we set/clear them in ISR
> > > 2. if count is 1, caching the vector number
> > > 3. if count != 1, invalidating the cache
> > > 
> > > The real purpose of this is enabling PV EOI
> > > which needs to quickly validate the vector.
> > > But non PV guests might also benefit.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > > 
> > > I am well aware of Thomas and Peter's suggestion of reworking APIC
> > > register handling in kvm instead of adding a cache like this patch does.
> > > 
> > > This revision does *not* address that comment yet: it only corrects a
> > > bug in the original patch.
> > > 
> > > Posting in this form for ease of testing.
> > > 
> > > Changes from v1:
> > > 	replace ASSERT by BUG_ON, correcting inverted logic
> > > 
> > >  arch/x86/kvm/lapic.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  arch/x86/kvm/lapic.h |    2 +
> > >  2 files changed, 51 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > index 93c1574..0d2985d 100644
> > > --- a/arch/x86/kvm/lapic.c
> > > +++ b/arch/x86/kvm/lapic.c
> > > @@ -107,6 +107,16 @@ static inline void apic_clear_vector(int vec, void *bitmap)
> > >  	clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
> > >  }
> > >  
> > > +static inline int __apic_test_and_set_vector(int vec, void *bitmap)
> > > +{
> > > +	return __test_and_set_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
> > > +}
> > > +
> > > +static inline int __apic_test_and_clear_vector(int vec, void *bitmap)
> > > +{
> > > +	return __test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
> > > +}
> > > +
> > >  static inline int apic_hw_enabled(struct kvm_lapic *apic)
> > >  {
> > >  	return (apic)->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE;
> > > @@ -210,6 +220,16 @@ static int find_highest_vector(void *bitmap)
> > >  		return fls(word[word_offset << 2]) - 1 + (word_offset << 5);
> > >  }
> > >  
> > > +static u8 count_vectors(void *bitmap)
> > > +{
> > > +	u32 *word = bitmap;
> > > +	int word_offset;
> > > +	u8 count = 0;
> > > +	for (word_offset = 0; word_offset < MAX_APIC_VECTOR >> 5; ++word_offset)
> > > +		count += hweight32(word[word_offset << 2]);
> > > +	return count;
> > > +}
> > > +
> > >  static inline int apic_test_and_set_irr(int vec, struct kvm_lapic *apic)
> > >  {
> > >  	apic->irr_pending = true;
> > > @@ -242,6 +262,25 @@ static inline void apic_clear_irr(int vec, struct kvm_lapic *apic)
> > >  		apic->irr_pending = true;
> > >  }
> > >  
> > > +static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
> > > +{
> > > +	if (!__apic_test_and_set_vector(vec, apic->regs + APIC_ISR))
> > > +		++apic->isr_count;
> > > +	BUG_ON(apic->isr_count > MAX_APIC_VECTOR);
> > > +	if (likely(apic->isr_count == 1))
> > > +		apic->isr_cache = vec;
> > > +	else
> > > +		apic->isr_cache = -1;
> > > +}
> > > +
> > > +static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
> > > +{
> > > +	if (__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
> > > +		--apic->isr_count;
> > > +	BUG_ON(apic->isr_count < 0);
> > > +	apic->isr_cache = -1;
> > > +}
> > > +
> > >  int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
> > >  {
> > >  	struct kvm_lapic *apic = vcpu->arch.apic;
> > > @@ -273,6 +312,10 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq)
> > >  static inline int apic_find_highest_isr(struct kvm_lapic *apic)
> > >  {
> > >  	int result;
> > > +	if (!apic->isr_count)
> > > +		return -1;
> > > +	if (likely(apic->isr_cache != -1))
> > 
> > assert(isr_count == 1).
> > 
> > Looks fine otherwise. Gleb can you review please?
> I am not convinced we need to keep track of isr_count. isr_cache should be enough.
> Why setting isr_cache to -1 if isr_count > 1?

So that data path can check cache and if != -1 know it
is valid: we check the cache more than we set it.

I'll add a comment to explain this.

> Just overwrite isr_cache
> values with newly injected vector and use it on next EOI.
> --
> 			Gleb.

Re: [PATCHv2] kvm: optimize ISR lookups

[Michael S. Tsirkin]

On Thu, May 31, 2012 at 01:06:57PM +0300, Gleb Natapov wrote:
> On Wed, May 30, 2012 at 05:54:42PM -0300, Marcelo Tosatti wrote:
> > On Tue, May 22, 2012 at 03:54:56PM +0300, Michael S. Tsirkin wrote:
> > > We perform ISR lookups twice: during interrupt
> > > injection and on EOI. Typical workloads only have
> > > a single bit set there. So we can avoid ISR scans by
> > > 1. counting bits as we set/clear them in ISR
> > > 2. if count is 1, caching the vector number
> > > 3. if count != 1, invalidating the cache
> > > 
> > > The real purpose of this is enabling PV EOI
> > > which needs to quickly validate the vector.
> > > But non PV guests might also benefit.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > > 
> > > I am well aware of Thomas and Peter's suggestion of reworking APIC
> > > register handling in kvm instead of adding a cache like this patch does.
> > > 
> > > This revision does *not* address that comment yet: it only corrects a
> > > bug in the original patch.
> > > 
> > > Posting in this form for ease of testing.
> > > 
> > > Changes from v1:
> > > 	replace ASSERT by BUG_ON, correcting inverted logic
> > > 
> > >  arch/x86/kvm/lapic.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  arch/x86/kvm/lapic.h |    2 +
> > >  2 files changed, 51 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > index 93c1574..0d2985d 100644
> > > --- a/arch/x86/kvm/lapic.c
> > > +++ b/arch/x86/kvm/lapic.c
> > > @@ -107,6 +107,16 @@ static inline void apic_clear_vector(int vec, void *bitmap)
> > >  	clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
> > >  }
> > >  
> > > +static inline int __apic_test_and_set_vector(int vec, void *bitmap)
> > > +{
> > > +	return __test_and_set_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
> > > +}
> > > +
> > > +static inline int __apic_test_and_clear_vector(int vec, void *bitmap)
> > > +{
> > > +	return __test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
> > > +}
> > > +
> > >  static inline int apic_hw_enabled(struct kvm_lapic *apic)
> > >  {
> > >  	return (apic)->vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE;
> > > @@ -210,6 +220,16 @@ static int find_highest_vector(void *bitmap)
> > >  		return fls(word[word_offset << 2]) - 1 + (word_offset << 5);
> > >  }
> > >  
> > > +static u8 count_vectors(void *bitmap)
> > > +{
> > > +	u32 *word = bitmap;
> > > +	int word_offset;
> > > +	u8 count = 0;
> > > +	for (word_offset = 0; word_offset < MAX_APIC_VECTOR >> 5; ++word_offset)
> > > +		count += hweight32(word[word_offset << 2]);
> > > +	return count;
> > > +}
> > > +
> > >  static inline int apic_test_and_set_irr(int vec, struct kvm_lapic *apic)
> > >  {
> > >  	apic->irr_pending = true;
> > > @@ -242,6 +262,25 @@ static inline void apic_clear_irr(int vec, struct kvm_lapic *apic)
> > >  		apic->irr_pending = true;
> > >  }
> > >  
> > > +static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
> > > +{
> > > +	if (!__apic_test_and_set_vector(vec, apic->regs + APIC_ISR))
> > > +		++apic->isr_count;
> > > +	BUG_ON(apic->isr_count > MAX_APIC_VECTOR);
> > > +	if (likely(apic->isr_count == 1))
> > > +		apic->isr_cache = vec;
> > > +	else
> > > +		apic->isr_cache = -1;
> > > +}
> > > +
> > > +static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
> > > +{
> > > +	if (__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
> > > +		--apic->isr_count;
> > > +	BUG_ON(apic->isr_count < 0);
> > > +	apic->isr_cache = -1;
> > > +}
> > > +
> > >  int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
> > >  {
> > >  	struct kvm_lapic *apic = vcpu->arch.apic;
> > > @@ -273,6 +312,10 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq)
> > >  static inline int apic_find_highest_isr(struct kvm_lapic *apic)
> > >  {
> > >  	int result;
> > > +	if (!apic->isr_count)
> > > +		return -1;
> > > +	if (likely(apic->isr_cache != -1))
> > 
> > assert(isr_count == 1).
> > 
> > Looks fine otherwise. Gleb can you review please?
> I am not convinced we need to keep track of isr_count. isr_cache should be enough.
> Why setting isr_cache to -1 if isr_count > 1? Just overwrite isr_cache
> values with newly injected vector and use it on next EOI.

I thought about this some more. This counter is a win because
apic_find_highest_isr is called twice:

	1. apic_set_eoi calls apic_find_highest_isr before clearing

	2. apic_update_ppr calls apic_find_highest_isr after clearing

So with isr_count you never scan ISR unless there is nesting.

So we do need isr_count. I agree we can make this work for
more cases because the latest ISR set is always the
highest one. Ronen suggested such an
optimization offline too, and it makes
code simpler so I'll implement this optimization.

> --
> 			Gleb.