[HALX86] Address code review comments

boot/freeldr/freeldr/ntldr/arch/i386/winldr.c

@@ -64,41 +64,6 @@ typedef struct
 #define TYPE_CODE   (0x10 | DESCRIPTOR_CODE | DESCRIPTOR_EXECUTE_READ)
 #define TYPE_DATA   (0x10 | DESCRIPTOR_READ_WRITE)
 
-VOID
-KiSetGdtEntryEx(
-    IN OUT PKGDTENTRY Entry,
-    IN ULONG32 Base,
-    IN ULONG Limit,
-    IN UCHAR Type,
-    IN UCHAR Dpl,
-    IN BOOLEAN Granularity,
-    IN UCHAR SegMode) // 0: 16-bit, 1: 32-bit, 2: 64-bit
-{
-    KiSetGdtDescriptorBase(Entry, Base);
-    KiSetGdtDescriptorLimit(Entry, Limit);
-    Entry->HighWord.Bits.Type = (Type & 0x1f);
-    Entry->HighWord.Bits.Dpl  = (Dpl & 0x3);
-    Entry->HighWord.Bits.Pres = (Type != 0); // Present, must be 1 when the GDT entry is valid.
-    Entry->HighWord.Bits.Sys  = 0;           // System
-    Entry->HighWord.Bits.Reserved_0  = 0;    // LongMode = !!(SegMode & 1);
-    Entry->HighWord.Bits.Default_Big = !!(SegMode & 2);
-    Entry->HighWord.Bits.Granularity |= !!Granularity; // The flag may have been already set by KiSetGdtDescriptorLimit().
-    // Entry->MustBeZero = 0;
-}
-
-FORCEINLINE
-VOID
-KiSetGdtEntry(
-    IN OUT PKGDTENTRY Entry,
-    IN ULONG32 Base,
-    IN ULONG Limit,
-    IN UCHAR Type,
-    IN UCHAR Dpl,
-    IN UCHAR SegMode) // 0: 16-bit, 1: 32-bit, 2: 64-bit
-{
-    KiSetGdtEntryEx(Entry, Base, Limit, Type, Dpl, FALSE, SegMode);
-}
-
 #if 0
 VOID
 DumpGDTEntry(ULONG_PTR Base, ULONG Selector)

hal/halx86/apic/apicsmp.c

