Changeset 75520 in vbox for trunk/src/VBox/Devices

Timestamp:

Nov 16, 2018 4:06:57 PM (6 years ago)

Author:

vboxsync

svn:sync-xref-src-repo-rev:

126678

Message:

VMMDev/HGCM: Eliminated the unlock-delay hack (from 3.0) and implemented lock based approach for HGCM request (disabled). bugref:9172

Location:

trunk/src/VBox/Devices/VMMDev

Files:

• VMMDev.cpp (modified) (9 diffs)
• VMMDevHGCM.cpp (modified) (16 diffs)
• VMMDevHGCM.h (modified) (1 diff)
• VMMDevState.h (modified) (1 diff)

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

@@ -1776 +1776 @@
  * @param   tsArrival       The STAM_GET_TS() value when the request arrived.
  */
-static int vmmdevReqHandler_HGCMCall(PVMMDEV pThis, VMMDevRequestHeader *pReqHdr, RTGCPHYS GCPhysReqHdr, uint64_t tsArrival)
+static int vmmdevReqHandler_HGCMCall(PVMMDEV pThis, VMMDevRequestHeader *pReqHdr, RTGCPHYS GCPhysReqHdr,
+                                     uint64_t tsArrival, PVMMDEVREQLOCK *ppLock)
 {
     VMMDevHGCMCall *pReq = (VMMDevHGCMCall *)pReqHdr;
@@ -1786 +1787 @@
         Log2(("%.*Rhxd\n", pReq->header.header.size, pReq));
 
-        return vmmdevHGCMCall(pThis, pReq, pReq->header.header.size, GCPhysReqHdr, pReq->header.header.requestType, tsArrival);
+        return vmmdevHGCMCall(pThis, pReq, pReq->header.header.size, GCPhysReqHdr, pReq->header.header.requestType,
+                              tsArrival, ppLock);
     }
 
@@ -2552 +2554 @@
 
 /**
+ * Sets request status to VINF_HGCM_ASYNC_EXECUTE.
+ *
+ * @param   pThis           The VMM device instance data.
+ * @param   GCPhysReqHdr    The guest physical address of the request.
+ * @param   ppLock          Pointer to the request locking info.  NULL if not
+ *                          locked.
+ */
+DECLINLINE(void) vmmdevReqHdrSetHgcmAsyncExecute(PVMMDEV pThis, RTGCPHYS GCPhysReqHdr, PVMMDEVREQLOCK pLock)
+{
+    if (pLock)
+        ((VMMDevRequestHeader volatile *)pLock->pvReq)->rc = VINF_HGCM_ASYNC_EXECUTE;
+    else
+    {
+        int32_t rcReq = VINF_HGCM_ASYNC_EXECUTE;
+        PDMDevHlpPhysWrite(pThis->pDevIns, GCPhysReqHdr + RT_UOFFSETOF(VMMDevRequestHeader, rc), &rcReq, sizeof(rcReq));
+    }
+}
+
+
+/** @name VMMDEVREQDISP_POST_F_XXX - post dispatcher optimizations.
+ * @{ */
+#define VMMDEVREQDISP_POST_F_NO_WRITE_OUT    RT_BIT_32(0)
+/** @} */
+
+
+/**
  * Dispatch the request to the appropriate handler function.
  *
@@ -2560 +2588 @@
  *                          HGCM).
  * @param   tsArrival       The STAM_GET_TS() value when the request arrived.
- * @param   pfDelayedUnlock Where to indicate whether the critical section exit
- *                          needs to be delayed till after the request has been
- *                          written back. This is a HGCM kludge, see critsect
- *                          work in hgcmCompletedWorker for more details.
+ * @param   pfPostOptimize  HGCM optimizations, VMMDEVREQDISP_POST_F_XXX.
  */
 static int vmmdevReqDispatcher(PVMMDEV pThis, VMMDevRequestHeader *pReqHdr, RTGCPHYS GCPhysReqHdr,
-                               uint64_t tsArrival, bool *pfDelayedUnlock)
+                               uint64_t tsArrival, uint32_t *pfPostOptimize, PVMMDEVREQLOCK *ppLock)
 {
     int rcRet = VINF_SUCCESS;
-    *pfDelayedUnlock = false;
+    Assert(*pfPostOptimize == 0);
 
     switch (pReqHdr->requestType)
@@ -2679 +2704 @@
 #ifdef VBOX_WITH_HGCM
         case VMMDevReq_HGCMConnect:
+            vmmdevReqHdrSetHgcmAsyncExecute(pThis, GCPhysReqHdr, *ppLock);
             pReqHdr->rc = vmmdevReqHandler_HGCMConnect(pThis, pReqHdr, GCPhysReqHdr);
-            *pfDelayedUnlock = true;
+            Assert(pReqHdr->rc == VINF_HGCM_ASYNC_EXECUTE || RT_FAILURE_NP(pReqHdr->rc));
+            if (RT_SUCCESS(pReqHdr->rc))
+                *pfPostOptimize |= VMMDEVREQDISP_POST_F_NO_WRITE_OUT;
             break;
 
         case VMMDevReq_HGCMDisconnect:
+            vmmdevReqHdrSetHgcmAsyncExecute(pThis, GCPhysReqHdr, *ppLock);
             pReqHdr->rc = vmmdevReqHandler_HGCMDisconnect(pThis, pReqHdr, GCPhysReqHdr);
-            *pfDelayedUnlock = true;
+            Assert(pReqHdr->rc == VINF_HGCM_ASYNC_EXECUTE || RT_FAILURE_NP(pReqHdr->rc));
+            if (RT_SUCCESS(pReqHdr->rc))
+                *pfPostOptimize |= VMMDEVREQDISP_POST_F_NO_WRITE_OUT;
             break;
 
@@ -2694 +2725 @@
         case VMMDevReq_HGCMCall:
 # endif /* VBOX_WITH_64_BITS_GUESTS */
-            pReqHdr->rc = vmmdevReqHandler_HGCMCall(pThis, pReqHdr, GCPhysReqHdr, tsArrival);
-            *pfDelayedUnlock = true;
+            vmmdevReqHdrSetHgcmAsyncExecute(pThis, GCPhysReqHdr, *ppLock);
+            pReqHdr->rc = vmmdevReqHandler_HGCMCall(pThis, pReqHdr, GCPhysReqHdr, tsArrival, ppLock);
+            Assert(pReqHdr->rc == VINF_HGCM_ASYNC_EXECUTE || RT_FAILURE_NP(pReqHdr->rc));
+            if (RT_SUCCESS(pReqHdr->rc))
+                *pfPostOptimize |= VMMDEVREQDISP_POST_F_NO_WRITE_OUT;
             break;
 
         case VMMDevReq_HGCMCancel:
             pReqHdr->rc = vmmdevReqHandler_HGCMCancel(pThis, pReqHdr, GCPhysReqHdr);
-            *pfDelayedUnlock = true;
             break;
 
@@ -2870 +2903 @@
 
     int                  rcRet          = VINF_SUCCESS;
-    bool                 fDelayedUnlock = false;
     VMMDevRequestHeader *pRequestHeader = NULL;
 
@@ -2898 +2930 @@
             {
                 memcpy(pRequestHeader, &requestHeader, sizeof(VMMDevRequestHeader));
-                size_t cbLeft = requestHeader.size - sizeof(VMMDevRequestHeader);
+
+                VMMDEVREQLOCK   Lock   = { NULL, { NULL, NULL } };
+                PVMMDEVREQLOCK  pLock  = NULL;
+                size_t          cbLeft = requestHeader.size - sizeof(VMMDevRequestHeader);
                 if (cbLeft)
-                    PDMDevHlpPhysRead(pDevIns,
-                                      (RTGCPHYS)u32             + sizeof(VMMDevRequestHeader),
-                                      (uint8_t *)pRequestHeader + sizeof(VMMDevRequestHeader),
-                                      cbLeft);
-
+                {
+#if 1
+                    RT_NOREF_PV(Lock);
+#else
+                    if (   (   requestHeader.requestType == VMMDevReq_HGCMCall32
+                            || requestHeader.requestType == VMMDevReq_HGCMCall64)
+                        && ((u32 + requestHeader.size) >> X86_PAGE_SHIFT) == (u32 >> X86_PAGE_SHIFT)
+                        && RT_SUCCESS(PDMDevHlpPhysGCPhys2CCPtr(pDevIns, u32, 0 /*fFlags*/, &Lock.pvReq, &Lock.Lock)) )
+                    {
+                        memcpy((uint8_t *)pRequestHeader + sizeof(VMMDevRequestHeader),
+                               (uint8_t *)Lock.pvReq     + sizeof(VMMDevRequestHeader), cbLeft);
+                        pLock = &Lock;
+                    }
+                    else
+#endif
+                        PDMDevHlpPhysRead(pDevIns,
+                                          (RTGCPHYS)u32             + sizeof(VMMDevRequestHeader),
+                                          (uint8_t *)pRequestHeader + sizeof(VMMDevRequestHeader),
+                                          cbLeft);
+                }
+
+                uint32_t fPostOptimize  = 0;
                 PDMCritSectEnter(&pThis->CritSect, VERR_IGNORED);
-                rcRet = vmmdevReqDispatcher(pThis, pRequestHeader, u32, tsArrival, &fDelayedUnlock);
-                if (!fDelayedUnlock)
-                    PDMCritSectLeave(&pThis->CritSect);
+                rcRet = vmmdevReqDispatcher(pThis, pRequestHeader, u32, tsArrival, &fPostOptimize, &pLock);
+                PDMCritSectLeave(&pThis->CritSect);
+
+                /*
+                 * Write the result back to guest memory (unless it is a locked HGCM call).
+                 */
+                if (!(fPostOptimize & VMMDEVREQDISP_POST_F_NO_WRITE_OUT))
+                {
+                    if (pLock)
+                        memcpy(pLock->pvReq, pRequestHeader, pRequestHeader->size);
+                    else
+                        PDMDevHlpPhysWrite(pDevIns, u32, pRequestHeader, pRequestHeader->size);
+                }
+
+                RTMemFree(pRequestHeader);
+                return rcRet;
             }
-            else
-            {
-                Log(("VMMDev: RTMemAlloc failed!\n"));
-                requestHeader.rc = VERR_NO_MEMORY;
-            }
+
+            Log(("VMMDev: RTMemAlloc failed!\n"));
+            requestHeader.rc = VERR_NO_MEMORY;
         }
         else
