Re: [ros-dev] [ros-diffs] [tfaber] 71307: [HAL] - Create a wrapper version of HalpEndSoftwareInterrupt that frees its stack before calling the noreturn version of the next software interrupt handler. Fixes excessive stack u...

[Alex Ionescu]

Wait, are you saying that Timo didn't discover this obvious bug for years?
Best regards,
Alex Ionescu
On Mon, May 16, 2016 at 4:06 PM, Timo Kreuzer <timo.kreuzer(a)web.de> wrote:
>   Wait, are you suggesting that "doing everything
> the same as MS does" and
>  "I don't need to care for C standards, I have a huge epeen!" and
>  "OnlyOnePersonCanWriteHalCode AndItsNotYou".... doesn't work? :O
>  Best regards,
>  Timo Kreuzer
>  Am 10.05.2016 um 17:03 schrieb tfaber(a)svn.reactos.org:
> >  Author: tfaber
> >  Date: Tue May 10 15:03:56 2016
> >  New Revision: 71307
> >  URL: http://svn.reactos.org/svn/reactos?rev=71307&view=rev
> >  Log:
> >  [HAL]
> >  - Create a wrapper version of HalpEndSoftwareInterrupt that frees its
> >  stack before calling the noreturn version of the next software interrupt
> >  handler. Fixes excessive stack usage when DPCs are queued in quick
> >  succession.
> >  CORE-11123 #resolve
> >  Added:
> >       trunk/reactos/hal/halx86/up/pic.S   (with props)
> >  Modified:
> >       trunk/reactos/hal/halx86/pic.cmake
> >       trunk/reactos/hal/halx86/up/pic.c
> >  Modified: trunk/reactos/hal/halx86/pic.cmake
> >  URL:
> > http://svn.reactos.org/svn/reactos/trunk/reactos/hal/halx86/pic.cmake?rev=7…
> >  ==============================================================================
> >  --- trunk/reactos/hal/halx86/pic.cmake  [iso-8859-1] (original)
> >  +++ trunk/reactos/hal/halx86/pic.cmake  [iso-8859-1] Tue May 10 15:03:56
> >  2016
> >  @@ -1,7 +1,8 @@
> >      list(APPEND HAL_PIC_ASM_SOURCE
> >        generic/systimer.S
> >  -    generic/trap.S)
> >  +    generic/trap.S
> >  +    up/pic.S)
> >      list(APPEND HAL_PIC_SOURCE
> >        generic/profil.c
> >  Added: trunk/reactos/hal/halx86/up/pic.S
> >  URL:
> >  http://svn.reactos.org/svn/reactos/trunk/reactos/hal/halx86/up/pic.S?rev=71…
> >  ==============================================================================
> >  --- trunk/reactos/hal/halx86/up/pic.S   (added)
> >  +++ trunk/reactos/hal/halx86/up/pic.S   [iso-8859-1] Tue May 10 15:03:56
> >  2016
> >  @@ -0,0 +1,48 @@
> >  +/*
> >  + * FILE:            hal/halx86/up/pic.S
> >  + * COPYRIGHT:       See COPYING in the top level directory
> >  + * PURPOSE:         HAL PIC Management and Control Code
> >  + * PROGRAMMER:      Thomas Faber (thomas.faber(a)reactos.org)
> >  + */
> >  +
> >  +/* INCLUDES
> >  ******************************************************************/
> >  +
> >  +#include <asm.inc>
> >  +
> >  +#include <ks386.inc>
> >  +
> >  +EXTERN _HalpEndSoftwareInterrupt2@8:PROC
> >  +
> >  +/* GLOBALS
> >  *******************************************************************/
> >  +
> >  +.data
> >  +ASSUME CS:NOTHING, DS:NOTHING, ES:NOTHING, FS:NOTHING, GS:NOTHING
> >  +
> >  +/* FUNCTIONS
> >  *****************************************************************/
> >  +
> >  +.code
> >  +PUBLIC _HalpEndSoftwareInterrupt@8
> >  +.PROC _HalpEndSoftwareInterrupt@8
> >  +    FPO 0, 2, 0, 0, 0, FRAME_FPO
> >  +
> >  +    /* Call the C function with the same arguments we got */
> >  +    push [esp+8]
> >  +    push [esp+8]
> >  +    call _HalpEndSoftwareInterrupt2@8
> >  +
> >  +    /* Check if we got a pointer back */
> >  +    test eax, eax
> >  +    jnz CallIntHandler
> >  +
> >  +    /* No? Just return */
> >  +    ret 8
> >  +
> >  +CallIntHandler:
> >  +    /* We got a pointer to call. Since it won't return, free up our stack
> >  +       space, or we could end up with some nasty deep recursion */
> >  +    mov ecx, [esp+8]
> >  +    add esp, 12
> >  +    jmp eax
> >  +.ENDP
> >  +
> >  +END
> >  Propchange: trunk/reactos/hal/halx86/up/pic.S
> >  ------------------------------------------------------------------------------
> >       svn:eol-style = native
> >  Modified: trunk/reactos/hal/halx86/up/pic.c
> >  URL:
> > http://svn.reactos.org/svn/reactos/trunk/reactos/hal/halx86/up/pic.c?rev=71…
> >  ==============================================================================
> >  --- trunk/reactos/hal/halx86/up/pic.c   [iso-8859-1] (original)
> >  +++ trunk/reactos/hal/halx86/up/pic.c   [iso-8859-1] Tue May 10 15:03:56
> >  2016
> >  @@ -11,6 +11,11 @@
> >    #include <hal.h>
> >    #define NDEBUG
> >    #include <debug.h>
> >  +
> >  +VOID
> >  +NTAPI
> >  +HalpEndSoftwareInterrupt(IN KIRQL OldIrql,
> >  +                         IN PKTRAP_FRAME TrapFrame);
> >      /* GLOBALS
> >  ********************************************************************/
> >    @@ -263,7 +268,7 @@
> >         * so it will always preempt until we reach PROFILE_LEVEL.
> >         */
> >        0b00000000000000000001011111110000, /* IRQL 20 */
> >  -    0b00000000000000000001001111110000, /* IRQL 20 */
> >  +    0b00000000000000000001001111110000, /* IRQL 21 */
> >        0b00000000000000000001000111110000, /* IRQL 22 */
> >        0b00000000000000000001000011110000, /* IRQL 23 */
> >        0b00000000000000000001000001110000, /* IRQL 24 */
> >  @@ -732,15 +737,17 @@
> >        KeGetPcr()->IRR &= ~(1 << Irql);
> >    }
> >    -VOID
> >  -NTAPI
> >  -HalpEndSoftwareInterrupt(IN KIRQL OldIrql,
> >  -                         IN PKTRAP_FRAME TrapFrame)
> >  +PHAL_SW_INTERRUPT_HANDLER_2ND_ENTRY
> >  +NTAPI
> >  +HalpEndSoftwareInterrupt2(IN KIRQL OldIrql,
> >  +                          IN PKTRAP_FRAME TrapFrame)
> >    {
> >        ULONG PendingIrql, PendingIrqlMask, PendingIrqMask;
> >        PKPCR Pcr = KeGetPcr();
> >        PIC_MASK Mask;
> >    +    UNREFERENCED_PARAMETER(TrapFrame);
> >  +
> >        /* Set old IRQL */
> >        Pcr->Irql = OldIrql;
> >    @@ -749,10 +756,10 @@
> >        {
> >            /* Check for pending software interrupts and compare with
> >  current IRQL */
> >            PendingIrqlMask = Pcr->IRR & FindHigherIrqlMask[OldIrql];
> >  -        if (!PendingIrqlMask) return;
> >  +        if (!PendingIrqlMask) return NULL;
> >              /* Check for in-service delayed interrupt */
> >  -        if (Pcr->IrrActive & 0xFFFFFFF0) return;
> >  +        if (Pcr->IrrActive & 0xFFFFFFF0) return NULL;
> >              /* Check if pending IRQL affects hardware state */
> >            BitScanReverse(&PendingIrql, PendingIrqlMask);
> >  @@ -777,10 +784,11 @@
> >            else
> >            {
> >                /* No need to loop checking for hardware interrupts */
> >  -            SWInterruptHandlerTable2[PendingIrql](TrapFrame);
> >  -            UNREACHABLE;
> >  +            return SWInterruptHandlerTable2[PendingIrql];
> >            }
> >        }
> >  +
> >  +    return NULL;
> >    }
> >      /* EDGE INTERRUPT DISMISSAL FUNCTIONS
> >  *****************************************/
> >  
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev