Messages in this thread | | | Subject | Re: [PATCH v2] kvm: ioapic: Introduce arch-specific check for lazy update EOI mechanism | From | Suravee Suthikulpanit <> | Date | Mon, 4 May 2020 13:31:02 +0700 |
| |
Paolo,
On 5/3/20 12:19 AM, Paolo Bonzini wrote: > On 02/05/20 11:24, Suravee Suthikulpanit wrote: > .... > The questions to answer are: what is causing the re-entrancy? and why > is dropping the second EOI update safe? > > The answer to the latter could well be "because we've already processed > it", but the answer to the former is more important. > > The re-entrancy happens because the irq state is the OR of > the interrupt state and the resamplefd state. That is, we don't > want to show the state as 0 until we've had a chance to set the > resamplefd. But if the interrupt has _not_ gone low then we get an > infinite loop.
I'm not too familiar w/ the resamplefd. I must have missed this part. Could you please point out to me where the OR logic is?
> So the actual root cause is that this is a level-triggered interrupt, > otherwise irqfd_inject would immediately set the KVM_USERSPACE_IRQ_SOURCE_ID > high and then low and you wouldn't have the infinite loop.
Okay.
> But in the case of level-triggered interrupts the VMEXIT already happens because > TMR is set; only edge-triggered interrupts need the lazy invocation > of the ack notifier.
For AVIC, EOI write for level-triggered would have also be trapped. And yes, edge-triggered needs lazy ack notifier.
> So this should be the fix: > > diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c > index 7668fed1ce65..ca2d73cd00a3 100644 > --- a/arch/x86/kvm/ioapic.c > +++ b/arch/x86/kvm/ioapic.c > @@ -225,12 +225,12 @@ static int ioapic_set_irq(struct kvm_ioapic *ioapic, unsigned int irq, > } > > /* > - * AMD SVM AVIC accelerate EOI write and do not trap, > - * in-kernel IOAPIC will not be able to receive the EOI. > + * AMD SVM AVIC accelerate EOI write iff the interrupt is level > + * triggered, in-kernel IOAPIC will not be able to receive the EOI.
Actually, it should be "AMD SVM AVIC accelerate EOI write iff the interrupt is _edge_ triggered".
> * In this case, we do lazy update of the pending EOI when > * trying to set IOAPIC irq. > */ > - if (kvm_apicv_activated(ioapic->kvm)) > + if (edge && kvm_apicv_activated(ioapic->kvm)) > ioapic_lazy_update_eoi(ioapic, irq); > > /* > > Did I miss anything in the above analysis with respect to AVIC?
For AMD: Tested-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Thanks, Suravee
| |