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...

[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, [e-mail] 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
> > > [e-mail]
> > > http://www.reactos.org/mailman/listinfo/ros-dev
> >
> > Ros-dev mailing list
> > [e-mail]
> > http://www.reactos.org/mailman/listinfo/ros-dev
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev