Changeset 78208 in vbox for trunk/src/VBox/VMM

Timestamp:

Apr 18, 2019 3:54:40 PM (6 years ago)

Author:

vboxsync

Message:

PDM,APIC,PIC: Don't use AssertRelease on VMCPU_FF_INTERRUPT_PIC, VMCPU_FF_INTERRUPT_APIC and friends during restore. Instead added debug assertions in the relevant functions causing these to be set by misbehaving device state loaders. ticketref:18331 ticketref:18265

Location:

trunk/src/VBox/VMM

Files:

• VMMAll/APICAll.cpp (modified) (2 diffs)
• VMMR3/APIC.cpp (modified) (1 diff)
• VMMR3/PDM.cpp (modified) (7 diffs)
• VMMR3/PDMDevMiscHlp.cpp (modified) (2 diffs)
• include/PDMInternal.h (modified) (1 diff)

trunk/src/VBox/VMM/VMMAll/APICAll.cpp

@@ -2979 +2979 @@
 static void apicSetInterruptFF(PVMCPU pVCpu, PDMAPICIRQ enmType)
 {
+#ifdef IN_RING3
+    /* IRQ state should be loaded as-is by "LoadExec". Changes can be made from LoadDone. */
+    Assert(pVCpu->pVMR3->enmVMState != VMSTATE_LOADING || PDMR3HasLoadedState(pVCpu->pVMR3));
+#endif
+
     switch (enmType)
     {
@@ -3035 +3040 @@
 VMM_INT_DECL(void) apicClearInterruptFF(PVMCPU pVCpu, PDMAPICIRQ enmType)
 {
+#ifdef IN_RING3
+    /* IRQ state should be loaded as-is by "LoadExec". Changes can be made from LoadDone. */
+    Assert(pVCpu->pVMR3->enmVMState != VMSTATE_LOADING || PDMR3HasLoadedState(pVCpu->pVMR3));
+#endif
+
     /* NMI/SMI can't be cleared. */
     switch (enmType)

trunk/src/VBox/VMM/VMMR3/APIC.cpp

@@ -1026 +1026 @@
     }
 
+    /*
+     * Restore per CPU state.
+     *
+     * Note! PDM will restore the VMCPU_FF_INTERRUPT_APIC flag for us.
+     *       This code doesn't touch it.  No devices should make us touch
+     *       it later during the restore either, only during the 'done' phase.
+     */
     for (VMCPUID idCpu = 0; idCpu < pVM->cCpus; idCpu++)
     {

trunk/src/VBox/VMM/VMMR3/PDM.cpp

@@ -830 +830 @@
 
 /**
+ * For APIC assertions.
+ *
+ * @returns true if we've loaded state.
+ * @param   pVM             The cross context VM structure.
+ */
+VMMR3_INT_DECL(bool)    PDMR3HasLoadedState(PVM pVM)
+{
+    return pVM->pdm.s.fStateLoaded;
+}
+
+
+/**
  * Bits that are saved in pass 0 and in the final pass.
  *
@@ -975 +987 @@
         /*
          * Load the interrupt and DMA states.
+         *
+         * The APIC, PIC and DMA devices does not restore these, we do.  In the
+         * APIC and PIC cases, it is possible that some devices is incorrectly
+         * setting IRQs during restore.  We'll warn when this happens.  (There
+         * are debug assertions in PDMDevMiscHlp.cpp and APICAll.cpp for
+         * catching the buggy device.)
          */
         for (VMCPUID idCpu = 0; idCpu < pVM->cCpus; idCpu++)
@@ -990 +1008 @@
                 return VERR_SSM_DATA_UNIT_FORMAT_CHANGED;
             }
-            AssertRelease(!VMCPU_FF_IS_SET(pVCpu, VMCPU_FF_INTERRUPT_APIC)); /** @todo r=bird: bogus assertion, see @ticketref{18331} */
+            AssertLogRelMsg(!VMCPU_FF_IS_SET(pVCpu, VMCPU_FF_INTERRUPT_APIC),
+                            ("VCPU%03u: VMCPU_FF_INTERRUPT_APIC set! Devices shouldn't set interrupts during state restore...\n", idCpu));
             if (fInterruptPending)
                 VMCPU_FF_SET(pVCpu, VMCPU_FF_INTERRUPT_APIC);
@@ -1004 +1023 @@
                 return VERR_SSM_DATA_UNIT_FORMAT_CHANGED;
             }
-            AssertRelease(!VMCPU_FF_IS_SET(pVCpu, VMCPU_FF_INTERRUPT_PIC)); /** @todo r=bird: bogus assertion, see @ticketref{18331} */
+            AssertLogRelMsg(!VMCPU_FF_IS_SET(pVCpu, VMCPU_FF_INTERRUPT_PIC),
+                            ("VCPU%03u: VMCPU_FF_INTERRUPT_PIC set!  Devices shouldn't set interrupts during state restore...\n", idCpu));
             if (fInterruptPending)
                 VMCPU_FF_SET(pVCpu, VMCPU_FF_INTERRUPT_PIC);
@@ -1020 +1040 @@
                     return VERR_SSM_DATA_UNIT_FORMAT_CHANGED;
                 }
-                AssertRelease(!VMCPU_FF_IS_SET(pVCpu, VMCPU_FF_INTERRUPT_NMI));
+                AssertLogRelMsg(!VMCPU_FF_IS_SET(pVCpu, VMCPU_FF_INTERRUPT_NMI), ("VCPU%3u: VMCPU_FF_INTERRUPT_NMI set!\n", idCpu));
                 if (fInterruptPending)
                     VMCPU_FF_SET(pVCpu, VMCPU_FF_INTERRUPT_NMI);
@@ -1034 +1054 @@
                     return VERR_SSM_DATA_UNIT_FORMAT_CHANGED;
                 }
-                AssertRelease(!VMCPU_FF_IS_SET(pVCpu, VMCPU_FF_INTERRUPT_SMI));
+                AssertLogRelMsg(!VMCPU_FF_IS_SET(pVCpu, VMCPU_FF_INTERRUPT_SMI), ("VCPU%3u: VMCPU_FF_INTERRUPT_SMI set!\n", idCpu));
                 if (fInterruptPending)
                     VMCPU_FF_SET(pVCpu, VMCPU_FF_INTERRUPT_SMI);
@@ -1125 +1145 @@
                                         pDevIns->pReg->szName, pDevIns->iInstance);
         }
+
+
+    /*
+     * Indicate that we've been called (for assertions).
+     */
+    pVM->pdm.s.fStateLoaded = true;
 
     return VINF_SUCCESS;

trunk/src/VBox/VMM/VMMR3/PDMDevMiscHlp.cpp

@@ -54 +54 @@
     PVM    pVM = pDevIns->Internal.s.pVMR3;
     PVMCPU pVCpu = &pVM->aCpus[0];  /* for PIC we always deliver to CPU 0, MP use APIC */
+
+    /* IRQ state should be loaded as-is by "LoadExec". Changes can be made from LoadDone. */
+    Assert(pVM->enmVMState != VMSTATE_LOADING || pVM->pdm.s.fStateLoaded);
+
     APICLocalInterrupt(pVCpu, 0 /* u8Pin */, 1 /* u8Level */, VINF_SUCCESS /* rcRZ */);
 }
@@ -64 +68 @@
     PVM pVM = pDevIns->Internal.s.pVMR3;
     PVMCPU pVCpu = &pVM->aCpus[0];  /* for PIC we always deliver to CPU 0, MP use APIC */
+
+    /* IRQ state should be loaded as-is by "LoadExec". Changes can be made from LoadDone. */
+    Assert(pVM->enmVMState != VMSTATE_LOADING || pVM->pdm.s.fStateLoaded);
+
     APICLocalInterrupt(pVCpu, 0 /* u8Pin */,  0 /* u8Level */, VINF_SUCCESS /* rcRZ */);
 }

trunk/src/VBox/VMM/include/PDMInternal.h

@@ -1080 +1080 @@
     /** Pending reset flags (PDMVMRESET_F_XXX). */
     uint32_t volatile               fResetFlags;
+
+    /** Set by pdmR3LoadExec for use in assertions. */
+    bool                            fStateLoaded;
     /** Alignment padding. */
-    uint32_t volatile               u32Padding;
+    bool                            afPadding[3];
 
     /** The tracing ID of the next device instance.