Re: [ros-dev] [ros-diffs] [sginsberg] 69528: [NTOS] Add super-complicated handling of global pages to KeFlushCurrentTb (pretty much the same code which has been in HalpFlushTLB for the past ~6 years). This should be all tha...

[Alexander Andrejevic]

If I recall correctly, one of the commits that followed this one also added
ValidKernelPteLocal and ValidKernelPdeLocal, which are used for PTEs/PDEs
that shouldn't be global.

Regards,
Alex

On Thu, Oct 15, 2015 at 06:55:51PM -0700, Alex Ionescu wrote:
>   This is wrong, wrong, wrong.
>  
>  1) The feature needs to be enabled on all processors, not just one. There's
>  a reason for that loop.
>  
>  2) ValidKernelPte/Pde being marked global will make ALL kernel pages
>  global. This is probably not what you want.
>  
>  Best regards,
>  Alex Ionescu
>  
>  On Wed, Oct 14, 2015 at 12:33 PM, <sginsberg(a)svn.reactos.org> wrote:
>  
>  > Author: sginsberg
>  > Date: Wed Oct 14 19:33:35 2015
>  > New Revision: 69528
>  >
>  > URL: http://svn.reactos.org/svn/reactos?rev=69528&view=rev
>  > Log:
>  > [NTOS]
>  > Add super-complicated handling of global pages to KeFlushCurrentTb (pretty
>  > much the same code which has been in HalpFlushTLB for the past ~6 years).
>  > This should be all that is required to make this feature work (everything
>  > else being in place already), and *seems* to work fine but is disabled
>  > under a switch until tested thoroughly.
>  >
>  > Global pages, an important optimization that allows for not flushing the
>  > whole x86 TLB every time CR3 is changed (typically on context switch to a
>  > new process, or during process attach/detach), relies on us doing extra
>  > work whenever we do alter a global page. This is likely where any bugs will
>  > have to be flushed out!
>  >
>  > Fixup Ki386EnableGlobalPage while we are at it -- disable/restore
>  > interrupts properly, and verify PGE-bit isn't set (nothing should have
>  > touched it before this routine, which is responsible for initializing it,
>  > so we shouldn't have to disable it). Fix, but disable, the CPU-sync spin as
>  > well as there should be no particular reason to do this for PGE-enabling
>  > during initialization (no other processor will be messing with PTEs at this
>  > stage, as compared to a call to KeFlushEntireTb).
>  >
>  > Everyone, repeat after me: Global pages are awesome!
>  >
>  > Modified:
>  >     trunk/reactos/ntoskrnl/include/ntoskrnl.h
>  >     trunk/reactos/ntoskrnl/ke/i386/cpu.c
>  >     trunk/reactos/ntoskrnl/ke/i386/patpge.c
>  >     trunk/reactos/ntoskrnl/mm/ARM3/i386/init.c
>  >
>  > Modified: trunk/reactos/ntoskrnl/include/ntoskrnl.h
>  > URL:
>  >
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/include/ntoskrnl.…
>  >
>  > ==============================================================================
>  > --- trunk/reactos/ntoskrnl/include/ntoskrnl.h   [iso-8859-1] (original)
>  > +++ trunk/reactos/ntoskrnl/include/ntoskrnl.h   [iso-8859-1] Wed Oct 14
>  > 19:33:35 2015
>  > @@ -98,6 +98,14 @@
>  >  #define ASSERT NT_ASSERT
>  >  #endif
>  >
>  > +
>  > +//
>  > +// Switch for enabling global page support
>  > +//
>  > +
>  > +//#define _GLOBAL_PAGES_ARE_AWESOME_
>  > +
>  > +
>  >  /* Internal Headers */
>  >  #include "internal/ntoskrnl.h"
>  >  #include "config.h"
>  >
>  > Modified: trunk/reactos/ntoskrnl/ke/i386/cpu.c
>  > URL:
>  >
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ke/i386/cpu.c?rev…
>  >
>  > ==============================================================================
>  > --- trunk/reactos/ntoskrnl/ke/i386/cpu.c        [iso-8859-1] (original)
>  > +++ trunk/reactos/ntoskrnl/ke/i386/cpu.c        [iso-8859-1] Wed Oct 14
>  > 19:33:35 2015
>  > @@ -878,8 +878,37 @@
>  >  NTAPI
>  >  KeFlushCurrentTb(VOID)
>  >  {
>  > +
>  > +#if !defined(_GLOBAL_PAGES_ARE_AWESOME_)
>  > +
>  >      /* Flush the TLB by resetting CR3 */
>  >      __writecr3(__readcr3());
>  > +
>  > +#else
>  > +
>  > +    /* Check if global pages are enabled */
>  > +    if (KeFeatureBits & KF_GLOBAL_PAGE)
>  > +    {
>  > +        ULONG Cr4;
>  > +
>  > +        /* Disable PGE */
>  > +        Cr4 = __readcr4() & ~CR4_PGE;
>  > +        __writecr4(Cr4);
>  > +
>  > +        /* Flush everything */
>  > +        __writecr3(__readcr3());
>  > +
>  > +        /* Re-enable PGE */
>  > +        __writecr4(Cr4 | CR4_PGE);
>  > +    }
>  > +    else
>  > +    {
>  > +        /* No global pages, resetting CR3 is enough */
>  > +        __writecr3(__readcr3());
>  > +    }
>  > +
>  > +#endif
>  > +
>  >  }
>  >
>  >  VOID
>  >
>  > Modified: trunk/reactos/ntoskrnl/ke/i386/patpge.c
>  > URL:
>  >
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ke/i386/patpge.c?…
>  >
>  > ==============================================================================
>  > --- trunk/reactos/ntoskrnl/ke/i386/patpge.c     [iso-8859-1] (original)
>  > +++ trunk/reactos/ntoskrnl/ke/i386/patpge.c     [iso-8859-1] Wed Oct 14
>  > 19:33:35 2015
>  > @@ -17,40 +17,41 @@
>  >
>  >  /* FUNCTIONS
>  > *****************************************************************/
>  >
>  > +INIT_SECTION
>  >  ULONG_PTR
>  >  NTAPI
>  > -INIT_FUNCTION
>  >  Ki386EnableGlobalPage(IN ULONG_PTR Context)
>  >  {
>  > -    PLONG Count = (PLONG)Context;
>  > -    ULONG Cr4, Cr3;
>  > +    //PLONG Count;
>  > +#if defined(_GLOBAL_PAGES_ARE_AWESOME_)
>  > +    ULONG Cr4;
>  > +#endif
>  > +    BOOLEAN Enable;
>  >
>  >      /* Disable interrupts */
>  > -    _disable();
>  > -
>  > -    /* Decrease CPU Count and loop until it's reached 0 */
>  > -    do {InterlockedDecrement(Count);} while (!*Count);
>  > -
>  > -    /* Now check if this is the Boot CPU */
>  > -    if (!KeGetPcr()->Number)
>  > -    {
>  > -        /* It is.FIXME: Patch KeFlushCurrentTb */
>  > -    }
>  > -
>  > -    /* Now get CR4 and make sure PGE is masked out */
>  > +    Enable = KeDisableInterrupts();
>  > +
>  > +    /* Spin until other processors are ready */
>  > +    //Count = (PLONG)Context;
>  > +    //InterlockedDecrement(Count);
>  > +    //while (*Count) YieldProcessor();
>  > +
>  > +#if defined(_GLOBAL_PAGES_ARE_AWESOME_)
>  > +
>  > +    /* Get CR4 and ensure global pages are disabled */
>  >      Cr4 = __readcr4();
>  > -    __writecr4(Cr4 & ~CR4_PGE);
>  > -
>  > -    /* Flush the TLB */
>  > -    Cr3 = __readcr3();
>  > -    __writecr3(Cr3);
>  > +    ASSERT(!(Cr4 & CR4_PGE));
>  > +
>  > +    /* Reset CR3 to flush the TLB */
>  > +    __writecr3(__readcr3());
>  >
>  >      /* Now enable PGE */
>  > -    DPRINT("Global page support detected but not yet taken advantage
>  > of\n");
>  > -    //__writecr4(Cr4 | CR4_PGE);
>  > -
>  > -    /* Restore interrupts */
>  > -    _enable();
>  > +    __writecr4(Cr4 | CR4_PGE);
>  > +
>  > +#endif
>  > +
>  > +    /* Restore interrupts and return */
>  > +    KeRestoreInterrupts(Enable);
>  >      return 0;
>  >  }
>  >
>  >
>  > Modified: trunk/reactos/ntoskrnl/mm/ARM3/i386/init.c
>  > URL:
>  >
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/i386/init…
>  >
>  > ==============================================================================
>  > --- trunk/reactos/ntoskrnl/mm/ARM3/i386/init.c  [iso-8859-1] (original)
>  > +++ trunk/reactos/ntoskrnl/mm/ARM3/i386/init.c  [iso-8859-1] Wed Oct 14
>  > 19:33:35 2015
>  > @@ -249,15 +249,18 @@
>  >      PMMPFN Pfn1;
>  >      ULONG Flags;
>  >
>  > +#if defined(_GLOBAL_PAGES_ARE_AWESOME_)
>  > +
>  >      /* Check for global bit */
>  > -#if 0
>  >      if (KeFeatureBits & KF_GLOBAL_PAGE)
>  >      {
>  >          /* Set it on the template PTE and PDE */
>  >          ValidKernelPte.u.Hard.Global = TRUE;
>  >          ValidKernelPde.u.Hard.Global = TRUE;
>  >      }
>  > +
>  >  #endif
>  > +
>  >      /* Now templates are ready */
>  >      TempPte = ValidKernelPte;
>  >      TempPde = ValidKernelPde;
>  >
>  >
>  > 
>   _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev 

-- 
Alexander Andrejevic <theflash(a)sdf.lonestar.org>
SDF Public Access UNIX System - http://sdf.lonestar.org