Re: [RFC PATCH 7/8] KVM: i8254: Remove need for irq ack notifier

From: Paolo Bonzini <pbonzini@redhat.com>
To: "Suthikulpanit, Suravee" <Suravee.Suthikulpanit@amd.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"x86@kernel.org" <x86@kernel.org>
Cc: "joro@8bytes.org" <joro@8bytes.org>,
	"rkrcmar@redhat.com" <rkrcmar@redhat.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"bp@alien8.de" <bp@alien8.de>, "hpa@zytor.com" <hpa@zytor.com>,
	"jsteckli@amazon.de" <jsteckli@amazon.de>,
	"sironi@amazon.de" <sironi@amazon.de>,
	"wawei@amazon.de" <wawei@amazon.de>
Subject: Re: [RFC PATCH 7/8] KVM: i8254: Remove need for irq ack notifier
Date: Wed, 6 Feb 2019 11:29:15 +0100	[thread overview]
Message-ID: <8ecf791c-d850-25d9-562e-aa2ff5944d2e@redhat.com> (raw)
In-Reply-To: <20190204144128.9489-8-suravee.suthikulpanit@amd.com>

On 04/02/19 15:42, Suthikulpanit, Suravee wrote:
> From: Julian Stecklina <jsteckli@amazon.de>
> 
> ACK notifiers don't work with AMD AVIC when the PIT interrupt is
> delivered as edge-triggered fixed interrupt via the IOAPIC. AMD
> processors cannot exit on EOI for these interrupts. The ACK notifiers do
> work when the interrupt is delivered via PIC as ExtINT, because the ACK
> comes as PIO write that KVM sees.
> 
> Change the PIT code to not rely on the ACK notifiers. The IRQ ACK
> notifier in the PIT emulation re-schedules pit->expired to reinject any
> pending PIT interrupt. This seems useless, because we can pulse the PIT
> interrupt even when the interrupt is not ACKed yet. This means any timer
> expiry when the interrupt was being handled by the guest, will cause an
> interrupt to be injected automatically when the interrupt is ACKed.

The difference is that with the irq ack notifier *all* the pending PIT 
interrupts will be delivered.  Without, only one will.  If you don't 
want the ack notifier, you can do that with the KVM_REINJECT_CONTROL 
ioctl.

Unfortunately, reinject is enabled by default and that is part of the 
userspace ABI and we cannot change it.

It's possible to enable AVIC when reinject is disabled, and we fix that
at the QEMU level by changing the default reinject behavior, basically
with this one-line change:

diff --git a/hw/i386/kvm/i8254.c b/hw/i386/kvm/i8254.c
index d4d4a85..1d82860 100644
--- a/hw/i386/kvm/i8254.c
+++ b/hw/i386/kvm/i8254.c
@@ -305,7 +305,7 @@ static void kvm_pit_realizefn(DeviceState *dev, Error **errp)
 static Property kvm_pit_properties[] = {
     DEFINE_PROP_UINT32("iobase", PITCommonState, iobase,  -1),
     DEFINE_PROP_LOSTTICKPOLICY("lost_tick_policy", KVMPITState,
-                               lost_tick_policy, LOST_TICK_POLICY_DELAY),
+                               lost_tick_policy, LOST_TICK_POLICY_DISCARD),
     DEFINE_PROP_END_OF_LIST(),
 };
 

plus the corresponding change to compat properties in hw/i386/pc.c.

Alternatively, it is probably a good time to switch the default to split irqchip
in QEMU.  Split irqchip was introduced in kernel 4.5, which was released about
three years ago.

Paolo

> Reviewed-by: Filippo Sironi <sironi@amazon.de>
> Signed-off-by: Julian Stecklina <jsteckli@amazon.de>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
