[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <87v7n6gcrm.ffs@tglx>
Date: Sat, 02 Aug 2025 13:54:05 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Hogan Wang <hogan.wang@...wei.com>, x86@...nel.org,
dave.hansen@...ux.intel.com, kvm@...r.kernel.org,
alex.williamson@...hat.com
Cc: weidong.huang@...wei.com, yechuan@...wei.com, hogan.wang@...wei.com,
wangxinxin.wang@...wei.com, jianjay.zhou@...wei.com, wangjie88@...wei.com,
maz@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86/irq: Plug vector setup race
On Fri, Aug 01 2025 at 22:56, Hogan Wang wrote:
> I believe an effective solution to the issue of lost interrupts
> might be to modify the vifo module to avoid un-plug/plug irq,
> and instead use a more lightweight method to switch interrupt
> modes. Just like:
>
> vfio_irq_handler()
> if kvm_mode
> vfio_send_eventfd(kvm_irq_fd);
> else
> vfio_send_eventfd(qemu_irq_fd);
>
> However, this will bring about some troubles:
> 1) The kvm_mode variable should be protected, leading to performance loss.
> 2) The VFIO interface requires the passing of two eventfds.
> 3) Add another interface to implement mode switching.
>
> Do you have a better solution to fix this interrupt loss issue?
Interesting. I looked at vfio_irq_handler(), which is in the platform/
part of VFIO. The corresponding vfio_set_trigger(), which switches eventfds
does the right thing:
disable_irq();
update(trigger);
enable_irq();
disable_irq() ensures that there is no interrupt handler in progress, so
it becomes safe to switch the trigger in the data structure which is has
been handed to request_irq() as @dev_id argument. For edge type
interupts this ensures that a interrupt which arrives while disabled is
retriggered on enable, so that no interrupt can get lost.
The PCI variant is using the trigger itself as the @dev_id argument and
therefore has to do the free_irq()/request_irq() dance. It shouldn't be
hard to convert the PCI implementation over to the disable/enable scheme.
> There is a question that has been troubling me: Why are interrupts
> still reported after they have been masked and the interrupt remapping
> table entries have been disabled? Is this interrupt cached somewhere?
Let me bring back the picture I used before:
CPU0 CPU1
vmenter(vCPU0)
.... local_irq_disable()
msi_set_affinity()
#1 mask(MSI-X)
vmexit()
#2 ... interrupt is raised in APIC
but not handled
#3 really_mask(MSI-X)
free_irq()
mask();
#4 __synchronize_irq()
msi_domain_deactivate()
write_msg(0);
x86_vector_deactivate()
#5 per_cpu(vector_irq, cpu)[vector] = VECTOR_SHUTDOWN;
#6 local_irq_enable()
interrupt is handled and
observes VECTOR_SHUTDOWN
writes VECTOR_UNUSED
request_irq()
x86_vector_activate()
per_cpu(vector_irq, cpu)[vector] = desc;
msi_domain_deactivate()
write_msg(msg);
unmask();
#1 is the mask operation in the VM, which is trapped, i.e. the interrupt
is not yet masked at the MSIX level.
#2 The device raises the interupt _before_ the host can mask the
interrupt at the PCI-MSIX level (#3).
The interrupt is sent to the APIC of the target CPU 1, which sets the
corresponding IRR bit in the APIC if the CPU cannot handle it at that
point, because it has interrupts disabled.
#4 cannot observe the pending IRR bit on CPU1's APIC and therefore
concludes that there is no interrupt in flight.
If the host side VMM manages to shut down the interrupt completely (#5)
_before_ CPU1 reenables interrupts (#6), then CPU1 will observe
VECTOR_SHUTDOWN and treats it as a spurious interrupt.
The same problem exists on bare metal, when a driver leaves the device
interrupts enabled and then does a free/request dance:
CPU0 CPU1
.... local_irq_disable()
#1 free_irq()
#2 ... interrupt is raised in APIC
but not handled
#3 really_mask(MSI-X)
#4 __synchronize_irq()
msi_domain_deactivate()
write_msg(0);
x86_vector_deactivate()
#5 per_cpu(vector_irq, cpu)[vector] = VECTOR_SHUTDOWN;
#6 local_irq_enable()
interrupt is handled and
observes VECTOR_SHUTDOWN
writes VECTOR_UNUSED
request_irq()
x86_vector_activate()
per_cpu(vector_irq, cpu)[vector] = desc;
msi_domain_deactivate()
write_msg(msg);
unmask();
See?
Thanks,
tglx
Powered by blists - more mailing lists