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