Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <eaa50a4f-d0af-0aa6-8431-f83e772545f1@semihalf.com>
Date:   Fri, 21 Oct 2022 15:31:32 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.0
Subject: Re: [PATCH v3 0/2] KVM: x86/ioapic: Fix oneshot interrupts forwarding
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, linux-kernel@vger.kernel.org,
        Marc Zyngier <maz@kernel.org>,
        Eric Auger <eric.auger@redhat.com>,
        Alex Williamson <alex.williamson@redhat.com>,
        Rong L Liu <rong.l.liu@intel.com>,
        Zhenyu Wang <zhenyuw@linux.intel.com>,
        Tomasz Nowicki <tn@semihalf.com>,
        Grzegorz Jaszczyk <jaz@semihalf.com>, upstream@semihalf.com,
        Dmitry Torokhov <dtor@google.com>,
        "Dong, Eddie" <eddie.dong@intel.com>
References: <20220818202701.3314045-1-dmy@semihalf.com>
From:   Dmytro Maluka <dmy@semihalf.com>
In-Reply-To: <20220818202701.3314045-1-dmy@semihalf.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Paolo, Marc, Eric and others,

Did you have a chance to take a look at this v3 patchset?

Just to remind the context: this patchset implements a solution I
proposed in [1], only that it doesn't introduce the additional "pending
oneshot" state (which was Marc's concern in [2]): I realized that this
extra state is not really needed, as we can just unconditionally notify
the resamplefd once again at unmask, with the same end result.

[1] https://lore.kernel.org/kvm/72e40c17-e5cd-1ffd-9a38-00b47e1cbd8e@semihalf.com/
[2] https://lore.kernel.org/kvm/87o7wrug0w.wl-maz@kernel.org/

On 8/18/22 22:26, Dmytro Maluka wrote:
> KVM irqfd based emulation of level-triggered interrupts doesn't work
> quite correctly in some cases, particularly in the case of interrupts
> that are handled in a Linux guest as oneshot interrupts (IRQF_ONESHOT).
> Such an interrupt is acked to the device in its threaded irq handler,
> i.e. later than it is acked to the interrupt controller (EOI at the end
> of hardirq), not earlier.
> 
> Linux keeps such interrupt masked until its threaded handler finishes,
> to prevent the EOI from re-asserting an unacknowledged interrupt.
> However, with KVM + vfio (or whatever is listening on the resamplefd)
> we always notify resamplefd at the EOI, so vfio prematurely unmasks the
> host physical IRQ, thus a new physical interrupt is fired in the host.
> This extra interrupt in the host is not a problem per se. The problem is
> that it is unconditionally queued for injection into the guest, so the
> guest sees an extra bogus interrupt. [*]
> 
> There are observed at least 2 user-visible issues caused by those
> extra erroneous interrupts for a oneshot irq in the guest:
> 
> 1. System suspend aborted due to a pending wakeup interrupt from
>    ChromeOS EC (drivers/platform/chrome/cros_ec.c).
> 2. Annoying "invalid report id data" errors from ELAN0000 touchpad
>    (drivers/input/mouse/elan_i2c_core.c), flooding the guest dmesg
>    every time the touchpad is touched.
> 
> The core issue here is that by the time when the guest unmasks the IRQ,
> the physical IRQ line is no longer asserted (since the guest has
> acked the interrupt to the device in the meantime), yet we
> unconditionally inject the interrupt queued into the guest by the
> previous resampling. So to fix the issue, we need a way to detect that
> the IRQ is no longer pending, and cancel the queued interrupt in this
> case.
> 
> With IOAPIC we are not able to probe the physical IRQ line state
> directly (at least not if the underlying physical interrupt controller
> is an IOAPIC too), so in this patch series we use irqfd resampler for
> that. Namely, instead of injecting the queued interrupt, we just notify
> the resampler that this interrupt is done. If the IRQ line is actually
> already deasserted, we are done. If it is still asserted, a new
> interrupt will be shortly triggered through irqfd and injected into the
> guest.
> 
> In the case if there is no irqfd resampler registered for this IRQ, we
> cannot fix the issue, so we keep the existing behavior: immediately
> unconditionally inject the queued interrupt.
> 
> This patch series fixes the issue for x86 IOAPIC only. In the long run,
> we can fix it for other irqchips and other architectures too, possibly
> taking advantage of reading the physical state of the IRQ line, which is
> possible with some other irqchips (e.g. with arm64 GIC, maybe even with
> the legacy x86 PIC).
> 
> [*] In this description we assume that the interrupt is a physical host
>     interrupt forwarded to the guest e.g. by vfio. Potentially the same
>     issue may occur also with a purely virtual interrupt from an
>     emulated device, e.g. if the guest handles this interrupt, again, as
>     a oneshot interrupt.
> 
> 
> v3:
>   - Completely reworked: instead of postponing resamplefd notify until
>     unmask (to avoid extra interrupts in the host), resample the pending
>     status at unmask to avoid erroneous propagation of those extra
>     interrupts to the guest.
>     Thanks to Marc Zyngier for helping to identify the core issue, which
>     resulted in a simpler and probably more sensible implementation
>     (even though Marc's concern about presenting inaccurate pending
>     status to the guest is a non-issue in the case of IOAPIC, since
>     IOAPIC doesn't present this information anyway).
> 
> v2:
>   - Fixed compilation failure on non-x86: mask_notifier_list moved from
>     x86 "struct kvm_arch" to generic "struct kvm".
>   - kvm_fire_mask_notifiers() also moved from x86 to generic code, even
>     though it is not called on other architectures for now.
>   - Instead of kvm_irq_is_masked() implemented
>     kvm_register_and_fire_irq_mask_notifier() to fix potential race
>     when reading the initial IRQ mask state.
>   - Renamed for clarity:
>       - irqfd_resampler_mask() -> irqfd_resampler_mask_notify()
>       - kvm_irq_has_notifier() -> kvm_irq_has_ack_notifier()
>       - resampler->notifier -> resampler->ack_notifier
>   - Reorganized code in irqfd_resampler_ack() and
>     irqfd_resampler_mask_notify() to make it easier to follow.
>   - Don't follow unwanted "return type on separate line" style for
>     irqfd_resampler_mask_notify().
> 
> Dmytro Maluka (2):
>   KVM: irqfd: Make resampler_list an RCU list
>   KVM: x86/ioapic: Resample the pending state of an IRQ when unmasking
> 
>  arch/x86/kvm/ioapic.c     | 36 ++++++++++++++++++++++++++++--
>  include/linux/kvm_host.h  |  9 ++++++++
>  include/linux/kvm_irqfd.h |  2 +-
>  virt/kvm/eventfd.c        | 47 ++++++++++++++++++++++++++++++++-------
>  4 files changed, 83 insertions(+), 11 deletions(-)
>