Changeset 31093 in vbox for trunk/src/VBox/VMM/VMMAll

Timestamp:

Jul 26, 2010 8:02:01 AM (15 years ago)

Author:

vboxsync

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

64045

Message:

PGM: More #PF cleanup, eliminating a few PGMGstGetPage() calls that are unnecessary now.

File:

• trunk/src/VBox/VMM/VMMAll/PGMAllBth.h (modified) (10 diffs)

trunk/src/VBox/VMM/VMMAll/PGMAllBth.h

@@ -610 +610 @@
             {
                /* Check by physical address. */
-                unsigned        iPage;
-                rc = pgmHandlerVirtualFindByPhysAddr(pVM, GCPhys + (pvFault & PAGE_OFFSET_MASK),
-                                                     &pCur, &iPage);
+                unsigned iPage;
+                rc = pgmHandlerVirtualFindByPhysAddr(pVM, GCPhys + (pvFault & PAGE_OFFSET_MASK), &pCur, &iPage);
                 Assert(RT_SUCCESS(rc) || !pCur);
                 if (    pCur
@@ -734 +733 @@
         /*
          * Page is not present in our page tables. Try to sync it!
-         * BTW, fPageShw is invalid in this branch!
          */
         if (uErr & X86_TRAP_PF_US)
@@ -768 +766 @@
 
 #   if PGM_WITH_PAGING(PGM_GST_TYPE, PGM_SHW_TYPE) && !defined(IN_RING0)
-        if (CPUMGetGuestCPL(pVCpu, pRegFrame) == 0)
-        {
-            /** @todo It's not necessary to repeat this here, GstWalk has
-             *        all the information. */
-            uint64_t fPageGst;
-            rc = PGMGstGetPage(pVCpu, pvFault, &fPageGst, NULL);
-            if (    RT_SUCCESS(rc)
-                && !(fPageGst & X86_PTE_US))
+        if (   !GstWalk.Core.fEffectiveUS
+            && CPUMGetGuestCPL(pVCpu, pRegFrame) == 0)
+        {
+            /* Note: Can't check for X86_TRAP_ID bit, because that requires execute disable support on the CPU. */
+            if (    pvFault == (RTGCPTR)pRegFrame->eip
+                ||  pvFault - pRegFrame->eip < 8    /* instruction crossing a page boundary */
+#    ifdef CSAM_DETECT_NEW_CODE_PAGES
+                ||  (   !PATMIsPatchGCAddr(pVM, pRegFrame->eip)
+                     && CSAMDoesPageNeedScanning(pVM, pRegFrame->eip))   /* any new code we encounter here */
+#    endif /* CSAM_DETECT_NEW_CODE_PAGES */
+               )
             {
-                /* Note: Can't check for X86_TRAP_ID bit, because that requires execute disable support on the CPU. */
-                if (    pvFault == (RTGCPTR)pRegFrame->eip
-                    ||  pvFault - pRegFrame->eip < 8    /* instruction crossing a page boundary */
+                LogFlow(("CSAMExecFault %RX32\n", pRegFrame->eip));
+                rc = CSAMExecFault(pVM, (RTRCPTR)pRegFrame->eip);
+                if (rc != VINF_SUCCESS)
+                {
+                    /*
+                     * CSAM needs to perform a job in ring 3.
+                     *
+                     * Sync the page before going to the host context; otherwise we'll end up in a loop if
+                     * CSAM fails (e.g. instruction crosses a page boundary and the next page is not present)
+                     */
+                    LogFlow(("CSAM ring 3 job\n"));
+                    int rc2 = PGM_BTH_NAME(SyncPage)(pVCpu, GstWalk.Pde, pvFault, 1, uErr);
+                    AssertRC(rc2);
+
+                    STAM_PROFILE_STOP(&pVCpu->pgm.s.StatRZTrap0eTimeOutOfSync, c);
+                    STAM_STATS({ pVCpu->pgm.s.CTX_SUFF(pStatTrap0eAttribution) = &pVCpu->pgm.s.StatRZTrap0eTime2CSAM; });
+                    return rc;
+                }
+            }
 #    ifdef CSAM_DETECT_NEW_CODE_PAGES
-                    ||  (   !PATMIsPatchGCAddr(pVM, pRegFrame->eip)
-                         && CSAMDoesPageNeedScanning(pVM, pRegFrame->eip))   /* any new code we encounter here */
-#    endif /* CSAM_DETECT_NEW_CODE_PAGES */
-                   )
+            else if (    uErr == X86_TRAP_PF_RW
+                     &&  pRegFrame->ecx >= 0x100         /* early check for movswd count */
+                     &&  pRegFrame->ecx < 0x10000)
+            {
+                /* In case of a write to a non-present supervisor shadow page, we'll take special precautions
+                 * to detect loading of new code pages.
+                 */
+
+                /*
+                 * Decode the instruction.
+                 */
+                RTGCPTR PC;
+                rc = SELMValidateAndConvertCSAddr(pVM, pRegFrame->eflags, pRegFrame->ss, pRegFrame->cs,
+                                                  &pRegFrame->csHid, (RTGCPTR)pRegFrame->eip, &PC);
+                if (rc == VINF_SUCCESS)
                 {
-                    LogFlow(("CSAMExecFault %RX32\n", pRegFrame->eip));
-                    rc = CSAMExecFault(pVM, (RTRCPTR)pRegFrame->eip);
-                    if (rc != VINF_SUCCESS)
+                    PDISCPUSTATE pDis = &pVCpu->pgm.s.DisState;
+                    uint32_t     cbOp;
+                    rc = EMInterpretDisasOneEx(pVM, pVCpu, PC, pRegFrame, pDis, &cbOp);
+
+                    /* For now we'll restrict this to rep movsw/d instructions */
+                    if (    rc == VINF_SUCCESS
+                        &&  pDis->pCurInstr->opcode == OP_MOVSWD
+                        &&  (pDis->prefix & PREFIX_REP))
                     {
-                        /*
-                         * CSAM needs to perform a job in ring 3.
-                         *
-                         * Sync the page before going to the host context; otherwise we'll end up in a loop if
-                         * CSAM fails (e.g. instruction crosses a page boundary and the next page is not present)
-                         */
-                        LogFlow(("CSAM ring 3 job\n"));
-                        int rc2 = PGM_BTH_NAME(SyncPage)(pVCpu, GstWalk.Pde, pvFault, 1, uErr);
-                        AssertRC(rc2);
-
-                        STAM_PROFILE_STOP(&pVCpu->pgm.s.StatRZTrap0eTimeOutOfSync, c);
-                        STAM_STATS({ pVCpu->pgm.s.CTX_SUFF(pStatTrap0eAttribution) = &pVCpu->pgm.s.StatRZTrap0eTime2CSAM; });
-                        return rc;
+                        CSAMMarkPossibleCodePage(pVM, pvFault);
                     }
                 }
-#    ifdef CSAM_DETECT_NEW_CODE_PAGES
-                else if (    uErr == X86_TRAP_PF_RW
-                         &&  pRegFrame->ecx >= 0x100         /* early check for movswd count */
-                         &&  pRegFrame->ecx < 0x10000)
-                {
-                    /* In case of a write to a non-present supervisor shadow page, we'll take special precautions
-                     * to detect loading of new code pages.
-                     */
-
-                    /*
-                     * Decode the instruction.
-                     */
-                    RTGCPTR PC;
-                    rc = SELMValidateAndConvertCSAddr(pVM, pRegFrame->eflags, pRegFrame->ss, pRegFrame->cs,
-                                                      &pRegFrame->csHid, (RTGCPTR)pRegFrame->eip, &PC);
-                    if (rc == VINF_SUCCESS)
-                    {
-                        PDISCPUSTATE pDis = &pVCpu->pgm.s.DisState;
-                        uint32_t     cbOp;
-                        rc = EMInterpretDisasOneEx(pVM, pVCpu, PC, pRegFrame, pDis, &cbOp);
-
-                        /* For now we'll restrict this to rep movsw/d instructions */
-                        if (    rc == VINF_SUCCESS
-                            &&  pDis->pCurInstr->opcode == OP_MOVSWD
-                            &&  (pDis->prefix & PREFIX_REP))
-                        {
-                            CSAMMarkPossibleCodePage(pVM, pvFault);
-                        }
-                    }
-                }
+            }
 #    endif  /* CSAM_DETECT_NEW_CODE_PAGES */
 
-                /*
-                 * Mark this page as safe.
-                 */
-                /** @todo not correct for pages that contain both code and data!! */
-                Log2(("CSAMMarkPage %RGv; scanned=%d\n", pvFault, true));
-                CSAMMarkPage(pVM, pvFault, true);
-            }
+            /*
+             * Mark this page as safe.
+             */
+            /** @todo not correct for pages that contain both code and data!! */
+            Log2(("CSAMMarkPage %RGv; scanned=%d\n", pvFault, true));
+            CSAMMarkPage(pVM, pvFault, true);
         }
 #   endif /* PGM_WITH_PAGING(PGM_GST_TYPE, PGM_SHW_TYPE) && !defined(IN_RING0) */
@@ -862 +853 @@
     {
         /*
-         * Write protected pages are make writable when the guest makes the first
-         * write to it. This happens for pages that are shared, write monitored
-         * and not yet allocated.
+         * Write protected pages are made writable when the guest makes the
+         * first write to it.  This happens for pages that are shared, write
+         * monitored or not yet allocated.
          *
-         * Also, a side effect of not flushing global PDEs are out of sync pages due
-         * to physical monitored regions, that are no longer valid.
+         * We may also end up here when CR0.WP=0 in the guest.
+         *
+         * Also, a side effect of not flushing global PDEs are out of sync
+         * pages due to physical monitored regions, that are no longer valid.
          * Assume for now it only applies to the read/write flag.
          */
-        if (    RT_SUCCESS(rc)
-            &&  (uErr & X86_TRAP_PF_RW))
-        {
+        if (uErr & X86_TRAP_PF_RW)
+        {
+            /*
+             * Check if it is a read-only page.
+             */
             if (PGM_PAGE_GET_STATE(pPage) != PGM_PAGE_STATE_ALLOCATED)
             {
@@ -883 +878 @@
                 {
                     AssertMsg(rc == VINF_PGM_SYNC_CR3 || RT_FAILURE(rc), ("%Rrc\n", rc));
+                    STAM_PROFILE_STOP(&pVCpu->pgm.s.StatRZTrap0eTimeOutOfSync, c);
                     return rc;
                 }
@@ -890 +886 @@
 
 #   if PGM_WITH_PAGING(PGM_GST_TYPE, PGM_SHW_TYPE)
-            /* Check to see if we need to emulate the instruction as X86_CR0_WP has been cleared. */
-            if (    CPUMGetGuestCPL(pVCpu, pRegFrame) == 0
-                &&  (CPUMGetGuestCR0(pVCpu) & (X86_CR0_WP | X86_CR0_PG)) == X86_CR0_PG)
+            /*
+             * Check to see if we need to emulate the instruction if CR0.WP=0.
+             */
+            if (    !GstWalk.Core.fEffectiveRW
+                &&  (CPUMGetGuestCR0(pVCpu) & (X86_CR0_WP | X86_CR0_PG)) == X86_CR0_PG
+                &&  CPUMGetGuestCPL(pVCpu, pRegFrame) == 0)
             {
                 Assert((uErr & (X86_TRAP_PF_RW | X86_TRAP_PF_P)) == (X86_TRAP_PF_RW | X86_TRAP_PF_P));
-                /** @todo It's not necessary to repeat this here, GstWalk has
-                 *        all the information. */
-                uint64_t fPageGst;
-                rc = PGMGstGetPage(pVCpu, pvFault, &fPageGst, NULL);
-                if (    RT_SUCCESS(rc)
-                    && !(fPageGst & X86_PTE_RW))
-                {
-                    rc = PGMInterpretInstruction(pVM, pVCpu, pRegFrame, pvFault);
-                    if (RT_SUCCESS(rc))
-                        STAM_COUNTER_INC(&pVCpu->pgm.s.StatRZTrap0eWPEmulInRZ);
-                    else
-                        STAM_COUNTER_INC(&pVCpu->pgm.s.StatRZTrap0eWPEmulToR3);
-                    return rc;
-                }
-                AssertMsg(RT_SUCCESS(rc), ("Unexpected r/w page %RGv flag=%x rc=%Rrc\n", pvFault, (uint32_t)fPageGst, rc));
+                rc = PGMInterpretInstruction(pVM, pVCpu, pRegFrame, pvFault);
+                if (RT_SUCCESS(rc))
+                    STAM_COUNTER_INC(&pVCpu->pgm.s.StatRZTrap0eWPEmulInRZ);
+                else
+                    STAM_COUNTER_INC(&pVCpu->pgm.s.StatRZTrap0eWPEmulToR3);
+                STAM_PROFILE_STOP(&pVCpu->pgm.s.StatRZTrap0eTimeOutOfSync, c);
+                return rc;
             }
 #   endif
@@ -919 +910 @@
 
             /*
+             * Sync the page.
+             *
              * Note: Do NOT use PGM_SYNC_NR_PAGES here. That only works if the
              *       page is not present, which is not true in this case.
@@ -930 +923 @@
             {
                /*
-                * Page was successfully synced, return to guest.
-                * First invalidate the page as it might be in the TLB.
+                * Page was successfully synced, return to guest but invalidate
+                * the TLB first as the page is very likely to be in it.
                 */
 #   if PGM_SHW_TYPE == PGM_TYPE_EPT
@@ -956 +949 @@
             }
         }
-
-#   if PGM_WITH_PAGING(PGM_GST_TYPE, PGM_SHW_TYPE)
-#    ifdef VBOX_STRICT
+        /** @todo else: WTF are we here? */
+
+#   if PGM_WITH_PAGING(PGM_GST_TYPE, PGM_SHW_TYPE) && defined(VBOX_STRICT)
         /*
          * Check for VMM page flags vs. Guest page flags consistency.
@@ -985 +978 @@
         else
             AssertMsgFailed(("PGMGCGetPage rc=%Rrc\n", rc));
-#    endif /* VBOX_STRICT */
-#   endif /* PGM_WITH_PAGING(PGM_GST_TYPE, PGM_SHW_TYPE) */
+#   endif /* PGM_WITH_PAGING(PGM_GST_TYPE, PGM_SHW_TYPE) && VBOX_STRICT */
     }
     STAM_PROFILE_STOP(&pVCpu->pgm.s.StatRZTrap0eTimeOutOfSync, c);
 
-    /** @todo This point is never really reached, is it? */
+    /** @todo This point is only ever reached when something goes awry.  The
+     *        conclusion here is wrong, it is not a guest trap!  Will fix in
+     *        a bit... */
 
 #  if PGM_WITH_PAGING(PGM_GST_TYPE, PGM_SHW_TYPE)