[PATCH 2/6] x86/vlapic: Fix hardware virtualized APIC state after reset

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

There are various issues in the current vlapic logic handling reset
 - SVI and RVI aren't reset, and GUEST_INTR_STATUS is not updated
   appropriately
 - stale PIR bits are merged into fresh IRR

Fixes: d7dafa375bc1 ("VMX: Add posted interrupt supporting")
Signed-off-by: Teddy Astie <teddy.astie@xxxxxxxxxx>
Assisted-by: Claude Fable 5.1
---
While the change looks fine to me, I feel some of the comments are
too Intel-centric, and not sure how them apply to AMD AVIC.
---
 xen/arch/x86/hvm/vlapic.c  | 43 ++++++++++++++++++++++++++++++++++----
 xen/arch/x86/hvm/vmx/vmx.c | 30 ++++++++++++++++++++------
 2 files changed, 63 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 885f5d304b..53f8907640 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -68,6 +68,7 @@ static const unsigned int vlapic_lvt_mask[VLAPIC_LVT_NUM] =
      == APIC_TIMER_MODE_TSC_DEADLINE)
 
 static void vlapic_do_init(struct vlapic *vlapic);
+static void vlapic_reset_hw_state(struct vlapic *vlapic);
 
 static int vlapic_find_highest_vector(const void *bitmap)
 {
@@ -277,6 +278,7 @@ static void vlapic_init_sipi_one(struct vcpu *target, 
uint32_t icr)
     {
     case APIC_DM_INIT: {
         int rc;
+        struct vlapic *vlapic = vcpu_vlapic(target);
 
         /* No work on INIT de-assert for P4-type APIC. */
         if ( (icr & (APIC_INT_LEVELTRIG | APIC_INT_ASSERT)) ==
@@ -290,7 +292,8 @@ static void vlapic_init_sipi_one(struct vcpu *target, 
uint32_t icr)
         /* Reset necessary VCPU state. This does not include FPU state. */
         rc = vcpu_reset(target);
         ASSERT(!rc);
-        vlapic_do_init(vcpu_vlapic(target));
+        vlapic_do_init(vlapic);
+        vlapic_reset_hw_state(vlapic);
         domain_unlock(target->domain);
         break;
     }
@@ -1429,6 +1432,13 @@ static void vlapic_do_init(struct vlapic *vlapic)
     if ( !has_vlapic(vlapic_vcpu(vlapic)->domain) )
         return;
 
+    /*
+     * Drain the posted interrupt descriptor (and clear its ON bit) before
+     * zeroing the IRR, so that vectors posted before the reset don't get
+     * merged into the fresh IRR by a later sync.
+     */
+    hvm_sync_pir_to_irr(vlapic_vcpu(vlapic));
+
     vlapic_set_reg(vlapic, APIC_LVR, VLAPIC_VERSION);
 
     for ( i = 0; i < 8; i++ )
@@ -1462,8 +1472,26 @@ static void vlapic_do_init(struct vlapic *vlapic)
     destroy_periodic_time(&vlapic->pt);
 }
 
-/* Reset the VLAPIC back to its power-on/reset state. */
-void vlapic_reset(struct vlapic *vlapic)
+/*
+ * Bring hardware accelerated APIC state (on VMX: RVI, SVI and the EOI-exit
+ * bitmap) back in line with the just reset registers.  Hardware derives the
+ * virtual PPR from SVI and delivers virtual interrupts based on RVI without
+ * consulting the IRR, so leaving them stale after a reset would block
+ * subsequent interrupts, or inject a vector which isn't pending at all.
+ */
+static void vlapic_reset_hw_state(struct vlapic *vlapic)
+{
+    struct vcpu *v = vlapic_vcpu(vlapic);
+
+    if ( !has_vlapic(v->domain) )
+        return;
+
+    if ( hvm_funcs.process_isr )
+        alternative_vcall(hvm_funcs.process_isr, -1, v);
+}
+
+/* Reset the VLAPIC registers back to their power-on/reset state. */
+static void vlapic_reset_regs(struct vlapic *vlapic)
 {
     const struct vcpu *v = vlapic_vcpu(vlapic);
 
@@ -1478,6 +1506,13 @@ void vlapic_reset(struct vlapic *vlapic)
     vlapic_do_init(vlapic);
 }
 
+/* Reset the VLAPIC back to its power-on/reset state. */
+void vlapic_reset(struct vlapic *vlapic)
+{
+    vlapic_reset_regs(vlapic);
+    vlapic_reset_hw_state(vlapic);
+}
+
 /* rearm the actimer if needed, after a HVM restore */
 static void lapic_rearm(struct vlapic *s)
 {
@@ -1705,7 +1740,7 @@ int vlapic_init(struct vcpu *v)
 
     clear_page(vlapic->regs);
 
-    vlapic_reset(vlapic);
+    vlapic_reset_regs(vlapic);
 
     tasklet_init(&vlapic->init_sipi.tasklet, vlapic_init_sipi_action, v);
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 31bb094d6f..b655075c17 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2194,10 +2194,10 @@ static void cf_check vmx_update_eoi_exit_bitmap(
         vmx_clear_eoi_exit_bitmap(v, vector);
 }
 
-static u8 set_svi(int isr)
+static uint8_t set_svi(int isr)
 {
     unsigned long status;
-    u8 old;
+    uint8_t old;
 
     if ( isr < 0 )
         isr = 0;
@@ -2216,12 +2216,14 @@ static u8 set_svi(int isr)
 
 static void cf_check vmx_process_isr(int isr, struct vcpu *v)
 {
-    unsigned int i;
+    unsigned int i, rvi = 0;
+    unsigned long status;
     const struct vlapic *vlapic = vcpu_vlapic(v);
 
     vmx_vmcs_enter(v);
 
-    set_svi(isr);
+    /* Rebuild the EOI-exit bitmap from scratch, dropping stale entries. */
+    bitmap_zero(v->arch.hvm.vmx.eoi_exit_bitmap, X86_IDT_VECTORS);
 
     /*
      * Theoretically, only level triggered interrupts can have their
@@ -2237,13 +2239,29 @@ static void cf_check vmx_process_isr(int isr, struct 
vcpu *v)
      * bitmap correctly.
      */
     for ( i = 0x10; i < X86_IDT_VECTORS; ++i )
-        if ( vlapic_test_vector(i, &vlapic->regs->data[APIC_IRR]) ||
-             vlapic_test_vector(i, &vlapic->regs->data[APIC_ISR]) )
+    {
+        if ( vlapic_test_vector(i, &vlapic->regs->data[APIC_IRR]) )
+        {
+            set_bit(i, v->arch.hvm.vmx.eoi_exit_bitmap);
+            rvi = i;
+        }
+        else if ( vlapic_test_vector(i, &vlapic->regs->data[APIC_ISR]) )
             set_bit(i, v->arch.hvm.vmx.eoi_exit_bitmap);
+    }
 
     for ( i = 0; i < ARRAY_SIZE(v->arch.hvm.vmx.eoi_exit_bitmap); ++i )
         __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm.vmx.eoi_exit_bitmap[i]);
 
+    /*
+     * Hardware delivers virtual interrupts based on RVI alone, without
+     * looking at the IRR, and derives the virtual PPR from SVI.  Both must
+     * therefore reflect the register contents: RVI is the highest pending
+     * vector (0 if none), SVI the highest in-service one (0 if none).
+     */
+    status = MASK_INSR(rvi, VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK) |
+             ((isr < 0 ? 0 : isr) << VMX_GUEST_INTR_STATUS_SVI_OFFSET);
+    __vmwrite(GUEST_INTR_STATUS, status);
+
     vmx_vmcs_exit(v);
 }
 
-- 
2.56.0



--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech