Date:   Wed, 15 Mar 2023 17:16:29 -0700
In-Reply-To: <20220818202701.3314045-3-dmy@semihalf.com>
Mime-Version: 1.0
References: <20220818202701.3314045-1-dmy@semihalf.com> <20220818202701.3314045-3-dmy@semihalf.com>
Message-ID: <ZBJfuOOioFb0pVB6@google.com>
Subject: Re: [PATCH v3 2/2] KVM: x86/ioapic: Resample the pending state of an
 IRQ when unmasking
From:   Sean Christopherson <seanjc@google.com>
To:     Dmytro Maluka <dmy@semihalf.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org,
        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>,
        Eddie Dong <eddie.dong@intel.com>
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

Looks sane to me, just a bunch of cosmetic comments.  But this really needs input/review
from others.  I/O APIC and level triggered interrupts are not exactly in my wheelhouse.

On Thu, Aug 18, 2022, Dmytro Maluka wrote:
> ---
>  arch/x86/kvm/ioapic.c    | 36 ++++++++++++++++++++++++++++++++++--
>  include/linux/kvm_host.h |  8 ++++++++
>  virt/kvm/eventfd.c       | 39 +++++++++++++++++++++++++++++++++------
>  3 files changed, 75 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
> index 765943d7cfa5..da7074d9b04e 100644
> --- a/arch/x86/kvm/ioapic.c
> +++ b/arch/x86/kvm/ioapic.c
> @@ -368,8 +368,40 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
>  		if (mask_before != mask_after)
>  			kvm_fire_mask_notifiers(ioapic->kvm, KVM_IRQCHIP_IOAPIC, index, mask_after);
>  		if (e->fields.trig_mode == IOAPIC_LEVEL_TRIG
> -		    && ioapic->irr & (1 << index))
> -			ioapic_service(ioapic, index, false);
> +		    && ioapic->irr & (1 << index)
> +		    && !e->fields.mask
> +		    && !e->fields.remote_irr) {

Can you opportunistically change these to fit the preferred style of putting the &&
on the previous line?  Ignore the file's existing "style", this crud is ancient and
ugly (this goes for all of my comments).

> @@ -1987,6 +1988,13 @@ static inline int kvm_irqfd(struct kvm *kvm, struct kvm_irqfd *args)
>  }
>  
>  static inline void kvm_irqfd_release(struct kvm *kvm) {}
> +
> +static inline bool kvm_notify_irqfd_resampler(struct kvm *kvm,
> +					      unsigned irqchip,
> +					      unsigned pin)

"unsigned int" instead of bare "unsigned"

> +{
> +	return false;
> +}
>  #endif
>  
>  #else
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index 61aea70dd888..71f327019f1e 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -55,6 +55,16 @@ irqfd_inject(struct work_struct *work)
>  			    irqfd->gsi, 1, false);
>  }
>  
> +/* Called within kvm->irq_srcu read side. */

Ne need for the comment, let lockdep do the heavy lifting.

> +static void __irqfd_resampler_notify(struct kvm_kernel_irqfd_resampler *resampler)

I don't see a need for the double underscores.  I assume the idea is to convey
that this is called under kvm->irq_srcu, but I just ended up looking for a version
without the underscores.

> +{
> +	struct kvm_kernel_irqfd *irqfd;
> +
> +	list_for_each_entry_srcu(irqfd, &resampler->list, resampler_link,
> +	    srcu_read_lock_held(&resampler->kvm->irq_srcu))

Align the indentation, i.e.

	struct kvm_kernel_irqfd *irqfd;

	list_for_each_entry_srcu(irqfd, &resampler->list, resampler_link,
				 srcu_read_lock_held(&resampler->kvm->irq_srcu))
		eventfd_signal(irqfd->resamplefd, 1);

> @@ -648,6 +653,28 @@ void kvm_irq_routing_update(struct kvm *kvm)
>  	spin_unlock_irq(&kvm->irqfds.lock);
>  }
>  
> +bool kvm_notify_irqfd_resampler(struct kvm *kvm, unsigned irqchip, unsigned pin)
> +{
> +	struct kvm_kernel_irqfd_resampler *resampler;
> +	int gsi, idx;
> +
> +	idx = srcu_read_lock(&kvm->irq_srcu);
> +	gsi = kvm_irq_map_chip_pin(kvm, irqchip, pin);
> +	if (gsi != -1)

This if-statement needs curly braces, the exemption doesn't apply if there are
multiple blocks? (can't think of the right name at the moment) in the guts of
the if-statement.

> +		list_for_each_entry_srcu(resampler,
> +					 &kvm->irqfds.resampler_list, link,
> +					 srcu_read_lock_held(&kvm->irq_srcu)) {
> +			if (resampler->notifier.gsi == gsi) {
> +				__irqfd_resampler_notify(resampler);
> +				srcu_read_unlock(&kvm->irq_srcu, idx);
> +				return true;
> +			}
> +		}
> +	srcu_read_unlock(&kvm->irq_srcu, idx);
> +
> +	return false;
> +}
> +
>  /*
>   * create a host-wide workqueue for issuing deferred shutdown requests
>   * aggregated from all vm* instances. We need our own isolated
> -- 
> 2.37.1.595.g718a3a8f04-goog
>