Message-ID: <9ee7d5e3-2273-4aa7-6859-a5f82c82c34c@semihalf.com>
Date:   Thu, 16 Mar 2023 22:13:28 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v3 2/2] KVM: x86/ioapic: Resample the pending state of an
 IRQ when unmasking
To:     Sean Christopherson <seanjc@google.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>
References: <20220818202701.3314045-1-dmy@semihalf.com>
 <20220818202701.3314045-3-dmy@semihalf.com> <ZBJfuOOioFb0pVB6@google.com>
Content-Language: en-US
From:   Dmytro Maluka <dmy@semihalf.com>
In-Reply-To: <ZBJfuOOioFb0pVB6@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Sean,

On 3/16/23 01:16, Sean Christopherson wrote:
> 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.

Ok, sure. All of your cosmetic suggestions below sound good to me.

> 
> 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
>>