Changeset 27106 in vbox for trunk

Timestamp:

Mar 5, 2010 3:49:11 PM (15 years ago)

Author:

vboxsync

Message:

common/VBoxGuest: ballooning review.

Location:

trunk/src/VBox/Additions/common/VBoxGuest

Files:

• VBoxGuest.cpp (modified) (21 diffs)
• VBoxGuestInternal.h (modified) (2 diffs)

trunk/src/VBox/Additions/common/VBoxGuest/VBoxGuest.cpp

@@ -48 +48 @@
 #endif
 
-const size_t cbChangeMemBalloonReq = RT_OFFSETOF(VMMDevChangeMemBalloon, aPhysPage[VMMDEV_MEMORY_BALLOON_CHUNK_PAGES]);
+
+/*******************************************************************************
+*   Global Variables                                                           *
+*******************************************************************************/
+static const size_t cbChangeMemBalloonReq = RT_OFFSETOF(VMMDevChangeMemBalloon, aPhysPage[VMMDEV_MEMORY_BALLOON_CHUNK_PAGES]);
+
 
 
@@ -260 +265 @@
 {
     VMMDevReportGuestInfo *pReq;
-    int rc;
-    rc = VbglGRAlloc((VMMDevRequestHeader **)&pReq, sizeof(*pReq), VMMDevReq_ReportGuestInfo);
+    int rc = VbglGRAlloc((VMMDevRequestHeader **)&pReq, sizeof(*pReq), VMMDevReq_ReportGuestInfo);
     if (RT_SUCCESS(rc))
     {
@@ -298 +302 @@
      * doesn't work for R3-provided memory, therefore ignore the return value. Unprotect
      * done when object is freed. */
-    rc = RTR0MemObjProtect(*pMemObj, 0, VMMDEV_MEMORY_BALLOON_CHUNK_SIZE, RTMEM_PROT_NONE);
-    NOREF(rc);
+    RTR0MemObjProtect(*pMemObj, 0, VMMDEV_MEMORY_BALLOON_CHUNK_SIZE, RTMEM_PROT_NONE);
 
     pReq->fInflate = true;
@@ -313 +316 @@
 
 /**
- * Deflate the balloon by one chunk represented by an R0 memory object.
+ * Deflate the balloon by one chunk - info the host and free the memory object.
  *
  * @returns IPRT status code.
@@ -342 +345 @@
     }
 
+    /* undo previous protec call, ignore rc for reasons stated there. */
+    RTR0MemObjProtect(*pMemObj, 0, VMMDEV_MEMORY_BALLOON_CHUNK_SIZE, RTMEM_PROT_READ | RTMEM_PROT_WRITE);
+    /*RTR0MemObjProtect(*pMemObj, 0, VMMDEV_MEMORY_BALLOON_CHUNK_SIZE, RTMEM_PROT_READ | RTMEM_PROT_WRITE | RTMEM_PROT_EXEC); - probably not safe... */
+
     rc = RTR0MemObjFree(*pMemObj, true);
     if (RT_FAILURE(rc))
+    {
+        LogRel(("vboxGuestBalloonDeflate: RTR0MemObjFree(%p,true) -> %Rrc; this is *BAD*!\n", *pMemObj, rc));
         return rc;
+    }
 
     *pMemObj = NIL_RTR0MEMOBJ;
@@ -355 +365 @@
  *
  * @returns VBox status code.
- * @param   pDevExt         The device extension
+ * @param   pDevExt         The device extension.
  * @param   cBalloonChunks  The new size of the balloon in chunks of 1MB.
  */
@@ -369 +379 @@
         if (cBalloonChunks > pDevExt->MemBalloon.cMaxChunks)
         {
-            AssertMsgFailed(("vboxGuestSetBalloonSize illegal balloon size %d (max=%d)\n",
-                             cBalloonChunks, pDevExt->MemBalloon.cMaxChunks));
+            LogRel(("vboxGuestSetBalloonSizeKernel: illegal balloon size %u (max=%u)\n",
+                    cBalloonChunks, pDevExt->MemBalloon.cMaxChunks));
             return VERR_INVALID_PARAMETER;
         }
@@ -380 +390 @@
             && !pDevExt->MemBalloon.paMemObj)
         {
-            pDevExt->MemBalloon.paMemObj = (RTR0MEMOBJ*)RTMemAllocZ(sizeof(RTR0MEMOBJ) * pDevExt->MemBalloon.cMaxChunks);
+            pDevExt->MemBalloon.paMemObj = (PRTR0MEMOBJ)RTMemAllocZ(sizeof(RTR0MEMOBJ) * pDevExt->MemBalloon.cMaxChunks);
             if (!pDevExt->MemBalloon.paMemObj)
             {
@@ -399 +409 @@
                 rc = RTR0MemObjAllocPhysNC(&pDevExt->MemBalloon.paMemObj[i],
                                            VMMDEV_MEMORY_BALLOON_CHUNK_SIZE, NIL_RTHCPHYS);
-                if (RT_UNLIKELY(rc == VERR_NOT_SUPPORTED))
-                {
-                    /* not supported -- fall back to the R3-allocated memory */
-                    pDevExt->MemBalloon.fUseKernelAPI = false;
-                    Log(("VBoxGuestSetBalloonSizeKernel: PhysNC allocs not supported, falling back to R3 allocs.\n"));
-                    break;
-                }
-                if (RT_UNLIKELY(rc == VERR_NO_MEMORY))
-                {
-                    /* cannot allocate more memory => don't try further, just stop here */
-                    break;
-                }
                 if (RT_FAILURE(rc))
                 {
-                    /* XXX what else can fail? */
+                    if (rc == VERR_NOT_SUPPORTED)
+                    {
+                        /* not supported -- fall back to the R3-allocated memory */
+                        pDevExt->MemBalloon.fUseKernelAPI = false;
+                        Assert(pDevExt->MemBalloon.cChunks == 0);
+                        Log(("VBoxGuestSetBalloonSizeKernel: PhysNC allocs not supported, falling back to R3 allocs.\n"));
+                    }
+                    /* else if (rc == VERR_NO_MEMORY): cannot allocate more memory => don't try further, just stop here */
+                    /* else: XXX what else can fail?  VERR_MEMOBJ_INIT_FAILED for instance.   just stop. */
                     break;
                 }
@@ -421 +427 @@
                 {
                     Log(("vboxGuestSetBalloonSize(inflate): failed, rc=%Rrc!\n", rc));
+                    RTR0MemObjFree(pDevExt->MemBalloon.paMemObj[i], true);
+                    pDevExt->MemBalloon.paMemObj[i] = NIL_RTR0MEMOBJ;
                     break;
                 }
@@ -457 +465 @@
 /**
  * Inflate/deflate the balloon by one chunk.
+ *
+ * @returns VBox status code.
+ * @param   pDevExt         The device extension.
+ * @param   pSession        The session.
+ * @param   u64ChunkAddr    The address of the chunk to add to / remove from the
+ *                          balloon.
+ * @param   fInflate        Inflate if true, deflate if false.
  */
 static int vboxGuestSetBalloonSizeFromUser(PVBOXGUESTDEVEXT pDevExt, PVBOXGUESTSESSION pSession,
@@ -468 +483 @@
     if (fInflate)
     {
-        if (pSession->MemBalloon.cChunks > pDevExt->MemBalloon.cMaxChunks - 1)
-        {
-            AssertMsgFailed(("vboxGuestSetBalloonSize: cannot inflate balloon, already have (max=%d)\n",
-                             pSession->MemBalloon.cChunks, pDevExt->MemBalloon.cMaxChunks));
+        if (   pSession->MemBalloon.cChunks > pDevExt->MemBalloon.cMaxChunks - 1
+            || pDevExt->MemBalloon.cMaxChunks == 0 /* If called without first querying. */)
+        {
+            LogRel(("vboxGuestSetBalloonSize: cannot inflate balloon, already have %u chunks (max=%u)\n",
+                    pSession->MemBalloon.cChunks, pDevExt->MemBalloon.cMaxChunks));
             return VERR_INVALID_PARAMETER;
         }
@@ -477 +493 @@
         if (!pSession->MemBalloon.paMemObj)
         {
-            pSession->MemBalloon.paMemObj = (RTR0MEMOBJ*)RTMemAllocZ(sizeof(RTR0MEMOBJ) * pDevExt->MemBalloon.cMaxChunks);
+            pSession->MemBalloon.paMemObj = (PRTR0MEMOBJ)RTMemAlloc(sizeof(RTR0MEMOBJ) * pDevExt->MemBalloon.cMaxChunks);
             if (!pSession->MemBalloon.paMemObj)
             {
@@ -517 +533 @@
         if (fInflate)
             return VERR_NO_MEMORY; /* no free object pointer found -- should not happen */
-        else
-            return VERR_NOT_FOUND; /* cannot free this memory as it wasn't provided before */
+        return VERR_NOT_FOUND; /* cannot free this memory as it wasn't provided before */
     }
 
@@ -525 +540 @@
         return rc;
 
-    do
-    {
-        if (fInflate)
-        {
-            rc = RTR0MemObjLockUser(pMemObj, u64ChunkAddr, VMMDEV_MEMORY_BALLOON_CHUNK_SIZE,
-                                    RTMEM_PROT_READ | RTMEM_PROT_WRITE, NIL_RTR0PROCESS);
+    if (fInflate)
+    {
+        rc = RTR0MemObjLockUser(pMemObj, u64ChunkAddr, VMMDEV_MEMORY_BALLOON_CHUNK_SIZE,
+                                RTMEM_PROT_READ | RTMEM_PROT_WRITE, NIL_RTR0PROCESS);
+        if (RT_SUCCESS(rc))
+        {
+            rc = vboxGuestBalloonInflate(pMemObj, pReq);
             if (RT_SUCCESS(rc))
+                pSession->MemBalloon.cChunks++;
+            else
             {
-                rc = vboxGuestBalloonInflate(pMemObj, pReq);
-                if (RT_FAILURE(rc))
-                {
-                    Log(("vboxGuestSetBalloonSize(inflate): failed, rc=%Rrc!\n", rc));
-                    break;
-                }
-                pSession->MemBalloon.cChunks++;
+                Log(("vboxGuestSetBalloonSize(inflate): failed, rc=%Rrc!\n", rc));
+                RTR0MemObjFree(*pMemObj, true);
+                *pMemObj = NIL_RTR0MEMOBJ;
             }
         }
+    }
+    else
+    {
+        rc = vboxGuestBalloonDeflate(pMemObj, pReq);
+        if (RT_SUCCESS(rc))
+            pSession->MemBalloon.cChunks--;
         else
-        {
-            rc = vboxGuestBalloonDeflate(pMemObj, pReq);
-            if (RT_FAILURE(rc))
-            {
-                Log(("vboxGuestSetBalloonSize(deflate): failed, rc=%Rrc!\n", rc));
-                break;
-            }
-            pSession->MemBalloon.cChunks--;
-        }
-    } while (0);
+            Log(("vboxGuestSetBalloonSize(deflate): failed, rc=%Rrc!\n", rc));
+    }
 
     VbglGRFree(&pReq->header);
@@ -599 +611 @@
     pDevExt->MemBalloon.cChunks = 0;
     pDevExt->MemBalloon.cMaxChunks = 0;
-#if defined(RT_OS_LINUX) || defined(RT_OS_SOLARIS)
     pDevExt->MemBalloon.fUseKernelAPI = true;
-#else
-    pDevExt->MemBalloon.fUseKernelAPI = false;
-#endif
     pDevExt->MemBalloon.paMemObj = NULL;
 }
@@ -1760 +1768 @@
  * @returns VBox status code. Unlike the other HGCM IOCtls this will combine
  *          the VbglHGCMConnect/Disconnect return code with the Info.result.
+ *
+ * @param   pDevExt             The device extension.
+ * @param   pu32ClientId        The client id.
+ * @param   pcbDataReturned     Where to store the amount of returned data. Can
+ *                              be NULL.
  */
 static int VBoxGuestCommonIOCtl_HGCMClipboardReConnect(PVBOXGUESTDEVEXT pDevExt, uint32_t *pu32ClientId, size_t *pcbDataReturned)
@@ -1823 +1836 @@
 
 /**
- * Handle VBOXGUEST_IOCTL_CHECK_BALLOON from R3. Ask the host for the size
- * of the balloon and try to set it accordingly. If this fails, return with
- * VERR_NO_PHYS_MEMORY and userland has to provide the memory.
+ * Handle VBOXGUEST_IOCTL_CHECK_BALLOON from R3.
+ *
+ * Ask the host for the size of the balloon and try to set it accordingly. If
+ * this fails, return with VERR_NO_PHYS_MEMORY and userland has to provide the
+ * memory.
  *
  * @returns VBox status code.
+ *
+ * @param   pDevExt             The device extension.
+ * @param   pInfo               The output buffer.
+ * @param   pcbDataReturned     Where to store the amount of returned data. Can
+ *                              be NULL.
  */
 static int VBoxGuestCommonIOCtl_QueryMemoryBalloon(PVBOXGUESTDEVEXT pDevExt,
@@ -1838 +1858 @@
         return rc;
 
+/** @todo r=bird: The following needs to be serialized as there is no one
+ *        saying mr. evil user cannot start more than one VBoxService. */
+
     /* This is a response to that event. Setting this bit means that we request the value
      * from the host and change the guest memory balloon according to this value. */
@@ -1849 +1872 @@
     }
 
+    Assert(pDevExt->MemBalloon.cMaxChunks == pReq->cPhysMemChunks || pDevExt->MemBalloon.cMaxChunks == 0);
     pDevExt->MemBalloon.cMaxChunks = pReq->cPhysMemChunks;
 
@@ -1874 +1898 @@
 
 /**
- *
+ * Handle a request for changing the memory balloon.
+ *
+ * @returns VBox status code.
+ *
+ * @param   pDevExt             The device extention.
+ * @param   pSession            The session.
+ * @param   pInfo               The change request structure (input).
+ * @param   pcbDataReturned     Where to store the amount of returned data. Can
+ *                              be NULL.
  */
 static int VBoxGuestCommonIOCtl_ChangeMemoryBalloon(PVBOXGUESTDEVEXT pDevExt, PVBOXGUESTSESSION pSession,
                                                     VBoxGuestChangeBalloonInfo *pInfo, size_t *pcbDataReturned)
 {
+/** @todo r=bird: Don't allow this if fUseKernelAPI is true.  */
+/** @todo r=bird: This needs to be serialized as there is no one saying mr. evil
+ *        user cannot start more than one VBoxService.
+ *
+ *        Actually, it would be good to drop the VBOXGUESTSESSION::MemBalloon
+ *        and instead add a VBOXGUESTDEVEXT::pMemBallonOwner
+ *        (PVBOXGUESTSESSION).  Assign balloon ownership on a first come
+ *        basis, releasing it when the session quits.  This could be done
+ *        regardless of the method use for memory allocation. */
     int rc = vboxGuestSetBalloonSizeFromUser(pDevExt, pSession, pInfo->u64ChunkAddr, pInfo->fInflate);
     if (pcbDataReturned)

trunk/src/VBox/Additions/common/VBoxGuest/VBoxGuestInternal.h

@@ -82 +82 @@
 typedef struct VBOXGUESTMEMBALLOON
 {
-    /** The current number of chunks in the balloon */
+    /** The current number of chunks in the balloon. */
     uint32_t                    cChunks;
     /** The maximum number of chunks in the balloon (typically the amount of guest
-     * memory / chunksize) */
+     * memory / chunksize). */
     uint32_t                    cMaxChunks;
     /** This is true if we are using RTR0MemObjAllocPhysNC() / RTR0MemObjGetPagePhysAddr()
-     * and false otherwise */
+     * and false otherwise. */
     bool                        fUseKernelAPI;
-    /* The pointer to the array of memory objects holding the chunks of the balloon */
-    RTR0MEMOBJ                 *paMemObj;
+    /** The pointer to the array of memory objects holding the chunks of the
+     *  balloon.  This array is cMaxChunks in size when present. */
+    PRTR0MEMOBJ                 paMemObj;
 } VBOXGUESTMEMBALLOON;
+/** Pointer to a memory balloon. */
 typedef VBOXGUESTMEMBALLOON *PVBOXGUESTMEMBALLOON;
 
@@ -140 +142 @@
     uint32_t                    u32ClipboardClientId;
 
-    /* Memory balloon information for RTR0MemObjAllocPhysNC(). */
+    /** Memory balloon information for RTR0MemObjAllocPhysNC(). */
     VBOXGUESTMEMBALLOON         MemBalloon;