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

[Steve Rutherford <srutherford@google.com>]

On Thu, Jul 30, 2015 at 11:26:28PM +0000, Zhang, Yang Z wrote:
> Paolo Bonzini wrote on 2015-07-29:
> > 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.
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.]


> 
> 
> > 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)
> 
> 
> Best regards,
> Yang
> 
> 
> --
> 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