@@ -2930 +2993 @@
 
     /*
-     * Write the result back to guest memory
+     * Write the result back to guest memory.
      */
-    if (pRequestHeader)
-    {
-        PDMDevHlpPhysWrite(pDevIns, u32, pRequestHeader, pRequestHeader->size);
-        if (fDelayedUnlock)
-            PDMCritSectLeave(&pThis->CritSect);
-        RTMemFree(pRequestHeader);
-    }
-    else
-    {
-        /* early error case; write back header only */
-        PDMDevHlpPhysWrite(pDevIns, u32, &requestHeader, sizeof(requestHeader));
-        Assert(!fDelayedUnlock);
-    }
+    PDMDevHlpPhysWrite(pDevIns, u32, &requestHeader, sizeof(requestHeader));
 
     return rcRet;

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

@@ -140 +140 @@
     VMMDevRequestType   enmRequestType;
 
+    /** Pointer to the locked request, NULL if not locked. */
+    void               *pvReqLocked;
+    /** The PGM lock for GCPhys if pvReqLocked is not NULL. */
+    PGMPAGEMAPLOCK      ReqMapLock;
+
     /** The STAM_GET_TS() value when the request arrived. */
     uint64_t            tsArrival;
@@ -236 +241 @@
 /** Deallocate VBOXHGCMCMD memory.
  *
+ * @param   pThis           The VMMDev instance data.
  * @param   pCmd            Command to deallocate.
  */
