Re: [ros-dev] [ros-diffs] [jgardou] 66334: [NTOSKRNL/MM] - MiIsEntireRangeCommitted: Ensure the PTE we are checking is really faulted in. - Prefer MiPteToPde and MiPdeToPte (which should really be called MiFirstPteInPde) in...
19 Feb
2015
19 Feb
'15
4:46 a.m.
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(a)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…
==============================================================================
--- 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;
}
}
19 Feb
19 Feb
10:19 a.m.
New subject: [ros-diffs] [jgardou] 66334: [NTOSKRNL/MM] - MiIsEntireRangeCommitted: Ensure the PTE we are checking is really faulted in. - Prefer MiPteToPde and MiPdeToPte (which should really be called MiFirstPteInPde) in...
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(a)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…
==============================================================================
--- 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(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev
20 Feb
20 Feb
9 p.m.
New subject: [ros-diffs] [jgardou] 66334: [NTOSKRNL/MM] - MiIsEntireRangeCommitted: Ensure the PTE we are checking is really faulted in. - Prefer MiPteToPde and MiPdeToPte (which should really be called MiFirstPteInPde) in...
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(a)svn.reactos.org> wrote:
_______________________________________________
Ros-dev mailing list
Ros-dev(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev
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…
==============================================================================
--- 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(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev
21 Feb
21 Feb
3:41 p.m.
New subject: [ros-diffs] [jgardou] 66334: [NTOSKRNL/MM] - MiIsEntireRangeCommitted: Ensure the PTE we are checking is really faulted in. - Prefer MiPteToPde and MiPdeToPte (which should really be called MiFirstPteInPde) in...
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 :
_______________________________________________
Ros-dev mailing list
Ros-dev(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev 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(a)svn.reactos.org> wrote:
_______________________________________________
Ros-dev mailing list
Ros-dev(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev
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…
==============================================================================
--- 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(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev
4:25 p.m.
New subject: [ros-diffs] [jgardou] 66334: [NTOSKRNL/MM] - MiIsEntireRangeCommitted: Ensure the PTE we are checking is really faulted in. - Prefer MiPteToPde and MiPdeToPte (which should really be called MiFirstPteInPde) in...
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:
_______________________________________________
Ros-dev mailing list
Ros-dev(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev 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 :
_______________________________________________
Ros-dev mailing list
Ros-dev(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev 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(a)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…
>
>
> ==============================================================================
>
> --- 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(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev
_______________________________________________
Ros-dev mailing list
Ros-dev(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev
3548
days inactive
3550
days old
4 comments
4 participants
participants (4)
-
Alex Ionescu -
Jerome Gardou -
Jérôme Gardou
-
Timo Kreuzer