[ros-diffs] [reactos] 12/12: [HALX86] Fix the "ASSERT(j < 32); " problem in HalpStoreAndClearIopm() encountered from time to time. CORE-11921 CORE-13715

[Hermès Bélusca-Maïto]

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

commit 5887b170052db6af1ee86dc68f053d0c79d15028
Author:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
AuthorDate: Sun Nov 24 22:58:12 2019 +0100
Commit:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
CommitDate: Mon Nov 25 00:41:55 2019 +0100

    [HALX86] Fix the "ASSERT(j < 32);" problem in HalpStoreAndClearIopm()
encountered from time to time.
    CORE-11921 CORE-13715
    
    (Regression introduced by commit 2e1b82cf, r44841.)
    
    In some cases the number of valid (!= 0xFFFF) entries in the IOPM can be
    larger than the assumed size (32) of the entries cache. The maximum
    possible number of entries is equal to IOPM_SIZE / sizeof(USHORT).
    
    A way to reproduce the problem is as follows: start ReactOS in debugging
    mode using '/DEBUG /DEBUGPORT=SCREEN' . Then manage to break into the
    debugger exactly during the execution of Ke386CallBios() triggered by
    display initialization (for example in my case, while a video driver was
    being initialized via the HwInitialize() call done by videoport inside
    IntVideoPortDispatchOpen() ).
    
    When this happens, a "concurrent" execution between Ke386CallBios() and
    the HAL function HalpStoreAndClearIopm() takes place. This is due to the
    fact that when entering the debugger in SCREEN mode, the following
    call-chain holds:
    InbvResetDisplay() -> VidResetDisplay() -> HalResetDisplay() ->
    HalpBiosDisplayReset() -> HalpSetupRealModeIoPermissionsAndTask() ->
    HalpStoreAndClearIopm().
    
    However, the code of Ke386CallBios() has reset the IOPM contents with
    all zeroes instead of 0xFFFF, and this triggers the caching of all the
    entries of the IOPM by HalpStoreAndClearIopm(), whose number is greater
    than the wrongly assumed number of '32'.
    
    As Thomas explained to me, "Windows supports [the maximum number of IOPM
entries],
    it just makes a full copy of the table instead of this indexed partial copy."
    
    And I agree that this overengineered so-called "optimization" committed
    in 2e1b82cf contributed in introducing an unnecessary bug and making the
    code less clear. Also it makes the IOPM cache larger than the necessary
    size by twice as much. Finally, Ke386CallBios() also caches IOPM entries
    before doing a 16-bit call, and obviously uses the more straightforward
    way of doing a direct copy of the IOPM table (using RtlCopyMemory()).
    
    I wonder what kind of "optimization" this tried to achieve, knowing that
    we are not doing like thousands of 32->16bit BIOS interrupt calls per second
    in ReactOS...
---
 hal/halx86/generic/bios.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hal/halx86/generic/bios.c b/hal/halx86/generic/bios.c
index cc8e57d1721..a922084ca8d 100644
--- a/hal/halx86/generic/bios.c
+++ b/hal/halx86/generic/bios.c
@@ -41,7 +41,7 @@ USHORT HalpSavedTss;
 //
 USHORT HalpSavedIopmBase;
 PUSHORT HalpSavedIoMap;
-USHORT HalpSavedIoMapData[32][2];
+USHORT HalpSavedIoMapData[IOPM_SIZE / sizeof(USHORT)][2];
 ULONG HalpSavedIoMapEntries;
 
 /* Where the protected mode stack is */
@@ -400,7 +400,7 @@ HalpStoreAndClearIopm(VOID)
             //
             // Save it
             //
-            ASSERT(j < 32);
+            ASSERT(j < IOPM_SIZE / sizeof(USHORT));
             HalpSavedIoMapData[j][0] = i;
             HalpSavedIoMapData[j][1] = *Entry;
             j++;