|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [PATCH 3/6] vmx/apicv: Don't lose posted interrupts while SN=1
When a vCPU is descheduled, vmx_pi_switch_from() sets SN so the IOMMU
doesn't notify a pCPU the vCPU isn't running on. For non-urgent
interrupts the IOMMU then records the vector in the PIR but neither
sets ON nor sends a notification (VT-d spec "Interrupt-Posting
Hardware Operation"). vmx_deliver_posted_intr() mirrored that and
refused to set ON while SN was set.
vmx_sync_pir_to_irr() however uses ON as its "PIR needs scanning"
gate, and hardware only processes the PIR upon receipt of the
notification vector, never at VM entry. Clearing SN in
vmx_pi_switch_to() therefore left such vectors parked in the PIR
until an unrelated posting happened to set ON. A re-posting of
the same vector doesn't help either, as the early
pi_test_and_set_pir() return skips the ON logic altogether.
Set ON when clearing SN if the PIR is non-empty, as KVM does, and
have the software path always set ON: SN has no bearing on it, since
__vmx_deliver_posted_interrupt() already refrains from IPIing a vCPU
that isn't running. vmx_pi_desc_fixup() sets SN around moving
blocked vCPUs between pCPU lists and had the same window, so handle
it there too by waking the vCPU as the existing ON case does.
Only affect systems where iommu=intpost is enabled.
Fixes: 4b759ce163f4 ("vmx: VT-d posted-interrupt core logic handling")
Fixes: 7c00de17562e ("VMX: permanently assign PI hook vmx_pi_switch_to()")"
Assisted-by: Claude Fable 5.1
Signed-off-by: Teddy Astie <teddy.astie@xxxxxxxxxx>
---
xen/arch/x86/hvm/vmx/vmx.c | 93 +++++++++++++++-----------
xen/arch/x86/include/asm/hvm/vmx/vmx.h | 5 ++
2 files changed, 60 insertions(+), 38 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b655075c17..3d77d77534 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -151,6 +151,21 @@ static void vmx_pi_switch_to(struct vcpu *v)
x2apic_enabled ? dest : MASK_INSR(dest, PI_xAPIC_NDST_MASK));
pi_clear_sn(pi_desc);
+
+ /*
+ * Interrupts posted while SN was set (by the IOMMU for non-urgent
+ * interrupts, see VT-d spec 5.2.3) have their vector recorded in the PIR
+ * but neither set ON nor generate a notification. vmx_sync_pir_to_irr()
+ * uses ON as its "PIR needs scanning" indicator, and hardware only
+ * processes the PIR upon receipt of the notification vector, so such
+ * vectors would otherwise stay parked in the PIR until an unrelated
+ * posting sets ON. Set it ourselves if there is anything pending.
+ *
+ * pi_clear_sn() is a locked operation and hence orders the PIR read
+ * after the SN clear.
+ */
+ if ( !pi_is_pir_empty(pi_desc) )
+ pi_set_on(pi_desc);
}
static void vmx_pi_unblock_vcpu(struct vcpu *v)
@@ -263,6 +278,18 @@ void vmx_pi_desc_fixup(unsigned int cpu)
}
pi_clear_sn(&vmx->pi_desc);
+
+ /*
+ * A posting which arrived while SN was set above has left its vector
+ * in the PIR without setting ON or notifying anyone. The vCPU is on
+ * the new cpu's blocking list by now, so wake it up directly as in
+ * the ON case; vmx_pi_unblock_vcpu() will take it off the list.
+ */
+ if ( !pi_is_pir_empty(&vmx->pi_desc) )
+ {
+ pi_set_on(&vmx->pi_desc);
+ vcpu_unblock(container_of(vmx, struct vcpu, arch.hvm.vmx));
+ }
}
spin_unlock_irqrestore(old_lock, flags);
@@ -368,18 +395,17 @@ void vmx_pi_hooks_deassign(struct domain *d)
domain_pause(d);
/*
- * Note that we don't set 'd->arch.hvm.pi_ops.switch_to' to NULL
- * here. If we deassign the hooks while the vCPU is runnable in the
- * runqueue with 'SN' set, all the future notification event will be
- * suppressed since vmx_deliver_posted_intr() also use 'SN' bit
- * as the suppression flag. Preserving the 'switch_to' hook function can
- * clear the 'SN' bit when the vCPU becomes running next time. After
- * that, No matter which status(runnable, running or block) the vCPU is in,
- * the 'SN' bit will keep clear for the 'switch_from' hook function that
set
- * the 'SN' bit has been removed. At that time, the 'switch_to' hook
function
- * is also useless. Considering the function doesn't do harm to the whole
- * system, leave it here until we find a clean solution to deassign the
- * 'switch_to' hook function.
+ * Note that we keep the 'switch_to' hook. Any vCPU which isn't blocked
+ * has just been descheduled by domain_pause() with the 'switch_from'
+ * hook still in place, and hence has 'SN' set. Interrupts posted by the
+ * IOMMU while 'SN' is set only have their vector recorded in the PIR,
+ * without setting 'ON'. vmx_pi_switch_to() clears 'SN' and sets 'ON'
+ * if the PIR isn't empty, so that such vectors get delivered once the
+ * vCPU runs again. It also keeps 'NDST' up to date, which
+ * vmx_pi_hooks_assign() relies upon should a device get assigned again.
+ *
+ * Once 'SN' is clear it stays clear, as nothing sets it anymore, and the
+ * hook is redundant from then on.
*/
d->arch.hvm.pi_ops.vcpu_block = NULL;
d->arch.hvm.pi_ops.flags = PI_CSW_TO;
@@ -2322,8 +2348,6 @@ static void __vmx_deliver_posted_interrupt(struct vcpu *v)
static void cf_check vmx_deliver_posted_intr(struct vcpu *v, u8 vector)
{
- struct pi_desc old, new, prev;
-
if ( pi_test_and_set_pir(vector, &v->arch.hvm.vmx.pi_desc) )
return;
@@ -2339,30 +2363,23 @@ static void cf_check vmx_deliver_posted_intr(struct
vcpu *v, u8 vector)
return;
}
- prev.control = v->arch.hvm.vmx.pi_desc.control;
-
- do {
- /*
- * Currently, we don't support urgent interrupt, all
- * interrupts are recognized as non-urgent interrupt,
- * Besides that, if 'ON' is already set, no need to
- * send posted-interrupts notification event as well,
- * according to hardware behavior.
- */
- if ( pi_test_sn(&prev) || pi_test_on(&prev) )
- {
- vcpu_kick(v);
- return;
- }
-
- old.control = v->arch.hvm.vmx.pi_desc.control &
- ~((1 << POSTED_INTR_ON) | (1 << POSTED_INTR_SN));
- new.control = v->arch.hvm.vmx.pi_desc.control |
- (1 << POSTED_INTR_ON);
-
- prev.control = cmpxchg(&v->arch.hvm.vmx.pi_desc.control,
- old.control, new.control);
- } while ( prev.control != old.control );
+ /*
+ * Always set ON, irrespective of SN: ON is what vmx_sync_pir_to_irr()
+ * keys off, so leaving it clear while SN is set would park the vector in
+ * the PIR until an unrelated posting sets ON. SN only exists to stop the
+ * IOMMU from notifying a pCPU the vCPU isn't running on; it has no
+ * bearing on the software path, where __vmx_deliver_posted_interrupt()
+ * already refrains from sending an IPI to a vCPU that isn't running.
+ *
+ * If ON was already set, whoever set it has taken care of the
+ * notification (or hardware will pick the vector up when it processes
+ * the pending notification), so a plain kick suffices.
+ */
+ if ( pi_test_and_set_on(&v->arch.hvm.vmx.pi_desc) )
+ {
+ vcpu_kick(v);
+ return;
+ }
__vmx_deliver_posted_interrupt(v);
}
diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
index b7b8bc0c84..7f286f8679 100644
--- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
@@ -94,6 +94,11 @@ static inline int pi_test_pir(uint8_t vector, const struct
pi_desc *pi_desc)
return test_bit(vector, pi_desc->pir);
}
+static inline bool pi_is_pir_empty(const struct pi_desc *pi_desc)
+{
+ return bitmap_empty(pi_desc->pir, X86_IDT_VECTORS);
+}
+
static inline int pi_test_and_set_on(struct pi_desc *pi_desc)
{
return test_and_set_bit(POSTED_INTR_ON, &pi_desc->control);
--
2.56.0
--
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |