Changeset 21062 in vbox

Timestamp:

Jun 30, 2009 10:14:10 AM (15 years ago)

Author:

vboxsync

Message:

VMMDev: Ensure read/write order in hgcmCompletedWorker. (bug 4052)

Location:

trunk/src/VBox/Devices/VMMDev

Files:

• VBoxDev.cpp (modified) (6 diffs)
• VMMDevHGCM.cpp (modified) (9 diffs)
• VMMDevState.h (modified) (2 diffs)

trunk/src/VBox/Devices/VMMDev/VBoxDev.cpp

@@ -400 +400 @@
  * Port I/O Handler for the generic request interface
  * @see FNIOMIOPORTOUT for details.
+ *
+ * @todo Too long, suggest doing the request copying here and moving the
+ *       switch into a different function (or better case -> functions), and
+ *       looing the gotos.
  */
 static DECLCALLBACK(int) vmmdevRequestHandler(PPDMDEVINS pDevIns, void *pvUser, RTIOPORT Port, uint32_t u32, unsigned cb)
@@ -405 +409 @@
     VMMDevState *pThis = (VMMDevState*)pvUser;
     int rcRet = VINF_SUCCESS;
+    PDMCritSectEnter(&pThis->CritSect, VERR_SEM_BUSY);
 
     /*
@@ -420 +425 @@
         Log(("VMMDev request header size too small! size = %d\n", requestHeader.size));
         rcRet = VINF_SUCCESS;
-        goto end;
+        goto end; /** @todo shouldn't (/ no need to) write back.*/
     }
 
@@ -428 +433 @@
         Log(("VMMDev: guest header version (0x%08X) differs from ours (0x%08X)\n", requestHeader.version, VMMDEV_REQUEST_HEADER_VERSION));
         rcRet = VINF_SUCCESS;
-        goto end;
+        goto end; /** @todo shouldn't (/ no need to) write back.*/
     }
 
@@ -1596 +1601 @@
         PDMDevHlpPhysWrite(pDevIns, (RTGCPHYS)u32, &requestHeader, sizeof(requestHeader));
     }
+
+    PDMCritSectLeave(&pThis->CritSect);
     return rcRet;
 }
@@ -2276 +2283 @@
     /** @todo convert this into a config parameter like we do everywhere else! */
     pThis->cbGuestRAM = MMR3PhysGetRamSize(PDMDevHlpGetVM(pDevIns));
+
+    /*
+     * Create the critical section for the device.
+     */
+    rc = PDMDevHlpCritSectInit(pDevIns, &pThis->CritSect, "VMMDev");
+    AssertRCReturn(rc, rc);
+    /* Later: pDevIns->pCritSectR3 = &pThis->CritSect; */
 
     /*

trunk/src/VBox/Devices/VMMDev/VMMDevHGCM.cpp

@@ -391 +391 @@
     }
 
-    VMMDevHGCMConnect *pHGCMConnectCopy = (VMMDevHGCMConnect *)(pSavedCmd+1); 
+    VMMDevHGCMConnect *pHGCMConnectCopy = (VMMDevHGCMConnect *)(pSavedCmd+1);
 
     memcpy(pHGCMConnectCopy, pHGCMConnect, pHGCMConnect->header.header.size);
@@ -1019 +1019 @@
 
                                          AssertRCBreak(rc);
-                                         
+
                                          offPage = 0; /* A next page is read from 0 offset. */
                                          cbRemaining -= cbChunk;
@@ -1159 +1159 @@
 
                                          AssertRCBreak(rc);
-                                         
+
                                          offPage = 0; /* A next page is read from 0 offset. */
                                          cbRemaining -= cbChunk;
@@ -1324 +1324 @@
         }
 
+        /*
+         * Enter and leave the critical section here so we make sure
+         * vmmdevRequestHandler has completed before we read & write
+         * the request. (This isn't 100% optimal, but it solves the
+         * 3.0 blocker.)
+         */
+        /** @todo s/pVMMDevState/pThis/g */
+        /** @todo It would be faster if this interface would use MMIO2 memory and we
+         *        didn't have to mess around with PDMDevHlpPhysRead/Write. We're
+         *        reading the header 3 times now and writing the request back twice. */
+        PDMCritSectEnter(&pVMMDevState->CritSect, VERR_SEM_BUSY);
+        PDMCritSectLeave(&pVMMDevState->CritSect);
+
         PDMDevHlpPhysRead(pVMMDevState->pDevIns, pCmd->GCPhys, pHeader, pCmd->cbSize);
 
@@ -1441 +1454 @@
                             uint32_t size = pGuestParm->u.Pointer.size;
 
-                            if (size > 0) 
+                            if (size > 0)
                             {
                                 if (pGuestParm->type != VMMDevHGCMParmType_LinAddr_In)
@@ -1504 +1517 @@
                             uint32_t size = pGuestParm->u.Pointer.size;
 
-                            if (size > 0) 
+                            if (size > 0)
                             {
                                 if (pGuestParm->type != VMMDevHGCMParmType_LinAddr_In)
@@ -1583 +1596 @@
     VMMDevState *pVMMDevState = PDMIHGCMPORT_2_VMMDEVSTATE(pInterface);
 
+/** @todo no longer necessary to forward to EMT, but it might be more
+ *        efficient...? */
     /* Not safe to execute asynchroneously; forward to EMT */
     int rc = VMR3ReqCallEx(PDMDevHlpGetVM(pVMMDevState->pDevIns), VMCPUID_ANY, NULL, 0, VMREQFLAGS_NO_WAIT | VMREQFLAGS_VOID,
@@ -1857 +1872 @@
             rc = SSMR3GetU32(pSSM, &u32);
             AssertRCReturn(rc, rc);
-            
+
             vmmdevHGCMAddCommand (pVMMDevState, pCmd, GCPhys, cbSize, VBOXHGCMCMDTYPE_LOADSTATE);
         }
@@ -2138 +2153 @@
 
                    RTMemFree(pIter);
-               
+
                    VMMDevNotifyGuest (pVMMDevState, VMMDEV_EVENT_HGCM);
                 }

trunk/src/VBox/Devices/VMMDev/VMMDevState.h

@@ -91 +91 @@
     RTGCPHYS32 GCPhysVMMDevRAM;
 
-    /** R3 pointer to VMMDev Heap RAM area 
+    /** R3 pointer to VMMDev Heap RAM area
      */
     R3PTRTYPE(VMMDevMemory *) pVMMDevHeapR3;
@@ -191 +191 @@
     } SharedFolders;
 
+    /** The critical section for this device. */
+    PDMCRITSECT CritSect;
 } VMMDevState;