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
>
