Changeset 57224 in vbox for trunk/src/VBox/HostDrivers

Timestamp:

Aug 6, 2015 7:25:07 PM (10 years ago)

Author:

vboxsync

Message:

SUPDrv: Try deal more correctly with multiple SUPGIP_FLAGS_TESTING_ENABLE settings per session and multiple sessions setting & clearing SUPGIP_FLAGS_TESTING_ENABLE. Untested.

Location:

trunk/src/VBox/HostDrivers/Support

Files:

• SUPDrvGip.cpp (modified) (12 diffs)
• SUPDrvInternal.h (modified) (1 diff)

trunk/src/VBox/HostDrivers/Support/SUPDrvGip.cpp

@@ -635 +635 @@
         && pDevExt->pGip)
     {
-        uint32_t fFlags = pDevExt->pGip->fFlags;
-        fFlags &= ~SUPGIP_FLAGS_TESTING_ENABLE;
-        supdrvGipSetFlags(pDevExt, pSession, 0, fFlags);
+        supdrvGipSetFlags(pDevExt, pSession, 0, ~SUPGIP_FLAGS_TESTING_ENABLE);
         Assert(!pSession->fGipTestMode);
     }
@@ -2101 +2099 @@
 static void supdrvGipDoUpdateCpu(PSUPDRVDEVEXT pDevExt, PSUPGIPCPU pGipCpu, uint64_t u64NanoTS, uint64_t u64TSC, uint64_t iTick)
 {
-    uint64_t    u64TSCDelta;
-    uint32_t    u32UpdateIntervalTSC;
-    uint32_t    u32UpdateIntervalTSCSlack;
-    unsigned    iTSCHistoryHead;
-    bool        fUpdateCpuHz;
-    uint32_t    fGipFlags;
-    uint64_t    u64CpuHz;
-    uint32_t    u32TransactionId;
-
-    PSUPGLOBALINFOPAGE pGip = pDevExt->pGip;
+    uint64_t            u64TSCDelta;
+    bool                fUpdateCpuHz;
+    PSUPGLOBALINFOPAGE  pGip = pDevExt->pGip;
     AssertPtrReturnVoid(pGip);
 
@@ -2127 +2118 @@
     ASMAtomicWriteU64(&pGipCpu->u64TSC, u64TSC);
 
-    /* Determine when we need to update the TSC frequency. */
+    /*
+     * Determine if we need to update the CPU (TSC) frequency calculation.
+     *
+     * We don't need to keep realculating the frequency when it's invariant,
+     * unless the special tstGIP-2 testing mode is enabled.
+     */
     fUpdateCpuHz = pGip->u32Mode != SUPGIPMODE_INVARIANT_TSC;
-
-    /*
-     * Handle GIP test mode toggle.
-     */
-    fGipFlags = pGip->fFlags;
-    if (!(fGipFlags & SUPGIP_FLAGS_TESTING))
+    if (!(pGip->fFlags & SUPGIP_FLAGS_TESTING))
     { /* likely*/ }
     else
     {
+        uint32_t fGipFlags = pGip->fFlags;
         if (fGipFlags & (SUPGIP_FLAGS_TESTING_ENABLE | SUPGIP_FLAGS_TESTING_START))
         {
@@ -2145 +2137 @@
                 if (!fUpdateCpuHz)
                     pDevExt->uGipTestModeInvariantCpuHz = pGip->aCPUs[0].u64CpuHz;
-                fGipFlags &= ~SUPGIP_FLAGS_TESTING_START;
-                ASMAtomicWriteU32(&pGip->fFlags, fGipFlags);
+                ASMAtomicAndU32(&pGip->fFlags, ~SUPGIP_FLAGS_TESTING_START);
             }
             fUpdateCpuHz = true;
@@ -2158 +2149 @@
                 ASMAtomicWriteU64(&pGip->aCPUs[0].u64CpuHz, pDevExt->uGipTestModeInvariantCpuHz);
             }
-            fGipFlags &= ~(SUPGIP_FLAGS_TESTING_STOP | SUPGIP_FLAGS_TESTING);
-            ASMAtomicWriteU32(&pGip->fFlags, fGipFlags);
-        }
-    }
-
-    /*
-     * We don't need to keep realculating the frequency when it's invariant, so
-     * the remainder of this function is only for the sync and async TSC modes.
+            ASMAtomicAndU32(&pGip->fFlags, ~(SUPGIP_FLAGS_TESTING_STOP | SUPGIP_FLAGS_TESTING));
+        }
+    }
+
+    /*
+     * Calculate the CPU (TSC) frequency if necessary.
      */
     if (fUpdateCpuHz)
     {
+        uint64_t    u64CpuHz;
+        uint32_t    u32UpdateIntervalTSC;
+        uint32_t    u32UpdateIntervalTSCSlack;
+        uint32_t    u32TransactionId;
+        unsigned    iTSCHistoryHead;
+
         if (u64TSCDelta >> 32)
         {
@@ -2198 +2193 @@
          * Wait until we have at least one full history since the above history reset. The
          * assumption is that the majority of the previous history values will be tolerable.
-         * See @bugref{6710} comment #67.
+         * See @bugref{6710#c67}.
          */
         /** @todo Could we drop the fuding there now that we initializes the history
@@ -4778 +4773 @@
  *
  * @returns VBox status code.
+ * @retval  VERR_WRONG_ORDER if an enable-once-per-session flag is set again for
+ *          a session.
+ *
  * @param   pDevExt         Pointer to the device instance data.
  * @param   pSession        The support driver session.
@@ -4783 +4781 @@
  * @param   fAndMask        The AND mask of the GIP flags, see SUPGIP_FLAGS_XXX.
  *
+ * @remarks Caller must own the GIP mutex.
+ *
  * @remarks This function doesn't validate any of the flags.
  */
@@ -4789 +4789 @@
     uint32_t           cRefs;
     PSUPGLOBALINFOPAGE pGip = pDevExt->pGip;
+    AssertMsg((fOrMask & fAndMask) == fOrMask, ("%#x & %#x\n", fOrMask, fAndMask)); /* ASSUMED by code below */
 
     /*
@@ -4795 +4796 @@
     if (fOrMask & SUPGIP_FLAGS_TESTING_ENABLE)
     {
-        pSession->fGipTestMode = true;
-        cRefs = ASMAtomicIncU32(&pDevExt->cGipTestModeRefs);
-        if (cRefs == 1)
-            fOrMask |= SUPGIP_FLAGS_TESTING | SUPGIP_FLAGS_TESTING_START;
-    }
-    else
-    {
-        cRefs = ASMAtomicDecU32(&pDevExt->cGipTestModeRefs);
+        if (!pSession->fGipTestMode)
+        {
+            Assert(pDevExt->cGipTestModeRefs < _64K);
+            pSession->fGipTestMode = true;
+            cRefs = ++pDevExt->cGipTestModeRefs;
+            if (cRefs == 1)
+            {
+                fOrMask  |= SUPGIP_FLAGS_TESTING | SUPGIP_FLAGS_TESTING_START;
+                fAndMask &= ~SUPGIP_FLAGS_TESTING_STOP;
+            }
+        }
+        else
+        {
+            LogRelMax(10, ("supdrvGipSetFlags: SUPGIP_FLAGS_TESTING_ENABLE already set for this session\n"));
+            return VERR_WRONG_ORDER;
+        }
+    }
+    else if (   !(fAndMask & SUPGIP_FLAGS_TESTING_ENABLE)
+             && pSession->fGipTestMode)
+    {
+        Assert(pDevExt->cGipTestModeRefs > 0);
+        Assert(pDevExt->cGipTestModeRefs < _64K);
         pSession->fGipTestMode = false;
+        cRefs = --pDevExt->cGipTestModeRefs;
         if (!cRefs)
             fOrMask |= SUPGIP_FLAGS_TESTING_STOP;
-    }
-
-    /*
-     * Commit the flags.
-     */
-    uint32_t fFlags = ASMAtomicUoReadU32(&pGip->fFlags);
-    fFlags |= fOrMask;
-    fFlags &= fAndMask;
-    ASMAtomicWriteU32(&pGip->fFlags, fFlags);
+        else
+            fAndMask |= SUPGIP_FLAGS_TESTING_ENABLE;
+    }
+
+    /*
+     * Commit the flags.  This should be done as atomically as possible
+     * since the flag consumers won't be holding the GIP mutex.
+     */
+    ASMAtomicOrU32(&pGip->fFlags, fOrMask);
+    ASMAtomicAndU32(&pGip->fFlags, fAndMask);
+
     return VINF_SUCCESS;
 }
@@ -4831 +4849 @@
 {
     PSUPGLOBALINFOPAGE pGip;
+    int                rc;
 
     /*
@@ -4847 +4866 @@
         return VERR_INVALID_PARAMETER;
 
-    return supdrvGipSetFlags(pDevExt, pSession, fOrMask, fAndMask);
-}
-
+    /*
+     * Don't confuse supdrvGipSetFlags or anyone else by both setting
+     * and clearing the same flags.  AND takes precedence.
+     */
+    fOrMask &= fAndMask;
+
+    /*
+     * Take the loader lock to avoid having to think about races between two
+     * clients changing the flags at the same time (state is not simple).
+     */
+#ifdef SUPDRV_USE_MUTEX_FOR_GIP
+    RTSemMutexRequest(pDevExt->mtxGip, RT_INDEFINITE_WAIT);
+#else
+    RTSemFastMutexRequest(pDevExt->mtxGip);
+#endif
+
+    rc = supdrvGipSetFlags(pDevExt, pSession, fOrMask, fAndMask);
+
+#ifdef SUPDRV_USE_MUTEX_FOR_GIP
+    RTSemMutexRelease(pDevExt->mtxGip);
+#else
+    RTSemFastMutexRelease(pDevExt->mtxGip);
+#endif
+    return rc;
+}
+

trunk/src/VBox/HostDrivers/Support/SUPDrvInternal.h

@@ -757 +757 @@
      *  @{ */
     /** Reference counter for GIP test-mode sessions. */
-    uint32_t volatile               cGipTestModeRefs;
+    uint32_t                        cGipTestModeRefs;
     /** Cache of TSC frequency before enabling test-mode on invariant GIP systems. */
     uint64_t                        uGipTestModeInvariantCpuHz;