RE: [PATCH v3 3/4] KVM: x86: Add lowest-priority support for vt-d posted-interrupts

[Wu, Feng]

> -----Original Message-----
> From: Radim Krčmář [mailto:rkrcmar@xxxxxxxxxx]
> Sent: Friday, January 22, 2016 4:17 AM
> To: Wu, Feng <feng.wu@xxxxxxxxx>
> Cc: pbonzini@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> kvm@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 3/4] KVM: x86: Add lowest-priority support for vt-d
> posted-interrupts
> 
> 2016-01-20 09:42+0800, Feng Wu:
> > Use vector-hashing to deliver lowest-priority interrupts for
> > VT-d posted-interrupts. This patch extends kvm_intr_is_single_vcpu()
> > to support lowest-priority handling.
> >
> > Signed-off-by: Feng Wu <feng.wu@xxxxxxxxx>
> > ---
> > v3:
> > - Remove unnecessary check in fast irq delivery patch
> > - print a error message only once for each guest when we find hardware
> >   disabled LAPIC during interrupt injection.
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h
> b/arch/x86/include/asm/kvm_host.h
> > @@ -1316,8 +1316,8 @@ int x86_set_memory_region(struct kvm *kvm, int
> id, gpa_t gpa, u32 size);
> >  bool kvm_vcpu_is_reset_bsp(struct kvm_vcpu *vcpu);
> >  bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu);
> >
> > -bool kvm_intr_is_single_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
> > -			     struct kvm_vcpu **dest_vcpu);
> > +bool kvm_intr_can_posting(struct kvm *kvm, struct kvm_lapic_irq *irq,
> 
> I prefer the original one;  I think it's better to describe function
> than intent in names -- intention should be obvious from its use.
> 
> > +			  struct kvm_vcpu **dest_vcpu);
> >
> >  void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e,
> >  		     struct kvm_lapic_irq *irq);
> > diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> > @@ -300,13 +300,13 @@ out:
> >  	return r;
> >  }
> >
> > -bool kvm_intr_is_single_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
> > -			     struct kvm_vcpu **dest_vcpu)
> > +bool kvm_intr_can_posting(struct kvm *kvm, struct kvm_lapic_irq *irq,
> > +		struct kvm_vcpu **dest_vcpu)
> >  {
> >  	int i, r = 0;
> >  	struct kvm_vcpu *vcpu;
> >
> > -	if (kvm_intr_is_single_vcpu_fast(kvm, irq, dest_vcpu))
> > +	if (kvm_intr_can_posting_fast(kvm, irq, dest_vcpu))
> >  		return true;
> 
> There is one pitfall:  xAPIC flat logical broadcast returns false,

Do you mean kvm_intr_can_posting_fast() returns false for
xAPIC flat logical lowest-priority broadcast?

After carefully read the code for several times, I still cannot
find the reason, could you please give more hints?

BTW, I noticed there is  a "if(irq->dest_id == 0xFF) goto out;" in
this function, but it is for the physical dest mode. I am not
sure you mean this.

> but lowest priority is defined for it (practically isn't a broadcast) and
> the rest of this function doesn't check for lowest priority, so the
> interrupt won't be posted.
> 
> We could modify our _fast functions to cover 0xff in flat logical, but
> ignoring this case isn't bad either ... it can happen only with 8 VCPU
> guests. 

Could you please elaborate a bit more why only for the 8 VCPU guests?
Thanks a lot!

Thanks,
Feng