linux-kernel - Re: [PATCH v3 2/2] KVM: x86/ioapic: Resample the pending state of an IRQ when unmasking

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 16 Mar 2023 22:13:28 +0100
From:   Dmytro Maluka <dmy@...ihalf.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org,
        Marc Zyngier <maz@...nel.org>,
        Eric Auger <eric.auger@...hat.com>,
        Alex Williamson <alex.williamson@...hat.com>,
        Rong L Liu <rong.l.liu@...el.com>,
        Zhenyu Wang <zhenyuw@...ux.intel.com>,
        Tomasz Nowicki <tn@...ihalf.com>,
        Grzegorz Jaszczyk <jaz@...ihalf.com>, upstream@...ihalf.com,
        Dmitry Torokhov <dtor@...gle.com>,
        Eddie Dong <eddie.dong@...el.com>
Subject: Re: [PATCH v3 2/2] KVM: x86/ioapic: Resample the pending state of an
 IRQ when unmasking

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