[PATCH] KVM: x86: check ISR and TMR to construct eoi exit bitmap

[PATCH] KVM: x86: check ISR and TMR to construct eoi exit bitmap

[Yang Zhang]

From: Yang Zhang <yang.z.zhang@Intel.com>

Guest may mask the IOAPIC entry before issue EOI. In such case,
EOI will not be intercepted by hypervisor due to the corrensponding
bit in eoi exit bitmap is not setting.

The solution is to check ISR + TMR to construct the EOI exit bitmap.

This patch is a better fixing for the issue that commit "0f6c0a740b"
tries to solve.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 arch/x86/kvm/lapic.c |   22 ++++++++++++++++++++++
 arch/x86/kvm/lapic.h |    2 ++
 arch/x86/kvm/x86.c   |    9 +++++++++
 virt/kvm/ioapic.c    |    7 ++++---
 4 files changed, 37 insertions(+), 3 deletions(-)

hi, alex

This patch is not tested since i don't have the environment to do it. Can you
help to test it? thanks.

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 08e8a89..d2f9a6e 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -71,6 +71,11 @@
 #define VEC_POS(v) ((v) & (32 - 1))
 #define REG_POS(v) (((v) >> 5) << 4)
 
+static inline u32 apic_read_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;
@@ -515,6 +520,23 @@ static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu)
 	__clear_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention);
 }
 
+void kvm_apic_zap_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
+		u32 *tmr)
+{
+	u32 i, reg_off, intr_in_service[8];
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	for (i = 0; i < 8; i++) {
+		reg_off = 0x10 * i;
+		intr_in_service[i] = apic_read_reg(apic, APIC_ISR + reg_off) &
+			apic_read_reg(apic, APIC_TMR + reg_off);
+		if (intr_in_service[i]) {
+			*((u32 *)eoi_exit_bitmap + i) |= intr_in_service[i];
+			tmr[i] |= intr_in_service[i];
+		}
+	}
+}
+
 void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 6a11845..4ee3d70 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -53,6 +53,8 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value);
 u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu);
 void kvm_apic_set_version(struct kvm_vcpu *vcpu);
 
+void kvm_apic_zap_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
+		u32 *tmr);
 void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr);
 void kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir);
 int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 204422d..755b556 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6005,6 +6005,15 @@ static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
 	memset(tmr, 0, 32);
 
 	kvm_ioapic_scan_entry(vcpu, eoi_exit_bitmap, tmr);
+	/*
+	 * Guest may mask the IOAPIC entry before issue EOI. In such case,
+	 * EOI will not be intercepted by hypervisor due to the corrensponding
+	 * bit in eoi exit bitmap is not setting.
+	 *
+	 * The solution is to check ISR + TMR to construct the EOI exit bitmap.
+	 */
+	kvm_apic_zap_eoi_exitmap(vcpu, eoi_exit_bitmap, tmr);
+
 	kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
 	kvm_apic_update_tmr(vcpu, tmr);
 }
diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index e8ce34c..2458a1d 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -254,9 +254,10 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
 	spin_lock(&ioapic->lock);
 	for (index = 0; index < IOAPIC_NUM_PINS; index++) {
 		e = &ioapic->redirtbl[index];
-		if (e->fields.trig_mode == IOAPIC_LEVEL_TRIG ||
-		    kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC, index) ||
-		    index == RTC_GSI) {
+		if (!e->fields.mask &&
+			(e->fields.trig_mode == IOAPIC_LEVEL_TRIG ||
+			 kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC,
+				 index) || index == RTC_GSI)) {
 			if (kvm_apic_match_dest(vcpu, NULL, 0,
 				e->fields.dest_id, e->fields.dest_mode)) {
 				__set_bit(e->fields.vector,
-- 
1.7.1

Re: [PATCH] KVM: x86: check ISR and TMR to construct eoi exit bitmap

[Chen, Tiejun]

On 2014/8/8 14:02, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
>
> Guest may mask the IOAPIC entry before issue EOI. In such case,
> EOI will not be intercepted by hypervisor due to the corrensponding
> bit in eoi exit bitmap is not setting.
>
> The solution is to check ISR + TMR to construct the EOI exit bitmap.
>
> This patch is a better fixing for the issue that commit "0f6c0a740b"
> tries to solve.
>
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>   arch/x86/kvm/lapic.c |   22 ++++++++++++++++++++++
>   arch/x86/kvm/lapic.h |    2 ++
>   arch/x86/kvm/x86.c   |    9 +++++++++
>   virt/kvm/ioapic.c    |    7 ++++---
>   4 files changed, 37 insertions(+), 3 deletions(-)
>
> hi, alex
>
> This patch is not tested since i don't have the environment to do it. Can you
> help to test it? thanks.
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 08e8a89..d2f9a6e 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -71,6 +71,11 @@
>   #define VEC_POS(v) ((v) & (32 - 1))
>   #define REG_POS(v) (((v) >> 5) << 4)
>
> +static inline u32 apic_read_reg(struct kvm_lapic *apic, int reg_off)
> +{
> +	return *((u32 *) (apic->regs + reg_off));
> +}
> +

I think we already define the same in the arch/x86/kvm/lapic.h file,

static inline u32 kvm_apic_get_reg(struct kvm_lapic *apic, int reg_off)
{
             return *((u32 *) (apic->regs + reg_off));
}

Thanks
Tiejun

>   static inline void apic_set_reg(struct kvm_lapic *apic, int reg_off, u32 val)
>   {
>   	*((u32 *) (apic->regs + reg_off)) = val;
> @@ -515,6 +520,23 @@ static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu)
>   	__clear_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention);
>   }
>
> +void kvm_apic_zap_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
> +		u32 *tmr)
> +{
> +	u32 i, reg_off, intr_in_service[8];
> +	struct kvm_lapic *apic = vcpu->arch.apic;
> +
> +	for (i = 0; i < 8; i++) {
> +		reg_off = 0x10 * i;
> +		intr_in_service[i] = apic_read_reg(apic, APIC_ISR + reg_off) &
> +			apic_read_reg(apic, APIC_TMR + reg_off);
> +		if (intr_in_service[i]) {
> +			*((u32 *)eoi_exit_bitmap + i) |= intr_in_service[i];
> +			tmr[i] |= intr_in_service[i];
> +		}
> +	}
> +}
> +
>   void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr)
>   {
>   	struct kvm_lapic *apic = vcpu->arch.apic;
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 6a11845..4ee3d70 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -53,6 +53,8 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value);
>   u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu);
>   void kvm_apic_set_version(struct kvm_vcpu *vcpu);
>
> +void kvm_apic_zap_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
> +		u32 *tmr);
>   void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr);
>   void kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir);
>   int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 204422d..755b556 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6005,6 +6005,15 @@ static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
>   	memset(tmr, 0, 32);
>
>   	kvm_ioapic_scan_entry(vcpu, eoi_exit_bitmap, tmr);
> +	/*
> +	 * Guest may mask the IOAPIC entry before issue EOI. In such case,
> +	 * EOI will not be intercepted by hypervisor due to the corrensponding
> +	 * bit in eoi exit bitmap is not setting.
> +	 *
> +	 * The solution is to check ISR + TMR to construct the EOI exit bitmap.
> +	 */
> +	kvm_apic_zap_eoi_exitmap(vcpu, eoi_exit_bitmap, tmr);
> +
>   	kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
>   	kvm_apic_update_tmr(vcpu, tmr);
>   }
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index e8ce34c..2458a1d 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -254,9 +254,10 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
>   	spin_lock(&ioapic->lock);
>   	for (index = 0; index < IOAPIC_NUM_PINS; index++) {
>   		e = &ioapic->redirtbl[index];
> -		if (e->fields.trig_mode == IOAPIC_LEVEL_TRIG ||
> -		    kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC, index) ||
> -		    index == RTC_GSI) {
> +		if (!e->fields.mask &&
> +			(e->fields.trig_mode == IOAPIC_LEVEL_TRIG ||
> +			 kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC,
> +				 index) || index == RTC_GSI)) {
>   			if (kvm_apic_match_dest(vcpu, NULL, 0,
>   				e->fields.dest_id, e->fields.dest_mode)) {
>   				__set_bit(e->fields.vector,
>

Re: [PATCH] KVM: x86: check ISR and TMR to construct eoi exit bitmap

[Alex Williamson]

On Fri, 2014-08-08 at 14:02 +0800, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> Guest may mask the IOAPIC entry before issue EOI. In such case,
> EOI will not be intercepted by hypervisor due to the corrensponding
> bit in eoi exit bitmap is not setting.
> 
> The solution is to check ISR + TMR to construct the EOI exit bitmap.
> 
> This patch is a better fixing for the issue that commit "0f6c0a740b"
> tries to solve.
> 
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  arch/x86/kvm/lapic.c |   22 ++++++++++++++++++++++
>  arch/x86/kvm/lapic.h |    2 ++
>  arch/x86/kvm/x86.c   |    9 +++++++++
>  virt/kvm/ioapic.c    |    7 ++++---
>  4 files changed, 37 insertions(+), 3 deletions(-)
> 
> hi, alex
> 
> This patch is not tested since i don't have the environment to do it. Can you
> help to test it? thanks.

Yes, this also fixes the problem.

> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 08e8a89..d2f9a6e 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
...
> @@ -515,6 +520,23 @@ static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu)
>  	__clear_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention);
>  }
>  
> +void kvm_apic_zap_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
> +		u32 *tmr)
> +{
> +	u32 i, reg_off, intr_in_service[8];

There's no reason for intr_in_service to be an array here.  Thanks,

Alex

> +	struct kvm_lapic *apic = vcpu->arch.apic;
> +
> +	for (i = 0; i < 8; i++) {
> +		reg_off = 0x10 * i;
> +		intr_in_service[i] = apic_read_reg(apic, APIC_ISR + reg_off) &
> +			apic_read_reg(apic, APIC_TMR + reg_off);
> +		if (intr_in_service[i]) {
> +			*((u32 *)eoi_exit_bitmap + i) |= intr_in_service[i];
> +			tmr[i] |= intr_in_service[i];
> +		}
> +	}
> +}
> +