[HALX86] Address code review comments

hal/halx86/apic/apicsmp.c

@@ -67,6 +67,13 @@ ApicRequestGlobalInterrupt(
     _In_ APIC_DSH DestinationShortHand)
 {
     APIC_INTERRUPT_COMMAND_REGISTER Icr;
+    APIC_INTERRUPT_COMMAND_REGISTER IcrStatus;
+
+    /* Wait for the APIC to be idle */
+    do
+    {
+        IcrStatus.Long0 = ApicRead(APIC_ICR0);
+    } while (IcrStatus.DeliveryStatus);
 
     /* Setup the command register */
     Icr.LongLong = 0;
@@ -107,6 +114,10 @@ ApicStartApplicationProcessor(ULONG NTProcessorNumber, PHYSICAL_ADDRESS StartupL
     ApicRequestGlobalInterrupt(HalpProcessorIdentity[NTProcessorNumber].LapicId, 0,
         APIC_MT_INIT, APIC_TGM_Edge, APIC_DSH_Destination);
 
+    /* De-Assert Init IPI */
+    ApicRequestGlobalInterrupt(HalpProcessorIdentity[NTProcessorNumber].LapicId, 0,
+        APIC_MT_INIT, APIC_TGM_Level, APIC_DSH_Destination);
+
     /* Stall execution for a bit to give APIC time: MPS Spec - B.4 */
     KeStallExecutionProcessor(200);
 

hal/halx86/generic/up.c

@@ -8,6 +8,7 @@
 /* INCLUDES ******************************************************************/
 
 #include <hal.h>
+
 #define NDEBUG
 #include <debug.h>
 

hal/halx86/smp.cmake

@@ -10,10 +10,7 @@ if(ARCH STREQUAL "i386")
         smp/i386/apentry.S)
     list(APPEND HAL_SMP_SOURCE
         smp/i386/spinup.c)
-endif()
-
-
-if(ARCH STREQUAL "amd64")
+elseif(ARCH STREQUAL "amd64")
     list(APPEND HAL_SMP_ASM_SOURCE
         smp/amd64/apentry.S)
     list(APPEND HAL_SMP_SOURCE

hal/halx86/smp/amd64/spinup.c

@@ -9,6 +9,7 @@
 
 #include <hal.h>
 #include <smp.h>
+
 #define NDEBUG
 #include <debug.h>
 

hal/halx86/smp/i386/apentry.S

@@ -34,7 +34,7 @@ _HalpAPEntry16:
 #else
     data32 lgdt cs:[ZERO_OFFSET(Gdtr)]
 #endif
-    
+
     /* Load temp page table */
     mov eax, cs:[ZERO_OFFSET(PageTableRoot)]
     mov cr3, eax
@@ -63,11 +63,11 @@ Gdtr_Pad:
 Gdtr:
     .short 0 // Limit
     .long 0 // Base
-_HalpAPEntry16End:  
+_HalpAPEntry16End:
 .endcode16
 
 .code32
-_HalpAPEntry32: 
+_HalpAPEntry32:
     /* Set the Ring 0 DS/ES/SS Segment */
     mov ax, HEX(10)
     mov ds, ax

hal/halx86/smp/i386/spinup.c

@@ -10,6 +10,7 @@
 
 #include <hal.h>
 #include <smp.h>
+
 #define NDEBUG
 #include <debug.h>
 
@@ -28,12 +29,19 @@ extern HALP_APIC_INFO_TABLE HalpApicInfoTable;
 
 ULONG HalpStartedProcessorCount = 1;
 
+#ifndef Add2Ptr
+#define Add2Ptr(P,I) ((PVOID)((PUCHAR)(P) + (I)))
+#endif
+#ifndef PtrOffset
+#define PtrOffset(B,O) ((ULONG)((ULONG_PTR)(O) - (ULONG_PTR)(B)))
+#endif
+
 typedef struct _AP_ENTRY_DATA
 {
     ULONG Jump32Offset;
     ULONG Jump32Segment;
     PVOID SelfPtr;
-    PFN_NUMBER PageTableRoot;
+    ULONG PageTableRoot;
     PKPROCESSOR_STATE ProcessorState;
     KDESCRIPTOR Gdtr;
 } AP_ENTRY_DATA, *PAP_ENTRY_DATA;
@@ -47,64 +55,36 @@ typedef struct _AP_SETUP_STACK
 /* FUNCTIONS *****************************************************************/
 
 static
-VOID
-HalpMapAddressFlat(
-    _Inout_ PMMPDE PageDirectory,
-    _In_ PVOID VirtAddress,
-    _In_ PVOID TargetVirtAddress)
-{
-    if (TargetVirtAddress == NULL)
-        TargetVirtAddress = VirtAddress;
-
-    PMMPDE currentPde;
-
-    currentPde = &PageDirectory[MiAddressToPdeOffset(TargetVirtAddress)];
-
-    // Allocate a Page Table if there is no one for this address
-    if (currentPde->u.Long == 0)
-    {
-        PMMPTE pageTable = ExAllocatePoolZero(NonPagedPoolMustSucceed , PAGE_SIZE, TAG_HAL);
-
-        currentPde->u.Hard.PageFrameNumber = MmGetPhysicalAddress(pageTable).QuadPart >> PAGE_SHIFT;
-        currentPde->u.Hard.Valid = TRUE;
-        currentPde->u.Hard.Write = TRUE;
-    }
-
-    // Map the Page Table so we can add our VirtAddress there (hack around I/O memory mapper for that)
-    PHYSICAL_ADDRESS b = {.QuadPart = (ULONG_PTR)currentPde->u.Hard.PageFrameNumber << PAGE_SHIFT};
-
-    PMMPTE pageTable = MmMapIoSpace(b, PAGE_SIZE, MmCached);
-
-    PMMPTE currentPte = &pageTable[MiAddressToPteOffset(TargetVirtAddress)];
-    currentPte->u.Hard.PageFrameNumber = MmGetPhysicalAddress(VirtAddress).QuadPart >> PAGE_SHIFT;
-    currentPte->u.Hard.Valid = TRUE;
-    currentPte->u.Hard.Write = TRUE;
-
-    MmUnmapIoSpace(pageTable, PAGE_SIZE);
-
-    DPRINT("Map %p -> %p, PDE %u PTE %u\n",
-           TargetVirtAddress,
-           MmGetPhysicalAddress(VirtAddress).LowPart,
-           MiAddressToPdeOffset(TargetVirtAddress),
-           MiAddressToPteOffset(TargetVirtAddress));
-}
-
-static
-PHYSICAL_ADDRESS
+ULONG
 HalpSetupTemporaryMappings(
     _In_ PKPROCESSOR_STATE ProcessorState)
 {
-    PHYSICAL_ADDRESS Cr3PhysicalAddress;
-    Cr3PhysicalAddress.QuadPart = ProcessorState->SpecialRegisters.Cr3;
-
-    PMMPDE pageDirectory = MmMapIoSpace(Cr3PhysicalAddress, PAGE_SIZE, MmCached);
-    ASSERT(pageDirectory);
-
-    // Map the low stub
-    HalpMapAddressFlat(pageDirectory, HalpLowStub, (PVOID)(ULONG_PTR)HalpLowStubPhysicalAddress.QuadPart);
-    HalpMapAddressFlat(pageDirectory, HalpLowStub, NULL);
+    PMMPDE RootPageTable = Add2Ptr(HalpLowStub, PAGE_SIZE);
+    PMMPDE LowMapPde = Add2Ptr(HalpLowStub, 2 * PAGE_SIZE);
+    PMMPTE LowStubPte = MiAddressToPte(HalpLowStub);
+    PHYSICAL_ADDRESS PhysicalAddress;
+    ULONG StartPti;
+
+    /* Copy current mappings */
+    RtlCopyMemory(RootPageTable, MiAddressToPde(NULL), PAGE_SIZE);
+
+    /* Set up low PDE */
+    PhysicalAddress = MmGetPhysicalAddress(LowMapPde);
+    RootPageTable[0].u.Hard.PageFrameNumber = PhysicalAddress.QuadPart >> PAGE_SHIFT;
+    RootPageTable[0].u.Hard.Valid = 1;
+    RootPageTable[0].u.Hard.Write = 1;
+
+    /* Copy low stub PTEs */
+    StartPti = MiAddressToPteOffset(HalpLowStubPhysicalAddress.QuadPart);
+    ASSERT(StartPti + 10 < 1024);
+    for (ULONG i = 0; i < 10; i++)
+    {
+        LowMapPde[StartPti + i] = LowStubPte[i];
+    }
 
-    return MmGetPhysicalAddress(pageDirectory);
+    PhysicalAddress = MmGetPhysicalAddress(RootPageTable);
+    ASSERT(PhysicalAddress.QuadPart < 0x100000000);
+    return (ULONG)PhysicalAddress.QuadPart;
 }
 
 BOOLEAN
@@ -113,7 +93,6 @@ HalStartNextProcessor(
     _In_ PLOADER_PARAMETER_BLOCK LoaderBlock,
     _In_ PKPROCESSOR_STATE ProcessorState)
 {
-
     /* Write KeLoaderBlock into Stack */
     ProcessorState->ContextFrame.Esp = (ULONG)((ULONG_PTR)ProcessorState->ContextFrame.Esp - sizeof(AP_SETUP_STACK));
     PAP_SETUP_STACK ApStack = (PAP_SETUP_STACK)ProcessorState->ContextFrame.Esp;
@@ -123,17 +102,23 @@ HalStartNextProcessor(
     if (HalpStartedProcessorCount == HalpApicInfoTable.ProcessorCount)
         return FALSE;
 
+    // Initalize the temporary page table
+    // TODO: clean it up after an AP boots successfully
+    ULONG initialCr3 = HalpSetupTemporaryMappings(ProcessorState);
+    if (!initialCr3)
+        return FALSE;
+
     // Put the bootstrap code into low memory
     RtlCopyMemory(HalpLowStub, &HalpAPEntry16,  ((ULONG_PTR)&HalpAPEntry16End - (ULONG_PTR)&HalpAPEntry16));
 
-    // Get a pointer to apEntryData 
+    // Get a pointer to apEntryData
     PAP_ENTRY_DATA apEntryData = (PVOID)((ULONG_PTR)HalpLowStub + ((ULONG_PTR)&HalpAPEntryData - (ULONG_PTR)&HalpAPEntry16));
 
     *apEntryData = (AP_ENTRY_DATA){
         .Jump32Offset = (ULONG)&HalpAPEntry32,
         .Jump32Segment = (ULONG)ProcessorState->ContextFrame.SegCs,
         .SelfPtr = (PVOID)apEntryData,
-        .PageTableRoot = HalpSetupTemporaryMappings(ProcessorState).QuadPart,
+        .PageTableRoot = initialCr3,
         .ProcessorState = ProcessorState,
         .Gdtr = ProcessorState->SpecialRegisters.Gdtr,
     };

hal/halx86/smp/ipi.c

@@ -9,6 +9,7 @@
 
 #include <hal.h>
 #include <smp.h>
+
 #define NDEBUG
 #include <debug.h>
 

ntoskrnl/ke/amd64/mproc.c

@@ -8,7 +8,8 @@
 /* INCLUDES *****************************************************************/
 
 #include <ntoskrnl.h>
-// #define NDEBUG
+
+#define NDEBUG
 #include <debug.h>
 
 /* FUNCTIONS *****************************************************************/

ntoskrnl/ke/i386/kiinit.c

@@ -765,7 +765,7 @@ KiSystemStartup(IN PLOADER_PARAMETER_BLOCK LoaderBlock)
 
     /* Initialize the machine type */
     KiInitializeMachineType();
-
+ 
     /* Skip initial setup if this isn't the Boot CPU */
     if (Cpu) goto AppCpuInit;
 
@@ -825,15 +825,15 @@ KiSystemStartup(IN PLOADER_PARAMETER_BLOCK LoaderBlock)
     __writefsdword(KPCR_NUMBER, Cpu);
     __writefsdword(KPCR_SET_MEMBER, 1 << Cpu);
     __writefsdword(KPCR_SET_MEMBER_COPY, 1 << Cpu);
-    __writefsdword(KPCR_PRCB_SET_MEMBER, 1 << Cpu);
+    __writefsdword(KPCR_PRCB_SET_MEMBER, 1 << Cpu);;
 
     //TODO: We don't setup IPIs yet so freeze other processors here.
     if (Cpu)
     {
         KeMemoryBarrier();
-        LoaderBlock->Prcb = 0; 
-        
-        for(;;)
+        LoaderBlock->Prcb = 0;
+
+        for (;;)
         {
             KeMemoryBarrier();
             YieldProcessor();

ntoskrnl/ke/i386/mproc.c

@@ -9,7 +9,7 @@
 /* INCLUDES *****************************************************************/
 
 #include <ntoskrnl.h>
-// #define NDEBUG
+#define NDEBUG
 #include <debug.h>
 
 typedef struct _APINFO
@@ -62,8 +62,8 @@ KeStartAllProcessors(VOID)
         /* Initalize a new PCR for the specific AP */
         KiInitializePcr(ProcessorCount,
                         &APInfo->Pcr,
-                        &APInfo->Idt[0], 
-                        &APInfo->Gdt[0], 
+                        &APInfo->Idt[0],
+                        &APInfo->Gdt[0],
                         &APInfo->Tss,
                         (PKTHREAD)&APInfo->Thread,
                         DPCStack);
@@ -120,7 +120,7 @@ KeStartAllProcessors(VOID)
         KeLoaderBlock->Thread = (ULONG_PTR)&APInfo->Pcr.Prcb->IdleThread;
 
         // Start the CPU
-        DPRINT("Starting CPU: #%u\n", ProcessorCount);
+        DPRINT("Attempting to Start a CPU with number: %u\n", ProcessorCount);
         if (!HalStartNextProcessor(KeLoaderBlock, ProcessorState))
         {
             break;
@@ -131,12 +131,11 @@ KeStartAllProcessors(VOID)
             KeMemoryBarrier();
             YieldProcessor();
         }
-        DPRINT1("Escape\n");
     }
 
     // The last CPU didn't start - clean the data
     ProcessorCount--;
-    
+
     if (APInfo)
         ExFreePoolWithTag(APInfo, '  eK');
     if (KernelStack)