[ros-diffs] [reactos] 03/06: [HAL:APIC] Code fixes

[Timo Kreuzer]

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=b7a149fcec7f3626e671f…

commit b7a149fcec7f3626e671f1da658589f8c219503f
Author:     Timo Kreuzer <timo.kreuzer(a)reactos.org>
AuthorDate: Sat Jun 5 15:39:22 2021 +0200
Commit:     Timo Kreuzer <timo.kreuzer(a)reactos.org>
CommitDate: Thu Jun 17 23:27:44 2021 +0200

    [HAL:APIC] Code fixes
    
    * Fix some broken code
    * Add some ASSERTs
    * Use ApicWriteIORedirectionEntry where appropriate
    * Use KeQueryInterruptHandler/KeRegisterInterruptHandler to save/restore the old
handler instead of saving the KIDTENTRY
    * Move HalpProfileInterruptHandler to apictimer.c and implement it
    * Use READ/WRITE_REGISTER macros
    * Add some symbolic names
---
 hal/halx86/apic/apic.c      | 40 +++++++++++++++++++++++-----------------
 hal/halx86/apic/apicp.h     |  8 ++++++--
 hal/halx86/apic/apictimer.c |  7 +++++++
 hal/halx86/apic/rtctimer.c  |  7 -------
 hal/halx86/apic/tsc.c       | 10 ++++------
 5 files changed, 40 insertions(+), 32 deletions(-)

diff --git a/hal/halx86/apic/apic.c b/hal/halx86/apic/apic.c
index 2811cc45535..65a6d2e4d20 100644
--- a/hal/halx86/apic/apic.c
+++ b/hal/halx86/apic/apic.c
@@ -94,8 +94,9 @@ ULONG
 IOApicRead(UCHAR Register)
 {
     /* Select the register, then do the read */
-    *(volatile UCHAR *)(IOAPIC_BASE + IOAPIC_IOREGSEL) = Register;
-    return *(volatile ULONG *)(IOAPIC_BASE + IOAPIC_IOWIN);
+    ASSERT(Register <= 0x3F);
+    WRITE_REGISTER_ULONG((PULONG)(IOAPIC_BASE + IOAPIC_IOREGSEL), Register);
+    return READ_REGISTER_ULONG((PULONG)(IOAPIC_BASE + IOAPIC_IOWIN));
 }
 
 FORCEINLINE
@@ -103,8 +104,9 @@ VOID
 IOApicWrite(UCHAR Register, ULONG Value)
 {
     /* Select the register, then do the write */
-    *(volatile UCHAR *)(IOAPIC_BASE + IOAPIC_IOREGSEL) = Register;
-    *(volatile ULONG *)(IOAPIC_BASE + IOAPIC_IOWIN) = Value;
+    ASSERT(Register <= 0x3F);
+    WRITE_REGISTER_ULONG((PULONG)(IOAPIC_BASE + IOAPIC_IOREGSEL), Register);
+    WRITE_REGISTER_ULONG((PULONG)(IOAPIC_BASE + IOAPIC_IOWIN), Value);
 }
 
 FORCEINLINE
@@ -113,6 +115,7 @@ ApicWriteIORedirectionEntry(
     UCHAR Index,
     IOAPIC_REDIRECTION_REGISTER ReDirReg)
 {
+    ASSERT(Index < APIC_MAX_IRQ);
     IOApicWrite(IOAPIC_REDTBL + 2 * Index, ReDirReg.Long0);
     IOApicWrite(IOAPIC_REDTBL + 2 * Index + 1, ReDirReg.Long1);
 }
@@ -124,6 +127,7 @@ ApicReadIORedirectionEntry(
 {
     IOAPIC_REDIRECTION_REGISTER ReDirReg;
 
+    ASSERT(Index < APIC_MAX_IRQ);
     ReDirReg.Long0 = IOApicRead(IOAPIC_REDTBL + 2 * Index);
     ReDirReg.Long1 = IOApicRead(IOAPIC_REDTBL + 2 * Index + 1);
 
@@ -344,7 +348,7 @@ HalpAllocateSystemInterrupt(
     Vector = IrqlToTpr(Irql) & 0xF0;
 
     /* Find an empty vector */
-    while (HalpVectorToIndex[Vector] != 0xFF)
+    while (HalpVectorToIndex[Vector] != APIC_FREE_VECTOR)
     {
         Vector++;
 
@@ -372,8 +376,7 @@ HalpAllocateSystemInterrupt(
     ReDirReg.Destination = 0;
 
     /* Initialize entry */
-    IOApicWrite(IOAPIC_REDTBL + 2 * Irq, ReDirReg.Long0);
-    IOApicWrite(IOAPIC_REDTBL + 2 * Irq + 1, ReDirReg.Long1);
+    ApicWriteIORedirectionEntry(Irq, ReDirReg);
 
     return Vector;
 }
@@ -410,17 +413,16 @@ ApicInitializeIOApic(VOID)
     ReDirReg.Destination = 0;
 
     /* Loop all table entries */
-    for (Index = 0; Index < 24; Index++)
+    for (Index = 0; Index < APIC_MAX_IRQ; Index++)
     {
         /* Initialize entry */
-        IOApicWrite(IOAPIC_REDTBL + 2 * Index, ReDirReg.Long0);
-        IOApicWrite(IOAPIC_REDTBL + 2 * Index + 1, ReDirReg.Long1);
+        ApicWriteIORedirectionEntry(Index, ReDirReg);
     }
 
     /* Init the vactor to index table */
     for (Vector = 0; Vector <= 255; Vector++)
     {
-        HalpVectorToIndex[Vector] = 0xFF;
+        HalpVectorToIndex[Vector] = APIC_FREE_VECTOR;
     }
 
     // HACK: Allocate all IRQs, should rather do that on demand
@@ -437,7 +439,7 @@ ApicInitializeIOApic(VOID)
     ReDirReg.TriggerMode = APIC_TGM_Edge;
     ReDirReg.Mask = 0;
     ReDirReg.Destination = ApicRead(APIC_ID);
-    IOApicWrite(IOAPIC_REDTBL + 2 * APIC_CLOCK_INDEX, ReDirReg.Long0);
+    ApicWriteIORedirectionEntry(APIC_CLOCK_INDEX, ReDirReg);
 }
 
 VOID
@@ -457,9 +459,10 @@ HalpInitializePICs(IN BOOLEAN EnableInterrupts)
     ApicInitializeIOApic();
 
     /* Manually reserve some vectors */
+    HalpVectorToIndex[APC_VECTOR] = APIC_RESERVED_VECTOR;
+    HalpVectorToIndex[DISPATCH_VECTOR] = APIC_RESERVED_VECTOR;
     HalpVectorToIndex[APIC_CLOCK_VECTOR] = 8;
-    HalpVectorToIndex[APC_VECTOR] = 99;
-    HalpVectorToIndex[DISPATCH_VECTOR] = 99;
+    HalpVectorToIndex[APIC_SPURIOUS_VECTOR] = APIC_RESERVED_VECTOR;
 
     /* Set interrupt handlers in the IDT */
     KeRegisterInterruptHandler(APIC_CLOCK_VECTOR, HalpClockInterrupt);
@@ -669,7 +672,7 @@ HalDisableSystemInterrupt(
     ReDirReg.Mask = 1;
 
     /* Write back lower dword */
-    IOApicWrite(IOAPIC_REDTBL + 2 * Irql, ReDirReg.Long0);
+    IOApicWrite(IOAPIC_REDTBL + 2 * Index, ReDirReg.Long0);
 }
 
 #ifndef _M_AMD64
@@ -704,8 +707,8 @@ HalBeginSystemInterrupt(
         /* Get the irq for this vector */
         Index = HalpVectorToIndex[Vector];
 
-        /* Check if its valid */
-        if (Index != 0xff)
+        /* Check if it's valid */
+        if (Index < APIC_MAX_IRQ)
         {
             /* Read the I/O redirection entry */
             RedirReg = ApicReadIORedirectionEntry(Index);
@@ -715,6 +718,9 @@ HalBeginSystemInterrupt(
        }
        else
        {
+            /* This should be a reserved vector! */
+            ASSERT(Index == APIC_RESERVED_VECTOR);
+
             /* Re-request the interrupt to be handled later */
             ApicRequestInterrupt(Vector, APIC_TGM_Edge);
        }
diff --git a/hal/halx86/apic/apicp.h b/hal/halx86/apic/apicp.h
index ee8eec3d665..d3e1ece84aa 100644
--- a/hal/halx86/apic/apicp.h
+++ b/hal/halx86/apic/apicp.h
@@ -25,6 +25,10 @@
     #define TprToIrql(Tpr)  (HalVectorToIRQL[Tpr >> 4])
 #endif
 
+#define APIC_MAX_IRQ 24
+#define APIC_FREE_VECTOR 0xFF
+#define APIC_RESERVED_VECTOR 0xFE
+
 /* The IMCR is supported by two read/writable or write-only I/O ports,
    22h and 23h, which receive address and data respectively.
    To access the IMCR, write a value of 70h to I/O port 22h, which selects the IMCR.
@@ -284,14 +288,14 @@ FORCEINLINE
 ULONG
 ApicRead(ULONG Offset)
 {
-    return *(volatile ULONG *)(APIC_BASE + Offset);
+    return READ_REGISTER_ULONG((PULONG)(APIC_BASE + Offset));
 }
 
 FORCEINLINE
 VOID
 ApicWrite(ULONG Offset, ULONG Value)
 {
-    *(volatile ULONG *)(APIC_BASE + Offset) = Value;
+    WRITE_REGISTER_ULONG((PULONG)(APIC_BASE + Offset), Value);
 }
 
 VOID
diff --git a/hal/halx86/apic/apictimer.c b/hal/halx86/apic/apictimer.c
index 15f87a58499..ec1e0f1ecef 100644
--- a/hal/halx86/apic/apictimer.c
+++ b/hal/halx86/apic/apictimer.c
@@ -61,6 +61,13 @@ ApicInitializeTimer(ULONG Cpu)
 // KeSetTimeIncrement
 }
 
+VOID
+FASTCALL
+HalpProfileInterruptHandler(_In_ PKTRAP_FRAME TrapFrame)
+{
+    KeProfileInterruptWithSource(TrapFrame, ProfileTime);
+}
+
 
 /* PUBLIC FUNCTIONS ***********************************************************/
 
diff --git a/hal/halx86/apic/rtctimer.c b/hal/halx86/apic/rtctimer.c
index 817e6e2de88..2d30ad55c43 100644
--- a/hal/halx86/apic/rtctimer.c
+++ b/hal/halx86/apic/rtctimer.c
@@ -167,13 +167,6 @@ HalpClockInterruptHandler(IN PKTRAP_FRAME TrapFrame)
     KeUpdateSystemTime(TrapFrame, LastIncrement, Irql);
 }
 
-VOID
-FASTCALL
-HalpProfileInterruptHandler(IN PKTRAP_FRAME TrapFrame)
-{
-    __debugbreak();
-}
-
 ULONG
 NTAPI
 HalSetTimeIncrement(IN ULONG Increment)
diff --git a/hal/halx86/apic/tsc.c b/hal/halx86/apic/tsc.c
index ce06b2f6d8e..1ff881206fd 100644
--- a/hal/halx86/apic/tsc.c
+++ b/hal/halx86/apic/tsc.c
@@ -55,8 +55,7 @@ NTAPI
 HalpInitializeTsc(VOID)
 {
     ULONG_PTR Flags;
-    KIDTENTRY OldIdtEntry, *IdtPointer;
-    PKPCR Pcr = KeGetPcr();
+    PVOID PreviousHandler;
     UCHAR RegisterA, RegisterB;
 
     /* Check if the CPU supports RDTSC */
@@ -79,8 +78,7 @@ HalpInitializeTsc(VOID)
     HalpWriteCmos(RTC_REGISTER_A, RegisterA);
 
     /* Save old IDT entry */
-    IdtPointer = KiGetIdtEntry(Pcr, HalpRtcClockVector);
-    OldIdtEntry = *IdtPointer;
+    PreviousHandler = KeQueryInterruptHandler(HalpRtcClockVector);
 
     /* Set the calibration ISR */
     KeRegisterInterruptHandler(HalpRtcClockVector, TscCalibrationISR);
@@ -105,8 +103,8 @@ HalpInitializeTsc(VOID)
     /* Disable the timer interrupt */
     HalDisableSystemInterrupt(HalpRtcClockVector, CLOCK_LEVEL);
 
-    /* Restore old IDT entry */
-    *IdtPointer = OldIdtEntry;
+    /* Restore the previous handler */
+    KeRegisterInterruptHandler(HalpRtcClockVector, PreviousHandler);
 
     /* Calculate an average, using simplified linear regression */
     HalpCpuClockFrequency.QuadPart = DoLinearRegression(NUM_SAMPLES - 1,