Re: [Xen-devel] [PATCH for-4.13 v3] x86/passthrough: fix migration of MSI when using posted interrupts

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Nov 15, 2019 at 05:23:51AM +0000, Tian, Kevin wrote:
> > From: Roger Pau Monne [mailto:roger.pau@xxxxxxxxxx]
> > Sent: Friday, November 8, 2019 9:34 PM
> > 
> > When using posted interrupts and the guest migrates MSI from vCPUs Xen
> > needs to flush any pending PIRR vectors on the previous vCPU, or else
> > those vectors could get wrongly injected at a later point when the MSI
> > fields are already updated.
> 
> I may overlook but isn't it the guest's responsibility of handling such
> case? Even on bare metal, an in-fly interrupt may be delivered to
> wrong CPU when MSI is being migrated?

According to Joe from Oracle Linux already takes care of that by
checking IRR when migrating interrupts between CPUs, but it seems like
the vector is not pending in IRR (my hypothesis is that it's pending
in PIR but lacking a sync into IRR).

After digging more into the posted interrupt code, I think there's an
issue somewhere else, and the sync on MSI reconfiguration done by this
patch is just covering that up.

There shouldn't be any interrupts pending in the PIR when the vCPU is
running, and any pending vectors in the PIR should be synced into IRR
before executing the vCPU.

AFAICT there's an issue with how PIR is synced into IRR, it relies on
vlapic_find_highest_irr being called from vlapic_has_pending_irq, but
depending on which interrupts are pending it's possible that
vlapic_has_pending_irq is not called by hvm_vcpu_has_pending_irq, thus
leaving IRR stale.

The patch below should solve that and also simplify
__vmx_deliver_posted_interrupt, there's no reason to raise a softirq
in __vmx_deliver_posted_interrupt: if the vCPU is the one currently
running or if it's not running at all the sync of PIR to IRR will
happen on vmentry, without the need of any softirq being set. Also
note the raise_softirq in __vmx_deliver_posted_interrupt should have
been a cpu_raise_softirq(cpu, VCPU_KICK_SOFTIRQ) instead.

Joe, can you give a try to the patch below?

Thanks, Roger.
---8<---
commit 9ab79fcbc4dece15551fba177b59b51631101563
Author: Roger Pau Monne <roger.pau@xxxxxxxxxx>
Date:   Fri Nov 15 11:58:18 2019 +0100

diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
index 0d097cf1f2..ce70f4bc75 100644
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -232,6 +232,14 @@ void vmx_intr_assist(void)
     enum hvm_intblk intblk;
     int pt_vector;
 
+    if ( cpu_has_vmx_posted_intr_processing )
+        /*
+         * Always force PIR to be synced to IRR before vmentry, this is also
+         * done by vlapic_has_pending_irq but it's possible other pending
+         * interrupts prevent the execution of that function.
+         */
+        vmx_sync_pir_to_irr(v);
+
     /* Block event injection when single step with MTF. */
     if ( unlikely(v->arch.hvm.single_step) )
     {
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 33e68eaddf..82a1b972c5 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1945,8 +1945,6 @@ static void vmx_process_isr(int isr, struct vcpu *v)
 
 static void __vmx_deliver_posted_interrupt(struct vcpu *v)
 {
-    bool_t running = v->is_running;
-
     vcpu_unblock(v);
     /*
      * Just like vcpu_kick(), nothing is needed for the following two cases:
@@ -1954,48 +1952,28 @@ static void __vmx_deliver_posted_interrupt(struct vcpu 
*v)
      * 2. The target vCPU is the current vCPU and we're in non-interrupt
      * context.
      */
-    if ( running && (in_irq() || (v != current)) )
-    {
+    if ( vcpu_runnable(v) && v != current )
         /*
-         * Note: Only two cases will reach here:
-         * 1. The target vCPU is running on other pCPU.
-         * 2. The target vCPU is the current vCPU.
+         * If the vCPU is running on another pCPU send an IPI to the pCPU. When
+         * the IPI arrives, the target vCPU may be running in non-root mode,
+         * running in root mode, runnable or blocked. If the target vCPU is
+         * running in non-root mode, the hardware will sync PIR to vIRR for
+         * 'posted_intr_vector' is a special vector handled directly by the
+         * hardware.
          *
-         * Note2: Don't worry the v->processor may change. The vCPU being
-         * moved to another processor is guaranteed to sync PIR to vIRR,
-         * due to the involved scheduling cycle.
+         * If the target vCPU is running in root-mode, the interrupt handler
+         * starts to run.  Considering an IPI may arrive in the window between
+         * the call to vmx_intr_assist() and interrupts getting disabled, the
+         * interrupt handler should raise a softirq to ensure events will be
+         * delivered in time.
          */
-        unsigned int cpu = v->processor;
+        send_IPI_mask(cpumask_of(v->processor), posted_intr_vector);
 
-        /*
-         * For case 1, we send an IPI to the pCPU. When an IPI arrives, the
-         * target vCPU maybe is running in non-root mode, running in root
-         * mode, runnable or blocked. If the target vCPU is running in
-         * non-root mode, the hardware will sync PIR to vIRR for
-         * 'posted_intr_vector' is special to the pCPU. If the target vCPU is
-         * running in root-mode, the interrupt handler starts to run.
-         * Considering an IPI may arrive in the window between the call to
-         * vmx_intr_assist() and interrupts getting disabled, the interrupt
-         * handler should raise a softirq to ensure events will be delivered
-         * in time. If the target vCPU is runnable, it will sync PIR to
-         * vIRR next time it is chose to run. In this case, a IPI and a
-         * softirq is sent to a wrong vCPU which will not have any adverse
-         * effect. If the target vCPU is blocked, since vcpu_block() checks
-         * whether there is an event to be delivered through
-         * local_events_need_delivery() just after blocking, the vCPU must
-         * have synced PIR to vIRR. Similarly, there is a IPI and a softirq
-         * sent to a wrong vCPU.
-         */
-        if ( cpu != smp_processor_id() )
-            send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
-        /*
-         * For case 2, raising a softirq ensures PIR will be synced to vIRR.
-         * As any softirq will do, as an optimization we only raise one if
-         * none is pending already.
-         */
-        else if ( !softirq_pending(cpu) )
-            raise_softirq(VCPU_KICK_SOFTIRQ);
-    }
+    /*
+     * If the vCPU is not runnable or if it's the one currently running in this
+     * pCPU there's nothing to do, the vmentry code will already sync the PIR
+     * to IRR when resuming execution.
+     */
 }
 
 static void vmx_deliver_posted_intr(struct vcpu *v, u8 vector)
@@ -2048,7 +2026,7 @@ static void vmx_deliver_posted_intr(struct vcpu *v, u8 
vector)
     vcpu_kick(v);
 }
 
-static void vmx_sync_pir_to_irr(struct vcpu *v)
+void vmx_sync_pir_to_irr(struct vcpu *v)
 {
     struct vlapic *vlapic = vcpu_vlapic(v);
     unsigned int group, i;
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h 
b/xen/include/asm-x86/hvm/vmx/vmx.h
index ebaa74449b..c43fab7c4f 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -101,6 +101,7 @@ void vmx_update_debug_state(struct vcpu *v);
 void vmx_update_exception_bitmap(struct vcpu *v);
 void vmx_update_cpu_exec_control(struct vcpu *v);
 void vmx_update_secondary_exec_control(struct vcpu *v);
+void vmx_sync_pir_to_irr(struct vcpu *v);
 
 #define POSTED_INTR_ON  0
 #define POSTED_INTR_SN  1


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel