Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1195187 > unrolled thread

[PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted Paolo Bonzini <pbonzini@redhat.com> - 2015-07-29 15:40 +0200
    Re: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted Steve Rutherford <srutherford@google.com> - 2015-07-30 05:40 +0200
    RE: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted "Zhang, Yang Z" <yang.z.zhang@intel.com> - 2015-08-03 04:40 +0200
      Re: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted Paolo Bonzini <pbonzini@redhat.com> - 2015-08-03 10:20 +0200
        RE: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted "Zhang, Yang Z" <yang.z.zhang@intel.com> - 2015-08-03 12:30 +0200
          Re: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted Paolo Bonzini <pbonzini@redhat.com> - 2015-08-03 13:00 +0200
            RE: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted "Zhang, Yang Z" <yang.z.zhang@intel.com> - 2015-08-04 02:50 +0200
              Re: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted Paolo Bonzini <pbonzini@redhat.com> - 2015-08-04 09:10 +0200
                RE: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted "Zhang, Yang Z" <yang.z.zhang@intel.com> - 2015-08-04 09:30 +0200
    RE: [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted "Zhang, Yang Z" <yang.z.zhang@intel.com> - 2015-08-03 04:50 +0200

#1195187 — [PATCH 1/2] KVM: x86: set TMR when the interrupt is accepted

Do not compute TMR in advance.  Instead, set the TMR just before the interrupt
is accepted into the IRR.  This limits the coupling between IOAPIC and LAPIC.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/ioapic.c |  9 ++-------
 arch/x86/kvm/ioapic.h |  3 +--
 arch/x86/kvm/lapic.c  | 19 ++++++++++---------
 arch/x86/kvm/lapic.h  |  1 -
 arch/x86/kvm/x86.c    |  5 +----
 5 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index 856f79105bb5..eaf4ec38d980 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -246,8 +246,7 @@ static void update_handled_vectors(struct kvm_ioapic *ioapic)
 	smp_wmb();
 }
 
-void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
-			u32 *tmr)
+void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
 {
 	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic;
 	union kvm_ioapic_redirect_entry *e;
@@ -260,13 +259,9 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
 		    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)) {
+				e->fields.dest_id, e->fields.dest_mode))
 				__set_bit(e->fields.vector,
 					(unsigned long *)eoi_exit_bitmap);
-				if (e->fields.trig_mode == IOAPIC_LEVEL_TRIG)
-					__set_bit(e->fields.vector,
-						(unsigned long *)tmr);
-			}
 		}
 	}
 	spin_unlock(&ioapic->lock);
diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
index ca0b0b4e6256..3dbd0e2aac4e 100644
--- a/arch/x86/kvm/ioapic.h
+++ b/arch/x86/kvm/ioapic.h
@@ -120,7 +120,6 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 		struct kvm_lapic_irq *irq, unsigned long *dest_map);
 int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
 int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
-void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
-			u32 *tmr);
+void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap);
 
 #endif
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 2a5ca97c263b..9be64c77d6db 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -551,15 +551,6 @@ static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu)
 	__clear_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention);
 }
 
-void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr)
-{
-	struct kvm_lapic *apic = vcpu->arch.apic;
-	int i;
-
-	for (i = 0; i < 8; i++)
-		apic_set_reg(apic, APIC_TMR + 0x10 * i, tmr[i]);
-}
-
 static void apic_update_ppr(struct kvm_lapic *apic)
 {
 	u32 tpr, isrv, ppr, old_ppr;
@@ -781,6 +772,9 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 	case APIC_DM_LOWEST:
 		vcpu->arch.apic_arb_prio++;
 	case APIC_DM_FIXED:
+		if (unlikely(trig_mode && !level))
+			break;
+
 		/* FIXME add logic for vcpu on reset */
 		if (unlikely(!apic_enabled(apic)))
 			break;
@@ -790,6 +784,13 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 		if (dest_map)
 			__set_bit(vcpu->vcpu_id, dest_map);
 
+		if (apic_test_vector(vector, apic->regs + APIC_TMR) != !!trig_mode) {
+			if (trig_mode)
+				apic_set_vector(vector, apic->regs + APIC_TMR);
+			else
+				apic_clear_vector(vector, apic->regs + APIC_TMR);
+		}
+
 		if (kvm_x86_ops->deliver_posted_interrupt)
 			kvm_x86_ops->deliver_posted_interrupt(vcpu, vector);
 		else {
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 764037991d26..eb46d6bcaa75 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -57,7 +57,6 @@ 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_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr);
 void __kvm_apic_update_irr(u32 *pir, void *regs);
 void kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir);
 int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 23e47a0b054b..48dc954542db 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6149,17 +6149,14 @@ static void process_smi(struct kvm_vcpu *vcpu)
 static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
 {
 	u64 eoi_exit_bitmap[4];
-	u32 tmr[8];
 
 	if (!kvm_apic_hw_enabled(vcpu->arch.apic))
 		return;
 
 	memset(eoi_exit_bitmap, 0, 32);
-	memset(tmr, 0, 32);
 
-	kvm_ioapic_scan_entry(vcpu, eoi_exit_bitmap, tmr);
+	kvm_ioapic_scan_entry(vcpu, eoi_exit_bitmap);
 	kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
-	kvm_apic_update_tmr(vcpu, tmr);
 }
 
 static void kvm_vcpu_flush_tlb(struct kvm_vcpu *vcpu)
-- 
1.8.3.1


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1195699

On Wed, Jul 29, 2015 at 03:37:34PM +0200, Paolo Bonzini wrote:
> Do not compute TMR in advance.  Instead, set the TMR just before the interrupt
> is accepted into the IRR.  This limits the coupling between IOAPIC and LAPIC.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/ioapic.c |  9 ++-------
>  arch/x86/kvm/ioapic.h |  3 +--
>  arch/x86/kvm/lapic.c  | 19 ++++++++++---------
>  arch/x86/kvm/lapic.h  |  1 -
>  arch/x86/kvm/x86.c    |  5 +----
>  5 files changed, 14 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
> index 856f79105bb5..eaf4ec38d980 100644
> --- a/arch/x86/kvm/ioapic.c
> +++ b/arch/x86/kvm/ioapic.c
> @@ -246,8 +246,7 @@ static void update_handled_vectors(struct kvm_ioapic *ioapic)
>  	smp_wmb();
>  }
>  
> -void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
> -			u32 *tmr)
> +void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
>  {
>  	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic;
>  	union kvm_ioapic_redirect_entry *e;
> @@ -260,13 +259,9 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
>  		    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)) {
> +				e->fields.dest_id, e->fields.dest_mode))
>  				__set_bit(e->fields.vector,
>  					(unsigned long *)eoi_exit_bitmap);
> -				if (e->fields.trig_mode == IOAPIC_LEVEL_TRIG)
> -					__set_bit(e->fields.vector,
> -						(unsigned long *)tmr);
> -			}
>  		}
>  	}
>  	spin_unlock(&ioapic->lock);
> diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
> index ca0b0b4e6256..3dbd0e2aac4e 100644
> --- a/arch/x86/kvm/ioapic.h
> +++ b/arch/x86/kvm/ioapic.h
> @@ -120,7 +120,6 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>  		struct kvm_lapic_irq *irq, unsigned long *dest_map);
>  int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
>  int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
> -void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap,
> -			u32 *tmr);
> +void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap);
>  
>  #endif
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 2a5ca97c263b..9be64c77d6db 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -551,15 +551,6 @@ static void pv_eoi_clr_pending(struct kvm_vcpu *vcpu)
>  	__clear_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention);
>  }
>  
> -void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr)
> -{
> -	struct kvm_lapic *apic = vcpu->arch.apic;
> -	int i;
> -
> -	for (i = 0; i < 8; i++)
> -		apic_set_reg(apic, APIC_TMR + 0x10 * i, tmr[i]);
> -}
> -
>  static void apic_update_ppr(struct kvm_lapic *apic)
>  {
>  	u32 tpr, isrv, ppr, old_ppr;
> @@ -781,6 +772,9 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>  	case APIC_DM_LOWEST:
>  		vcpu->arch.apic_arb_prio++;
>  	case APIC_DM_FIXED:
> +		if (unlikely(trig_mode && !level))
> +			break;
> +
>  		/* FIXME add logic for vcpu on reset */
>  		if (unlikely(!apic_enabled(apic)))
>  			break;
> @@ -790,6 +784,13 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>  		if (dest_map)
>  			__set_bit(vcpu->vcpu_id, dest_map);
>  
> +		if (apic_test_vector(vector, apic->regs + APIC_TMR) != !!trig_mode) {
> +			if (trig_mode)
> +				apic_set_vector(vector, apic->regs + APIC_TMR);
> +			else
> +				apic_clear_vector(vector, apic->regs + APIC_TMR);
> +		}
> +
>  		if (kvm_x86_ops->deliver_posted_interrupt)
>  			kvm_x86_ops->deliver_posted_interrupt(vcpu, vector);
>  		else {
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 764037991d26..eb46d6bcaa75 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -57,7 +57,6 @@ 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_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr);
>  void __kvm_apic_update_irr(u32 *pir, void *regs);
>  void kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir);
>  int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 23e47a0b054b..48dc954542db 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6149,17 +6149,14 @@ static void process_smi(struct kvm_vcpu *vcpu)
>  static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
>  {
>  	u64 eoi_exit_bitmap[4];
> -	u32 tmr[8];
>  
>  	if (!kvm_apic_hw_enabled(vcpu->arch.apic))
>  		return;
>  
>  	memset(eoi_exit_bitmap, 0, 32);
> -	memset(tmr, 0, 32);
>  
> -	kvm_ioapic_scan_entry(vcpu, eoi_exit_bitmap, tmr);
> +	kvm_ioapic_scan_entry(vcpu, eoi_exit_bitmap);
>  	kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
> -	kvm_apic_update_tmr(vcpu, tmr);
>  }
>  
>  static void kvm_vcpu_flush_tlb(struct kvm_vcpu *vcpu)
> -- 
> 1.8.3.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thanks for writing this. This makes TMR handling far cleaner.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1198496