-static void vmmdevHGCMCmdFree(PVBOXHGCMCMD pCmd)
+static void vmmdevHGCMCmdFree(PVMMDEV pThis, PVBOXHGCMCMD pCmd)
 {
     if (pCmd)
@@ -259 +265 @@
                     RTMemFree(pGuestParm->u.ptr.paPages);
             }
+        }
+
+        if (pCmd->pvReqLocked)
+        {
+            PDMDevHlpPhysReleasePageMappingLock(pThis->pDevIns, &pCmd->ReqMapLock);
+            pCmd->pvReqLocked = NULL;
         }
 
@@ -869 +881 @@
  */
 int vmmdevHGCMCall(PVMMDEV pThis, const VMMDevHGCMCall *pHGCMCall, uint32_t cbHGCMCall, RTGCPHYS GCPhys,
-                   VMMDevRequestType enmRequestType, uint64_t tsArrival)
+                   VMMDevRequestType enmRequestType, uint64_t tsArrival, PVMMDEVREQLOCK *ppLock)
 {
     LogFunc(("client id = %d, function = %d, cParms = %d, enmRequestType = %d\n",
@@ -895 +907 @@
     {
         pCmd->tsArrival = tsArrival;
+        PVMMDEVREQLOCK pLock = *ppLock;
+        if (pLock)
+        {
+            pCmd->ReqMapLock  = pLock->Lock;
+            pCmd->pvReqLocked = pLock->pvReq;
+            *ppLock = NULL;
+        }
+
         rc = vmmdevHGCMCallFetchGuestParms(pThis, pCmd, pHGCMCall, cbHGCMCall, enmRequestType, cbHGCMParmStruct);
         if (RT_SUCCESS(rc))
@@ -912 +932 @@
                 if (RT_SUCCESS(rc))
                 {
+                    Assert(rc == VINF_HGCM_ASYNC_EXECUTE);
+
                     /*
                      * Done.  Just update statistics and return.
@@ -930 +952 @@
             }
         }
-        vmmdevHGCMCmdFree(pCmd);
+        vmmdevHGCMCmdFree(pThis, pCmd);
     }
     return rc;
@@ -1100 +1122 @@
     if (RT_LIKELY(!pCmd->fCancelled))
     {
-/** @todo r=bird: Given that we involve the heap and call PGM three times, we
- *        would most likely be better off locking the buffer memory here.  If
- *        nothing else, it will avoid taking the PGM lock once. */
-
-        VMMDevHGCMRequestHeader *pHeader = (VMMDevHGCMRequestHeader *)RTMemAlloc(pCmd->cbRequest);
-        if (pHeader)
+        if (!pCmd->pvReqLocked)
         {
             /*
-             * 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.)
+             * Request is not locked:
              */
-            /** @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(&pThis->CritSect, VERR_SEM_BUSY);
-            PDMCritSectLeave(&pThis->CritSect);
-
-            /*
-             * Read the request from the guest memory for updating.
-             * The request data is not be used for anything but checking the request type.
-             */
-            PDMDevHlpPhysRead(pThis->pDevIns, pCmd->GCPhys, pHeader, pCmd->cbRequest);
-            RT_UNTRUSTED_NONVOLATILE_COPY_FENCE();
+            VMMDevHGCMRequestHeader *pHeader = (VMMDevHGCMRequestHeader *)RTMemAlloc(pCmd->cbRequest);
+            if (pHeader)
+            {
+                /*
+                 * Read the request from the guest memory for updating.
+                 * The request data is not be used for anything but checking the request type.
+                 */
+                PDMDevHlpPhysRead(pThis->pDevIns, pCmd->GCPhys, pHeader, pCmd->cbRequest);
+                RT_UNTRUSTED_NONVOLATILE_COPY_FENCE();
+
+                /* Verify the request type. This is the only field which is used from the guest memory. */
+                const VMMDevRequestType enmRequestType = pHeader->header.requestType;
+                if (   enmRequestType == pCmd->enmRequestType
+                    || enmRequestType == VMMDevReq_HGCMCancel)
+                {
+                    RT_UNTRUSTED_VALIDATED_FENCE();
+
+                    /*
+                     * Update parameters and data buffers.
+                     */
+                    switch (enmRequestType)
+                    {
+#ifdef VBOX_WITH_64_BITS_GUESTS
+                        case VMMDevReq_HGCMCall64:
+                        case VMMDevReq_HGCMCall32:
+#else
+                        case VMMDevReq_HGCMCall:
+#endif
+                        {
+                            VMMDevHGCMCall *pHGCMCall = (VMMDevHGCMCall *)pHeader;
+                            rc = vmmdevHGCMCompleteCallRequest(pThis, pCmd, pHGCMCall);
+#ifdef VBOX_WITH_DTRACE
+                            idFunction = pCmd->u.call.u32Function;
+                            idClient   = pCmd->u.call.u32ClientID;
+#endif
+                            break;
+                        }
+
+                        case VMMDevReq_HGCMConnect:
+                        {
+                            /* save the client id in the guest request packet */
+                            VMMDevHGCMConnect *pHGCMConnect = (VMMDevHGCMConnect *)pHeader;
+                            pHGCMConnect->u32ClientID = pCmd->u.connect.u32ClientID;
+                            break;
+                        }
+
+                        default:
+                            /* make compiler happy */
+                            break;
+                    }
+                }
+                else
+                {
+                    /* Guest has changed the command type. */
+                    LogRelMax(50, ("VMMDEV: Invalid HGCM command: pCmd->enmCmdType = 0x%08X, pHeader->header.requestType = 0x%08X\n",
+                                   pCmd->enmCmdType, pHeader->header.requestType));
+
+                    ASSERT_GUEST_FAILED_STMT(rc = VERR_INVALID_PARAMETER);
+                }
+
+                /* Setup return code for the guest. */
+                if (RT_SUCCESS(rc))
+                    pHeader->result = result;
+                else
+                    pHeader->result = rc;
+
+                /* First write back the request. */
+                PDMDevHlpPhysWrite(pThis->pDevIns, pCmd->GCPhys, pHeader, pCmd->cbRequest);
+
+                /* Mark request as processed. */
+                pHeader->fu32Flags |= VBOX_HGCM_REQ_DONE;
+
+                /* Second write the flags to mark the request as processed. */
+                PDMDevHlpPhysWrite(pThis->pDevIns, pCmd->GCPhys + RT_UOFFSETOF(VMMDevHGCMRequestHeader, fu32Flags),
+                                   &pHeader->fu32Flags, sizeof(pHeader->fu32Flags));
+
+                /* Now, when the command was removed from the internal list, notify the guest. */
+                VMMDevNotifyGuest(pThis, VMMDEV_EVENT_HGCM);
+
+                RTMemFree(pHeader);
+            }
+            else
+            {
+                LogRelMax(10, ("VMMDev: Failed to allocate %u bytes for HGCM request completion!!!\n", pCmd->cbRequest));
+            }
+        }
+        /*
+         * Request was locked:
+         */
+        else
+        {
+            VMMDevHGCMRequestHeader volatile *pHeader = (VMMDevHGCMRequestHeader volatile *)pCmd->pvReqLocked;
 
             /* Verify the request type. This is the only field which is used from the guest memory. */
@@ -1183 +1276 @@
                 pHeader->result = rc;
 
-            /* First write back the request. */
-            PDMDevHlpPhysWrite(pThis->pDevIns, pCmd->GCPhys, pHeader, pCmd->cbRequest);
-
             /* Mark request as processed. */
-            pHeader->fu32Flags |= VBOX_HGCM_REQ_DONE;
-
-            /* Second write the flags to mark the request as processed. */
-            PDMDevHlpPhysWrite(pThis->pDevIns, pCmd->GCPhys + RT_UOFFSETOF(VMMDevHGCMRequestHeader, fu32Flags),
-                               &pHeader->fu32Flags, sizeof(pHeader->fu32Flags));
+            ASMAtomicOrU32(&pHeader->fu32Flags, VBOX_HGCM_REQ_DONE);
 
             /* Now, when the command was removed from the internal list, notify the guest. */
             VMMDevNotifyGuest(pThis, VMMDEV_EVENT_HGCM);
-
-            RTMemFree(pHeader);
-        }
-        else
-        {
-            LogRelMax(10, ("VMMDev: Failed to allocate %u bytes for HGCM request completion!!!\n", pCmd->cbRequest));
         }
     }
@@ -1216 +1296 @@
     /* Deallocate the command memory. */
     VBOXDD_HGCMCALL_COMPLETED_DONE(pCmd, idFunction, idClient, result);
-    vmmdevHGCMCmdFree(pCmd);
+    vmmdevHGCMCmdFree(pThis, pCmd);
 
 #ifndef VBOX_WITHOUT_RELEASE_STATISTICS
@@ -1773 +1853 @@
         *ppRestoredCmd = pCmd;
     else
-        vmmdevHGCMCmdFree(pCmd);
+        vmmdevHGCMCmdFree(pThis, pCmd);
 
     return rc;
@@ -1885 +1965 @@
          */
         VMMDevHGCMRequestHeader *pReqHdr = (VMMDevHGCMRequestHeader *)RTMemAlloc(pCmd->cbRequest);
-        AssertBreakStmt(pReqHdr, vmmdevHGCMCmdFree(pCmd); rcFunc = VERR_NO_MEMORY);
+        AssertBreakStmt(pReqHdr, vmmdevHGCMCmdFree(pThis, pCmd); rcFunc = VERR_NO_MEMORY);
 
         PDMDevHlpPhysRead(pThis->pDevIns, pCmd->GCPhys, pReqHdr, pCmd->cbRequest);
@@ -1905 +1985 @@
                 {
                     Assert(pCmd != pRestoredCmd); /* vmmdevHGCMRestoreCommand must allocate restored command. */
-                    vmmdevHGCMCmdFree(pCmd);
+                    vmmdevHGCMCmdFree(pThis, pCmd);
                     pCmd = pRestoredCmd;
                 }
@@ -1978 +2058 @@
 
             /* Deallocate the command memory. */
-            vmmdevHGCMCmdFree(pCmd);
+            vmmdevHGCMCmdFree(pThis, pCmd);
         }
 
@@ -1989 +2069 @@
         {
             RTListNodeRemove(&pCmd->node);
-            vmmdevHGCMCmdFree(pCmd);
+            vmmdevHGCMCmdFree(pThis, pCmd);
         }
     }
@@ -2008 +2088 @@
     {
         vmmdevHGCMRemoveCommand(pThis, pCmd);
-        vmmdevHGCMCmdFree(pCmd);
-    }
-}
+        vmmdevHGCMCmdFree(pThis, pCmd);
+    }
+}

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

@@ -25 +25 @@
 int vmmdevHGCMDisconnect(VMMDevState *pVMMDevState, const VMMDevHGCMDisconnect *pHGCMDisconnect, RTGCPHYS GCPtr);
 int vmmdevHGCMCall(VMMDevState *pVMMDevState, const VMMDevHGCMCall *pHGCMCall, uint32_t cbHGCMCall, RTGCPHYS GCPtr,
-                   VMMDevRequestType enmRequestType, uint64_t tsArrival);
+                   VMMDevRequestType enmRequestType, uint64_t tsArrival, PVMMDEVREQLOCK *ppLock);
 int vmmdevHGCMCancel(VMMDevState *pVMMDevState, const VMMDevHGCMCancel *pHGCMCancel, RTGCPHYS GCPtr);
 int vmmdevHGCMCancel2(VMMDevState *pVMMDevState, RTGCPHYS GCPtr);

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

@@ -31 +31 @@
 
 #define VMMDEV_WITH_ALT_TIMESYNC
+
+/** Request locking structure (HGCM optimization). */
+typedef struct VMMDEVREQLOCK
+{
+    void          *pvReq;
+    PGMPAGEMAPLOCK Lock;
+} VMMDEVREQLOCK;
+/** Pointer to a request lock structure. */
+typedef VMMDEVREQLOCK *PVMMDEVREQLOCK;
 
 typedef struct DISPLAYCHANGEREQUEST