I was thinking that a VAD with the Committed flag set would be entirely committed, so we could avoid all PTE checking. But I guess the flag stays set, even if we decommit ranges inside it. In that case it's clear that we need to check for decommitted pages.

Regarding your change, I see the issue now (I was misinterpreting it before)
Your fix looks correct to me.

Timo

Am 21.02.2015 um 16:41 schrieb Jerome Gardou:

It is possible that the entire VAD range is not committed, so we have to sweep over all the PTEs in the sub-range to check. If one of the PTEs is zero AND the entire VAD is not marked as committed, then we're sure that the range is not committed. Also, if the PTE is marked as decommitted, same thing happen.

So, to check the PTEs, we have to make sure the underlying PDE is there (== not paged out). Otherwise, by accessing a PTE, the CPU might raise a page fault while we're holding the process working set lock --> failed ASSERT.
Notice that if the PDE is zero, it's not faulted in and we check for the VAD commit flag. (like we do for the PTEs later in the loop iteration)

If I misunderstood something, I'd be happy to be given some hint as why this patch is incorrect.

Regards
Jérôme

Le 20/02/2015 22:00, Timo Kreuzer a écrit :
Why does the PDE need to be faulted in? The function is called MiIsEntireRangeCommitted, not MiCheckIfEntireRangeIsCommittedAndMakePdeValid.
I also wonder why one would even walk the PTEs if the VAD is MEM_COMMIT.

Am 19.02.2015 um 11:19 schrieb Jérôme Gardou:

If it takes me 6 months to understand why, then yes.

Hint: you can make this delay shorter.

Also the title is a bit misleading, it's the PDE that is not faulted in
in case we don't terminate the loop iteration early.

Le 19/02/2015 05:46, Alex Ionescu a écrit :

it might fix an assert but the patch is incorrect. will this also take 6
months to revert?

Best regards,
Alex Ionescu

On Tue, Feb 17, 2015 at 6:19 AM, <jgardou@svn.reactos.org> wrote:

Author: jgardou
Date: Tue Feb 17 14:19:05 2015
New Revision: 66334

URL: http://svn.reactos.org/svn/reactos?rev=66334&view=rev
Log:
[NTOSKRNL/MM]
- MiIsEntireRangeCommitted: Ensure the PTE we are checking is really
faulted in.
- Prefer MiPteToPde and MiPdeToPte (which should really be called
MiFirstPteInPde) instead of MiAddressToPte and MiPteToAddress
Fixes weird failed ASSERT in page fault handler when using DPH.

Modified:
     trunk/reactos/ntoskrnl/mm/ARM3/virtual.c

Modified: trunk/reactos/ntoskrnl/mm/ARM3/virtual.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/virtual.c?rev=66334&r1=66333&r2=66334&view=diff

==============================================================================
--- trunk/reactos/ntoskrnl/mm/ARM3/virtual.c    [iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/mm/ARM3/virtual.c    [iso-8859-1] Tue Feb 17
14:19:05 2015
@@ -1994,14 +1994,13 @@
          if (OnBoundary)
          {
              /* Is this PDE demand zero? */
-            PointerPde = MiAddressToPte(PointerPte);
+            PointerPde = MiPteToPde(PointerPte);
              if (PointerPde->u.Long != 0)
              {
                  /* It isn't -- is it valid? */
                  if (PointerPde->u.Hard.Valid == 0)
                  {
                      /* Nope, fault it in */
-                    PointerPte = MiPteToAddress(PointerPde);
                      MiMakeSystemAddressValid(PointerPte, Process);
                  }
              }
@@ -2009,13 +2008,13 @@
              {
                  /* The PTE was already valid, so move to the next one */
                  PointerPde++;
-                PointerPte = MiPteToAddress(PointerPde);
+                PointerPte = MiPdeToPte(PointerPde);

                  /* Is the entire VAD committed? If not, fail */
                  if (!Vad->u.VadFlags.MemCommit) return FALSE;

-                /* Everything is committed so far past the range, return
true */
-                if (PointerPte > LastPte) return TRUE;
+                /* New loop iteration with our new, on-boundary PTE. */
+                continue;
              }
          }

_______________________________________________
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev

_______________________________________________
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev
_______________________________________________
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev
_______________________________________________
Ros-dev mailing list
Ros-dev@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev