Re: [ros-dev] [ros-diffs] [tkreuzer] 35518: Implement KiSystemStartup in C instead of asm, implement KiSwapStack intrinsic, get rid of KiSetupStackandInitializeKernel, instead do the work in KiSystemStartupReal in C. Move Stack definitions into trap.S and get rid of the whole boot.S file. Comment out the sync loop for the moment and add a comment why it doesn't work (InterlockedBitTestAndSet64 doesn't work correctly).

[Thomas Bluemel]

Calling from KiSwapStack from C seems quite scary. C doesn't know 
anything about you messing with the stack, so any subsequent code may 
still refer to a now invalid stack. There's a reason this was 
implemented in asm...

tkreuzer(a)svn.reactos.org wrote:
>   Author: tkreuzer
>  Date: Thu Aug 21 16:16:57 2008
>  New Revision: 35518
>
>  URL: http://svn.reactos.org/svn/reactos?rev=35518&view=rev
>  Log:
>  Implement KiSystemStartup in C instead of asm, implement KiSwapStack intrinsic, get rid
> of KiSetupStackandInitializeKernel, instead do the work in KiSystemStartupReal in C. Move
> Stack definitions into trap.S and get rid of the whole boot.S file. Comment out the sync
> loop for the moment and add a comment why it doesn't work (InterlockedBitTestAndSet64
> doesn't work correctly).
>
>  Removed:
>      branches/ros-amd64-bringup/reactos/ntoskrnl/ke/amd64/boot.S
>  Modified:
>      branches/ros-amd64-bringup/reactos/ntoskrnl/include/internal/amd64/intrin_i.h
>      branches/ros-amd64-bringup/reactos/ntoskrnl/ke/amd64/kiinit.c
>      branches/ros-amd64-bringup/reactos/ntoskrnl/ke/amd64/trap.S
>      branches/ros-amd64-bringup/reactos/ntoskrnl/ntoskrnl-amd64hack.rbuild
>
>  Modified: branches/ros-amd64-bringup/reactos/ntoskrnl/include/internal/amd64/intrin_i.h
>  URL:
> http://svn.reactos.org/svn/reactos/branches/ros-amd64-bringup/reactos/ntosk…
>  ==============================================================================
>  --- branches/ros-amd64-bringup/reactos/ntoskrnl/include/internal/amd64/intrin_i.h
> [iso-8859-1] (original)
>  +++ branches/ros-amd64-bringup/reactos/ntoskrnl/include/internal/amd64/intrin_i.h
> [iso-8859-1] Thu Aug 21 16:16:57 2008
>  @@ -10,6 +10,17 @@
>   #define KeSetCurrentIrql(x) __writecr8(x)
>   
>   #if defined(__GNUC__)
>  +
>  +ULONG64
>  +FORCEINLINE
>  +KiSwapStack(ULONG64 NewStack)
>  +{
>  +    ULONG64 OldStack;
>  +    asm volatile ("movq %%rsp, %[oldstack]\n movq %[newstack], %%rsp\n" 
>  +    : [oldstack] "=rm" (OldStack)
>  +    : [newstack] "rm" (NewStack));
>  +    return OldStack;
>  +}
>   
>   #define Ke386SetInterruptDescriptorTable(X) \
>       __asm__("lidt %0\n\t" \
>
>  Removed: branches/ros-amd64-bringup/reactos/ntoskrnl/ke/amd64/boot.S
>  URL:
> http://svn.reactos.org/svn/reactos/branches/ros-amd64-bringup/reactos/ntosk…
>  ==============================================================================
>  --- branches/ros-amd64-bringup/reactos/ntoskrnl/ke/amd64/boot.S [iso-8859-1] (original)
>  +++ branches/ros-amd64-bringup/reactos/ntoskrnl/ke/amd64/boot.S (removed)
>  @@ -1,86 +1,0 @@
>  -/*
>  - * FILE:            ntoskrnl/ke/i386/boot.S
>  - * COPYRIGHT:       See COPYING in the top level directory
>  - * PURPOSE:         FreeLDR Wrapper Bootstrap Code and Bootstrap Trampoline
>  - * PROGRAMMERs:     Alex Ionescu (alex(a)relsoft.net)
>  - *                  Thomas Weidenmueller <w3seek(a)reactos.org>
>  - */
>  -
>  -/* INCLUDES ******************************************************************/
>  -
>  -#include <asm.h>
>  -.intel_syntax noprefix
>  -.code64
>  -
>  -/* GLOBALS *******************************************************************/
>  -
>  -.bss
>  -.align 16
>  -
>  -/* Kernel Boot Stack */
>  -.globl _P0BootStack
>  -.space KERNEL_STACK_SIZE
>  -_P0BootStack:
>  -
>  -/* Kernel Double-Fault and Temporary DPC Stack */
>  -.globl _KiDoubleFaultStack
>  -.space KERNEL_STACK_SIZE
>  -_KiDoubleFaultStack:
>  -
>  -/* FUNCTIONS *****************************************************************/
>  -.global _KiSystemStartup
>  -.text
>  -.func KiSystemStartup
>  -_KiSystemStartup:
>  -
>  -    /* NTLDR Boot: Call the main kernel initialization */
>  -    test rcx, 0x80000000
>  -    jnz _KiSystemStartupReal
>  -
>  -    /* FREELDR Boot: Cal the FreeLDR wrapper */
>  -    jmp _KiRosPrepareForSystemStartup
>  -.endfunc
>  -
>  -/**
>  - * VOID
>  - * KiSetupStackAndInitializeKernel(
>  - *             esp+4 = ? -> rcx
>  - *             esp+8 = ? -> rdx
>  - *        PVOID pNewstack     // esp+12 = new stack -> r8
>  - *               esp+16 -> r9
>  - *               esp+20 -> rsp + 8
>  - *               esp+24 -> rsp + 16?
>  - */
>  -.globl _KiSetupStackAndInitializeKernel
>  -.func KiSetupStackAndInitializeKernel
>  -_KiSetupStackAndInitializeKernel:
>  -
>  -    /* Save current stack */
>  -    mov rsi, rsp
>  -
>  -    /* Setup the new stack */
>  -    mov rsp, r8
>  -    sub rsp, NPX_FRAME_LENGTH + KTRAP_FRAME_ALIGN + KTRAP_FRAME_LENGTH
>  -    push CR0_EM + CR0_TS + CR0_MP
>  -
>  -    /* Copy stack parameters to the new stack */
>  -    push [rsi + 16]
>  -    push [rsi + 8]
>  -    xor rbp, rbp
>  -    call _KiInitializeKernel
>  -
>  -    /* Set the priority of this thread to 0 */
>  -    mov rbx, PCR[KPCR_CURRENT_THREAD]
>  -    mov byte ptr [rbx+KTHREAD_PRIORITY], 0
>  -
>  -    /* Force interrupts enabled and lower IRQL back to DISPATCH_LEVEL */
>  -    sti
>  -    mov rcx, DISPATCH_LEVEL
>  -    mov cr8, rcx
>  -
>  -    /* Set the right wait IRQL */
>  -    mov byte ptr [rbx+KTHREAD_WAIT_IRQL], DISPATCH_LEVEL;
>  -
>  -    /* Jump into the idle loop */
>  -    jmp _KiIdleLoop
>  -.endfunc
>
>  Modified: branches/ros-amd64-bringup/reactos/ntoskrnl/ke/amd64/kiinit.c
>  URL:
> http://svn.reactos.org/svn/reactos/branches/ros-amd64-bringup/reactos/ntosk…
>  ==============================================================================
>  --- branches/ros-amd64-bringup/reactos/ntoskrnl/ke/amd64/kiinit.c [iso-8859-1]
> (original)
>  +++ branches/ros-amd64-bringup/reactos/ntoskrnl/ke/amd64/kiinit.c [iso-8859-1] Thu Aug 21
> 16:16:57 2008
>  @@ -366,6 +366,7 @@
>   
>       /* Start us out at PASSIVE_LEVEL */
>   //    Pcr->Irql = PASSIVE_LEVEL;
>  +    KeSetCurrentIrql(PASSIVE_LEVEL);
>   
>       /* Set the GDI, IDT, TSS and DPC Stack */
>       Pcr->GdtBase = (PVOID)Gdt;
>  @@ -386,6 +387,7 @@
>                      IN CCHAR Number,
>                      IN PLOADER_PARAMETER_BLOCK LoaderBlock)
>   {
>  +    FrLdrDbgPrint("Enter KiInitializeKernel\n");
>   #if 0
>       BOOLEAN NpxPresent;
>       ULONG FeatureBits;
>  @@ -642,6 +644,17 @@
>                                 (ULONG64)TssSelector.BaseUpper << 32);
>   }
>   
>  +// Hack
>  +VOID KiRosPrepareForSystemStartup(ULONG, PROS_LOADER_PARAMETER_BLOCK);
>  +
>  +VOID
>  +NTAPI
>  +KiSystemStartup(IN ULONG_PTR Dummy,
>  +                IN PROS_LOADER_PARAMETER_BLOCK LoaderBlock)
>  +{
>  +    KiRosPrepareForSystemStartup(Dummy, LoaderBlock);
>  +}
>  +
>   VOID
>   NTAPI
>   KiSystemStartupReal(IN PLOADER_PARAMETER_BLOCK LoaderBlock)
>  @@ -740,13 +753,15 @@
>   //        RtlCopyMemory(&Idt[8], &DoubleFaultEntry, sizeof(KIDTENTRY));
>       }
>   
>  -
>  +#if 0 // FIXME: InterlockedBitTestAndSet64 is broken! It needs to be specified
>  +      // that it should reference an absolute address!
>       /* Loop until we can release the freeze lock */
>       do
>       {
>           /* Loop until execution can continue */
>           while (*(volatile PKSPIN_LOCK*)&KiFreezeExecutionLock == (PVOID)1);
>  -    } while(InterlockedBitTestAndSet((PLONG)&KiFreezeExecutionLock, 0));
>  +    } while(InterlockedBitTestAndSet64((PLONG64)&KiFreezeExecutionLock, 0));
>  +#endif
>   
>       /* Setup CPU-related fields */
>       Pcr->Prcb.Number = Cpu;
>  @@ -773,16 +788,29 @@
>       KfRaiseIrql(HIGH_LEVEL);
>   
>       /* Align stack and make space for the trap frame and NPX frame */
>  -    InitialStack &= ~(KTRAP_FRAME_ALIGN - 1);
>  -
>  -FrLdrDbgPrint("Before KiSetupStackAndInitializeKernel\n");
>  -for(;;);
>  -
>  -    /* Switch to new kernel stack and start kernel bootstrapping */
>  -    KiSetupStackAndInitializeKernel(&KiInitialProcess.Pcb,
>  -                                    InitialThread,
>  -                                    (PVOID)InitialStack,
>  -                                    &Pcr->Prcb,
>  -                                    (CCHAR)Cpu,
>  -                                    KeLoaderBlock);
>  +    InitialStack &= ~(16 - 1);
>  +
>  +    /* Switch to new kernel Stack */
>  +    KiSwapStack(InitialStack);
>  +
>  +    /* Initialize kernel */
>  +    KiInitializeKernel(&KiInitialProcess.Pcb,
>  +                       InitialThread,
>  +                       (PVOID)InitialStack,
>  +                       &Pcr->Prcb,
>  +                       (CCHAR)Cpu,
>  +                       KeLoaderBlock);
>  +
>  +    /* Set the priority of this thread to 0 */
>  +    InitialThread->Priority = 0;
>  +
>  +    /* Force interrupts enabled and lower IRQL back to DISPATCH_LEVEL */
>  +    _enable();
>  +    KeLowerIrql(DISPATCH_LEVEL);
>  +
>  +    /* Set the right wait IRQL */
>  +    InitialThread->WaitIrql = DISPATCH_LEVEL;
>  +
>  +    /* Jump into the idle loop */
>  +    KiIdleLoop();
>   }
>
>  Modified: branches/ros-amd64-bringup/reactos/ntoskrnl/ke/amd64/trap.S
>  URL:
> http://svn.reactos.org/svn/reactos/branches/ros-amd64-bringup/reactos/ntosk…
>  ==============================================================================
>  --- branches/ros-amd64-bringup/reactos/ntoskrnl/ke/amd64/trap.S [iso-8859-1] (original)
>  +++ branches/ros-amd64-bringup/reactos/ntoskrnl/ke/amd64/trap.S [iso-8859-1] Thu Aug 21
> 16:16:57 2008
>  @@ -13,7 +13,22 @@
>   .intel_syntax noprefix
>   .code64
>   
>  +#define KERNEL_STACK_SIZE 0x6000
>  +
>   /* GLOBALS *******************************************************************/
>  +
>  +.bss
>  +.align 16
>  +
>  +/* Kernel Boot Stack */
>  +.globl _P0BootStack
>  +.space KERNEL_STACK_SIZE
>  +_P0BootStack:
>  +
>  +/* Kernel Double-Fault and Temporary DPC Stack */
>  +.globl _KiDoubleFaultStack
>  +.space KERNEL_STACK_SIZE
>  +_KiDoubleFaultStack:
>   
>   .data
>   
>
>  Modified: branches/ros-amd64-bringup/reactos/ntoskrnl/ntoskrnl-amd64hack.rbuild
>  URL:
> http://svn.reactos.org/svn/reactos/branches/ros-amd64-bringup/reactos/ntosk…
>  ==============================================================================
>  --- branches/ros-amd64-bringup/reactos/ntoskrnl/ntoskrnl-amd64hack.rbuild [iso-8859-1]
> (original)
>  +++ branches/ros-amd64-bringup/reactos/ntoskrnl/ntoskrnl-amd64hack.rbuild [iso-8859-1]
> Thu Aug 21 16:16:57 2008
>  @@ -90,7 +90,6 @@
>   		</if>
>   		<if property="ARCH" value="amd64">
>   			<directory name="amd64">
>  -				<file first="true">boot.S</file>
>   				<file>cpu.c</file>
>   				<file>except.c</file>
>   				<file>irql.c</file>
>
>