@@ -3,14 +3,15 @@
  * LICENSE:     GPL-2.0-or-later (https://spdx.org/licenses/GPL-2.0-or-later)
  * PURPOSE:     SMP specific APIC code
  * COPYRIGHT:   Copyright 2021 Timo Kreuzer <[email protected]>
- *              Copyright 2021 Justin Miller <[email protected]>
+ *              Copyright 2023 Justin Miller <[email protected]>
  */
 
 /* INCLUDES *******************************************************************/
 
 #include <hal.h>
 #include "apicp.h"
 #include <smp.h>
+
 #define NDEBUG
 #include <debug.h>
 
@@ -40,7 +41,7 @@ extern PPROCESSOR_IDENTITY HalpProcessorIdentity;
             local APIC(s) specified in Destination field. Vector specifies
             the startup address.
         APIC_MT_ExtInt - Delivers an external interrupt to the target local
-            APIC specified in Destination field. 
+            APIC specified in Destination field.
 
     \param TriggerMode - The trigger mode of the interrupt. Can be:
         APIC_TGM_Edge - The interrupt is edge triggered.
@@ -68,6 +69,12 @@ ApicRequestGlobalInterrupt(
 {
     APIC_INTERRUPT_COMMAND_REGISTER Icr;
 
+    /* Wait for the APIC to be idle */
+    do
+    {
+        Icr.Long0 = ApicRead(APIC_ICR0);
+    } while (Icr.DeliveryStatus);
+
     /* Setup the command register */
     Icr.LongLong = 0;
     Icr.Vector = Vector;
@@ -90,14 +97,14 @@ ApicRequestGlobalInterrupt(
 
 VOID
 NTAPI
-HalpRequestIpi(KAFFINITY TargetProcessors)
+HalpRequestIpi(_In_ KAFFINITY TargetProcessors)
 {
     UNIMPLEMENTED;
     __debugbreak();
 }
 
 VOID
-ApicStartApplicationProcessor(ULONG NTProcessorNumber, PHYSICAL_ADDRESS StartupLoc)
+ApicStartApplicationProcessor(_In_ ULONG NTProcessorNumber, _In_ PHYSICAL_ADDRESS StartupLoc)
 {
     ASSERT(StartupLoc.HighPart == 0);
     ASSERT((StartupLoc.QuadPart & 0xFFF) == 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

@@ -1,13 +1,14 @@
 /*
  * PROJECT:     ReactOS Kernel
  * LICENSE:     GPL-2.0-or-later (https://spdx.org/licenses/GPL-2.0-or-later)
- * PURPOSE:     Core source file for UP alternative functions
+ * PURPOSE:     Core source file for Uniprocessor (UP) alternative functions
  * COPYRIGHT:   Copyright 2021 Justin Miller <[email protected]>
  */
 
 /* INCLUDES ******************************************************************/
 
 #include <hal.h>
+
 #define NDEBUG
 #include <debug.h>
 

hal/halx86/include/smp.h

@@ -45,8 +45,8 @@ HalpPrintApicTables(VOID);
 /* APIC specific functions inside apic/apicsmp.c */
 
 VOID
-ApicStartApplicationProcessor(ULONG NTProcessorNumber, PHYSICAL_ADDRESS StartupLoc);
+ApicStartApplicationProcessor(_In_ ULONG NTProcessorNumber, _In_ PHYSICAL_ADDRESS StartupLoc);
 
 VOID
 NTAPI
-HalpRequestIpi(KAFFINITY TargetProcessors);
+HalpRequestIpi(_In_ KAFFINITY TargetProcessors);

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/apentry.S

@@ -1,7 +1,7 @@
 /*
  * PROJECT:     ReactOS Kernel
  * LICENSE:     GPL-2.0-or-later (https://spdx.org/licenses/GPL-2.0-or-later)
- * PURPOSE:     Assembly file for real mode AP code
+ * PURPOSE:     AMD64 Application Processor (AP) spinup setup
  * COPYRIGHT:   Copyright 2021 Justin Miller <[email protected]>
  */
 

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

@@ -1,7 +1,7 @@
 /*
  * PROJECT:     ReactOS HAL
  * LICENSE:     GPL-2.0-or-later (https://spdx.org/licenses/GPL-2.0-or-later)
- * PURPOSE:     Application processor startup code for i386
+ * PURPOSE:     i386 Application Processor (AP) spinup setup
  * COPYRIGHT:   Copyright 2021 Justin Miller <[email protected]>
  *              Copyright 2021 Victor Perevertkin <[email protected]>
  */
@@ -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

@@ -1,7 +1,7 @@
 /*
  * PROJECT:     ReactOS Kernel
  * LICENSE:     GPL-2.0-or-later (https://spdx.org/licenses/GPL-2.0-or-later)
- * PURPOSE:     i386 AP spinup setup
+ * PURPOSE:     i386 Application Processor (AP) spinup setup
  * COPYRIGHT:   Copyright 2021 Justin Miller <[email protected]>
  *              Copyright 2021 Victor Perevertkin <[email protected]>
  */
@@ -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

@@ -1,14 +1,15 @@
 /*
  * PROJECT:     ReactOS Kernel
  * LICENSE:     GPL-2.0-or-later (https://spdx.org/licenses/GPL-2.0-or-later)
- * PURPOSE:     Source file for IPI management
+ * PURPOSE:     Source file for Inter-Processor Interrupts management
  * COPYRIGHT:   Copyright 2021 Justin Miller <[email protected]>
  */
 
 /* INCLUDES ******************************************************************/
 
 #include <hal.h>
 #include <smp.h>
+
 #define NDEBUG
 #include <debug.h>
 

hal/halx86/smp/smp.c

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