Changeset 98088 in vbox

Timestamp:

Jan 16, 2023 12:57:07 AM (23 months ago)

Author:

vboxsync

Message:

Main/ConsoleImpl: Added a dedicated read/write lock to the LED sets to protect potential activity queries during VM construction. bugref:9892

Location:

trunk/src/VBox/Main

Files:

• include/ConsoleImpl.h (modified) (1 diff)
• src-client/ConsoleImpl.cpp (modified) (4 diffs)
• src-client/ConsoleImpl2.cpp (modified) (1 diff)

trunk/src/VBox/Main/include/ConsoleImpl.h

@@ -1068 +1068 @@
     /** @name LEDs and their management
      * @{ */
+    /** Read/write lock separating LED allocations (write) from queries (read). */
+    RWLockHandle mLedLock;
     /** Number of LED sets in use in maLedSets. */
     uint32_t          mcLedSets;

trunk/src/VBox/Main/src-client/ConsoleImpl.cpp

@@ -427 +427 @@
 #endif
     , mBusMgr(NULL)
+    , mLedLock(LOCKCLASS_LISTOFOTHEROBJECTS /* must be higher than LOCKCLASS_OTHEROBJECT */)
     , m_pKeyStore(NULL)
     , mpIfSecKey(NULL)
@@ -879 +880 @@
     }
 
-    /* Release memory held by the LED sets. */
+    /* Release memory held by the LED sets (no need to take lock). */
     for (size_t idxSet = 0; idxSet < mcLedSets; idxSet++)
     {
         RTMemFree(maLedSets[idxSet].papLeds);
-        RTMemFree(maLedSets[idxSet].paSubTypes);
         maLedSets[idxSet].papLeds = NULL;
         maLedSets[idxSet].paSubTypes = NULL;
@@ -2828 +2828 @@
 {
     /*
-     * Note: we don't lock the console object here because
-     * readAndClearLed() should be thread safe.
-     */
-/** @todo r=bird: readAndClearLed is safe, provided that we're not running at the
- * same time as Console::i_allocateDriverLeds.  This assumption is not correct
- * during VM construction. */
-
-    /*
      * Make a roadmap of which DeviceType_T LED types are wanted:
      */
@@ -2854 +2846 @@
     /*
      * Collect all the LEDs in a single sweep through all drivers' sets:
+     *
+     * Because this method can be called by the frontend and others while the
+     * VM is being constructed, we use a dedicated lock to prevent stumbling
+     * into the allocator.
      */
     PDMLEDCORE aLEDs[DeviceType_End] = { {0} };
     Assert(aLEDs[1].u32 == 0 && aLEDs[DeviceType_End / 2].u32 == 0 && aLEDs[DeviceType_End - 1].u32 == 0); /* paranoia */
-    uint32_t idxSet = mcLedSets;
-    while (idxSet-- > 0)
-    {
-        /* Look inside this driver's set of LEDs and check if the types mask overlap with the request: */
-        PLEDSET pLS = &maLedSets[idxSet];
-        if (pLS->fTypes & fWanted)
-        {
-            uint32_t const        cLeds      = pLS->cLeds;
-            PPDMLED const * const papSrcLeds = pLS->papLeds;
-
-            /* Multi-type drivers (e.g. SCSI) have a subtype array which must be matched. */
-            DeviceType_T const *paSubTypes = pLS->paSubTypes;
-            if (paSubTypes)
-                for (uint32_t idxLed = 0; idxLed < cLeds; idxLed++)
+    {
+        AutoReadLock alock(mLedLock COMMA_LOCKVAL_SRC_POS);
+        uint32_t idxSet = mcLedSets;
+        while (idxSet-- > 0)
+        {
+            /* Look inside this driver's set of LEDs and check if the types mask overlap with the request: */
+            PLEDSET pLS = &maLedSets[idxSet];
+            if (pLS->fTypes & fWanted)
+            {
+                uint32_t const        cLeds      = pLS->cLeds;
+                PPDMLED const * const papSrcLeds = pLS->papLeds;
+
+                /* Multi-type drivers (e.g. SCSI) have a subtype array which must be matched. */
+                DeviceType_T const *paSubTypes = pLS->paSubTypes;
+                if (paSubTypes)
+                    for (uint32_t idxLed = 0; idxLed < cLeds; idxLed++)
+                    {
+                        DeviceType_T const enmType = paSubTypes[idxLed];
+                        Assert((unsigned)enmType < (unsigned)DeviceType_End);
+                        if (fWanted & RT_BIT_32((unsigned)enmType))
+                            aLEDs[enmType].u32 |= readAndClearLed(papSrcLeds[idxLed]);
+                    }
+                /* Single-type drivers (e.g. floppy) have the type in ->enmType */
+                else
                 {
-                    DeviceType_T const enmType = paSubTypes[idxLed];
-                    Assert((unsigned)enmType < (unsigned)DeviceType_End);
-                    if (fWanted & RT_BIT_32((unsigned)enmType))
-                        aLEDs[enmType].u32 |= readAndClearLed(papSrcLeds[idxLed]);
+                    uint32_t const idxType = ASMBitFirstSetU32(pLS->fTypes) - 1;
+                    for (uint32_t idxLed = 0; idxLed < cLeds; idxLed++)
+                        aLEDs[idxType].u32 |= readAndClearLed(papSrcLeds[idxLed]);
                 }
-            /* Single-type drivers (e.g. floppy) have the type in ->enmType */
-            else
-            {
-                uint32_t const idxType = ASMBitFirstSetU32(pLS->fTypes) - 1;
-                for (uint32_t idxLed = 0; idxLed < cLeds; idxLed++)
-                    aLEDs[idxType].u32 |= readAndClearLed(papSrcLeds[idxLed]);
             }
         }

trunk/src/VBox/Main/src-client/ConsoleImpl2.cpp

@@ -674 +674 @@
     Assert(!(fTypes & (RT_BIT_32(DeviceType_Null) | ~(RT_BIT_32(DeviceType_End) - 1))));
 
-    /* Grab a LED set entry before we start allocating anything so the destructor can do the cleanups. */
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); /* Caller should have this already. Need protect mcLedSets check and update. */
-    AssertStmt(mcLedSets < RT_ELEMENTS(maLedSets),
-               throw ConfigError("AllocateDriverPapLeds", VERR_OUT_OF_RANGE, "Too many LED sets"));
-    uint32_t const idxLedSet = mcLedSets++;
-    PLEDSET pLS = &maLedSets[idxLedSet];
-    pLS->papLeds = (PPDMLED *)RTMemAllocZ(sizeof(PPDMLED) * cLeds);
-    AssertStmt(pLS->papLeds, throw E_OUTOFMEMORY);
-    pLS->cLeds = cLeds;
-    pLS->fTypes = fTypes;
-    pLS->paSubTypes = NULL;
-
-    if (ppaSubTypes)
+    /* Preallocate the arrays we need, bunching them together. */
+    AssertCompile((unsigned)DeviceType_Null == 0);
+    PPDMLED *papLeds = (PPDMLED *)RTMemAllocZ((sizeof(PPDMLED) + (ppaSubTypes ? sizeof(**ppaSubTypes) : 0)) * cLeds);
+    AssertStmt(papLeds, throw E_OUTOFMEMORY);
+
+    /* Take the LED lock in allocation mode and see if there are more LED set entries availalbe. */
     {
-        AssertCompile((unsigned)DeviceType_Null == 0);
-        *ppaSubTypes = pLS->paSubTypes = (DeviceType_T *)RTMemAllocZ(sizeof(DeviceType_T) * cLeds);
-        AssertStmt(pLS->paSubTypes, throw E_OUTOFMEMORY);
+        AutoWriteLock alock(mLedLock COMMA_LOCKVAL_SRC_POS);
+        uint32_t const idxLedSet = mcLedSets;
+        if (idxLedSet < RT_ELEMENTS(maLedSets))
+        {
+            /* Initialize the set and return the index. */
+            PLEDSET pLS = &maLedSets[idxLedSet];
+            pLS->papLeds = papLeds;
+            pLS->cLeds = cLeds;
+            pLS->fTypes = fTypes;
+            if (ppaSubTypes)
+                 *ppaSubTypes = pLS->paSubTypes = (DeviceType_T *)&papLeds[cLeds];
+            else
+                pLS->paSubTypes = NULL;
+
+            mcLedSets = idxLedSet + 1;
+            LogRel2(("return idxLedSet=%d (mcLedSets=%u out of max %zu)\n", idxLedSet, mcLedSets, RT_ELEMENTS(maLedSets)));
+            return idxLedSet;
+        }
     }
 
-    LogRel2(("mcLedSets = %d, RT_ELEMENTS(maLedSets) = %d\n", mcLedSets, RT_ELEMENTS(maLedSets)));
-    return idxLedSet;
+    RTMemFree(papLeds);
+    AssertFailed();
+    throw ConfigError("AllocateDriverPapLeds", VERR_OUT_OF_RANGE, "Too many LED sets");
 }