[Alex Ionescu]

CIA-23:tkreuzer ros-amd64-bringup * r35518 /reactos/ntoskrnl/ (5 files  
in 3 dirs):
[3:03pm]CIA-23:Implement KiSystemStartup in C instead of asm,  
implement KiSwapStack intrinsic,
[3:03pm]CIA-23:get rid of KiSetupStackandInitializeKernel, instead do  
the work in
[3:03pm]CIA-23:KiSystemStartupReal in C. Move Stack definitions into  
trap.S and get rid of the
[3:03pm]CIA-23:whole boot.S file. Comment out the sync loop for the  
moment and add a comment
[3:03pm]CIA-23:why it doesn't work (InterlockedBitTestAndSet64 doesn't  
work correctly).
[3:04pm]Alex_Ionescu:Physicus: w3seek is going to rape you
Physicus:why?
[3:05pm]Alex_Ionescu:you're swapping stacks...in C
[3:05pm]Alex_Ionescu:you don't see a problem there?

:)
On 21-Aug-08, at 3:09 PM, Thomas Bluemel wrote:

>   Calling from KiSwapStack from C seems quite scary. C
> doesn't know
>  anything about you messing with the stack, so any subsequent code may
>  still refer to a now invalid stack. There's a reason this was
>  implemented in asm... 
Best regards,
Alex Ionescu