Paolo Bonzini wrote on 2015-07-31:
> 
> 
> On 31/07/2015 01:26, Zhang, Yang Z wrote:
>>>> Do not compute TMR in advance.  Instead, set the TMR just before
>>>> the interrupt is accepted into the IRR.  This limits the coupling
>>>> between IOAPIC and LAPIC.
>> 
>> Uh.., it back to original way which is wrong. You cannot modify the
>> apic page(here is the TMR reg) directly when the corresponding VMCS
>> may be used at same time.
> 
> Where is this documented?  The TMR is not part of the set of virtualized
> APIC registers (TPR, PPR, EOI, ISR, IRR, ICR+ICR2; SDM 29.1.1).
> 
> Only virtualized APIC register reads use the virtual TMR registers (SDM
> 29.4.2 or 29.5), but these just read data from the corresponding field
> in the virtual APIC page.

It's not only for virtual apic page. All data structures that is used by VMCS(either directly inside in VMCS or referenced by pointer in a VMCS) need follow this rule:

24.11.4 Software Access to Related Structures
In addition to data in the VMCS region itself, VMX non-root operation can be controlled by data structures that are
referenced by pointers in a VMCS (for example, the I/O bitmaps). While the pointers to these data structures are
parts of the VMCS, the data structures themselves are not. They are not accessible using VMREAD and VMWRITE
but by ordinary memory writes.
Software should ensure that each such data structure is modified only when no logical processor with a current
VMCS that references it is in VMX non-root operation. Doing otherwise may lead to unpredictable behavior
(including behaviors identified in Section 24.11.1).

> 
> Paolo


Best regards,
Yang


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1198629

On 03/08/2015 04:37, Zhang, Yang Z wrote:
>> > Only virtualized APIC register reads use the virtual TMR registers (SDM
>> > 29.4.2 or 29.5), but these just read data from the corresponding field
>> > in the virtual APIC page.
> 
> 24.11.4 Software Access to Related Structures
> In addition to data in the VMCS region itself, VMX non-root operation
> can be controlled by data structures that are referenced by pointers
> in a VMCS (for example, the I/O bitmaps). While the pointers to these
> data structures are parts of the VMCS, the data structures themselves
> are not. They are not accessible using VMREAD and VMWRITE but by
> ordinary memory writes.

I don't think the TMR fields of the virtual-APIC page are among the data
structures that _controls_ VMX non-root operations, because:

* it is not part of the virtualized APIC state is listed in 29.1.1

* read accesses from the APIC-access page simply return data from the
corresponding page offset on the virtual-APIC page using the memory
access type stored in IA32_VMX_BASIC_MSR.  I think this explicitly says
that the effects of 24.11.1 (especially non-deterministic behavior after
a write) do not apply here.

In any case, the TMR behavior introduced by the APICv patches is
completely different from the hardware behavior, so it has to be fixed.
 The alternative is to inject level-triggered interrupts synchronously,
without using posted interrupts.

I'll write some testcases to understand the functioning of TMR in the
virtual-APIC page, but the manual seems clear to me.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1198711

Paolo Bonzini wrote on 2015-08-03:
> 
> 
> On 03/08/2015 04:37, Zhang, Yang Z wrote:
>>>> Only virtualized APIC register reads use the virtual TMR
>>>> registers (SDM
>>>> 29.4.2 or 29.5), but these just read data from the corresponding
>>>> field in the virtual APIC page.
>> 
>> 24.11.4 Software Access to Related Structures In addition to data in
>> the VMCS region itself, VMX non-root operation can be controlled by
>> data structures that are referenced by pointers in a VMCS (for
>> example, the I/O bitmaps). While the pointers to these data
>> structures are parts of the VMCS, the data structures themselves are
>> not. They are not accessible using VMREAD and VMWRITE but by
>> ordinary memory writes.
> 
> I don't think the TMR fields of the virtual-APIC page are among the
> data structures that _controls_ VMX non-root operations, because:
> 
> * it is not part of the virtualized APIC state is listed in 29.1.1
> 
> * read accesses from the APIC-access page simply return data from the
> corresponding page offset on the virtual-APIC page using the memory
> access type stored in IA32_VMX_BASIC_MSR.  I think this explicitly
> says that the effects of 24.11.1 (especially non-deterministic
> behavior after a write) do not apply here.
> 
> In any case, the TMR behavior introduced by the APICv patches is
> completely different from the hardware behavior, so it has to be fixed.

But any real problem with it?

>  The alternative is to inject level-triggered interrupts
> synchronously, without using posted interrupts.
> 
> I'll write some testcases to understand the functioning of TMR in the
> virtual-APIC page, but the manual seems clear to me.

Currently, no existing hardware will use TMR and will not cause any problem.(That's the reason why we leave it in Xen).But we don't know whether future hardware will use it or not(SDM always keeps changing :)).And per 24.11.4's description, the perfect solution is don't modify it. 
btw, IIRC, only TMR doesn't follow the rule. All other VMCS accesses are issued in right VMCS context.

> 
> Paolo


Best regards,
Yang


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1198739

On 03/08/2015 12:23, Zhang, Yang Z wrote:
> > In any case, the TMR behavior introduced by the APICv patches is
> > completely different from the hardware behavior, so it has to be fixed.
> 
> But any real problem with it?

It is a problem for split irqchip, where the EOI exit bitmap can be
inferred from the IOAPIC routes but the TMR cannot.  The hardware
behavior on the other hand can be implemented purely within the LAPIC.

> >  The alternative is to inject level-triggered interrupts
> > synchronously, without using posted interrupts.
> > 
> > I'll write some testcases to understand the functioning of TMR in the
> > virtual-APIC page, but the manual seems clear to me.
> 
> Currently, no existing hardware will use TMR and will not cause any
> problem.(That's the reason why we leave it in Xen).But we don't know
> whether future hardware will use it or not(SDM always keeps changing
> :)).

But that would be covered by a different execution control (for
backwards compatibility).  We'll get there when such a feature is
introduced.

> And per 24.11.4's description, the perfect solution is don't
> modify it. btw, IIRC, only TMR doesn't follow the rule. All other
> VMCS accesses are issued in right VMCS context.

Yes, that's correct.  It's just the TMR.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199345

Paolo Bonzini wrote on 2015-08-03:
> 
> 
> On 03/08/2015 12:23, Zhang, Yang Z wrote:
>>> In any case, the TMR behavior introduced by the APICv patches is
>>> completely different from the hardware behavior, so it has to be fixed.
>> 
>> But any real problem with it?
> 
> It is a problem for split irqchip, where the EOI exit bitmap can be
> inferred from the IOAPIC routes but the TMR cannot.  The hardware
> behavior on the other hand can be implemented purely within the LAPIC.

So updating the TMR within LAPIC is the only solution to handle it?

> 
>>>  The alternative is to inject level-triggered interrupts
>>> synchronously, without using posted interrupts.
>>> 
>>> I'll write some testcases to understand the functioning of TMR in
>>> the virtual-APIC page, but the manual seems clear to me.
>> 
>> Currently, no existing hardware will use TMR and will not cause any
>> problem.(That's the reason why we leave it in Xen).But we don't know
>> whether future hardware will use it or not(SDM always keeps changing
>> :)).
> 
> But that would be covered by a different execution control (for
> backwards compatibility).  We'll get there when such a feature is introduced.

Yes, we can leave it in future. But one concern is that it may hard to handle it at that time if someone also develops feature which rely on it (like current patch to split irqchip). 

> 
>> And per 24.11.4's description, the perfect solution is don't modify
>> it. btw, IIRC, only TMR doesn't follow the rule. All other VMCS
>> accesses are issued in right VMCS context.
> 
> Yes, that's correct.  It's just the TMR.
> 
> Paolo


Best regards,
Yang


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199462

On 04/08/2015 02:46, Zhang, Yang Z wrote:
> > It is a problem for split irqchip, where the EOI exit bitmap can be
> > inferred from the IOAPIC routes but the TMR cannot.  The hardware
> > behavior on the other hand can be implemented purely within the LAPIC.
> 
> So updating the TMR within LAPIC is the only solution to handle it?

It's the simplest and the one that makes most sense.  Considering that
TMR is a pretty obscure feature, it's unlikely that it will be
accelerated in the future.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1199478

Paolo Bonzini wrote on 2015-08-04:
> 
> 
> On 04/08/2015 02:46, Zhang, Yang Z wrote:
>>> It is a problem for split irqchip, where the EOI exit bitmap can be
>>> inferred from the IOAPIC routes but the TMR cannot.  The hardware
>>> behavior on the other hand can be implemented purely within the LAPIC.
>> 
>> So updating the TMR within LAPIC is the only solution to handle it?
> 
> It's the simplest and the one that makes most sense.  Considering that
> TMR is a pretty obscure feature, it's unlikely that it will be
> accelerated in the future.

You may be right. It is safe if no future hardware plans to use it. Let me check with our hardware team to see whether it will be used or not in future. 

> 
> Paolo


Best regards,
Yang


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1198502

Paolo Bonzini wrote on 2015-07-31:
> 
> 
> On 31/07/2015 04:49, Steve Rutherford wrote:
>> Oh... Yeah. That's a damn good point, given that the interrupt can be
>> injected from another thread while one is in that guest vcpu.
>> 
>> Easiest time to update the TMR should be on guest entry through
>> vcpu_scan_ioapic, as before.
>> 
>> The best way to go is probably to ditch the new per vcpu EOI exit
>> bitmap, and just update/use the TMR. There's no reason to duplicate
>> that data in the representation of the apic (I suspect that the
>> duplication was inspired by my mistaken notion of the TMR). The
>> IOAPIC exit check can use the TMR instead.
>> 
>> Based upon my reading of the SDM, the only reason that the eoi exit
>> bitmaps are not the exact same as the TMR is that it is possible to
>> have virtual-interrupt delivery enabled /without/ an apic access page
>> (Note: V-ID => EOI exit bitmap enabled).
>> 
>> Yang, do you happen to know if that is the case?
>> 
>> [Note: Just looked back at the old code for updating the EOI exit
>> bitmaps, which for some reason was configured to trigger EOI exits
>> for all IOAPIC interrupts, not just level-triggered IOAPIC
>> interrupts. Which is weird, and I believe totally unecessary.]
> 
> The RTC interrupt needs to trigger an EOI exit with the in-kernel
> IOAPIC, in order to detect coalesced interrupts.  This is necessary to
> avoid severe time drift in Windows guest.

Correct! EOI exit bitmap is not absolutely same with TMR. Some interrupts (especially timer interrupt) which is edge trigger but need a notification when it got injected into guest. 

> 
> Paolo


Best regards,
Yang


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web