Re: [ros-dev] [ros-diffs] [cgutman] 54418: [HALX86] - Do not allow software interrupts to preempt code running with interrupts disabled during KfLowerIrql

[Aleksey Bragin]

I would like to ask our kernel gurus if this one is correct.

WBR,
Aleksey Bragin.

On 18.11.2011 22:53, cgutman(a)svn.reactos.org wrote:
>   Author: cgutman
>  Date: Fri Nov 18 18:53:41 2011
>  New Revision: 54418
>
>  URL: http://svn.reactos.org/svn/reactos?rev=54418&view=rev
>  Log:
>  [HALX86]
>  - Do not allow software interrupts to preempt code running with interrupts disabled
> during KfLowerIrql
>
>  Modified:
>       trunk/reactos/hal/halx86/up/pic.c
>
>  Modified: trunk/reactos/hal/halx86/up/pic.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/hal/halx86/up/pic.c?rev=54…
>  ==============================================================================
>  --- trunk/reactos/hal/halx86/up/pic.c [iso-8859-1] (original)
>  +++ trunk/reactos/hal/halx86/up/pic.c [iso-8859-1] Fri Nov 18 18:53:41 2011
>  @@ -665,26 +665,30 @@
>
>        /* Set old IRQL */
>        Pcr->Irql = OldIrql;
>  -
>  -    /* Check for pending software interrupts and compare with current IRQL */
>  -    PendingIrqlMask = Pcr->IRR&  FindHigherIrqlMask[OldIrql];
>  -    if (PendingIrqlMask)
>  -    {
>  -        /* Check if pending IRQL affects hardware state */
>  -        BitScanReverse(&PendingIrql, PendingIrqlMask);
>  -        if (PendingIrql>  DISPATCH_LEVEL)
>  +
>  +    /* Make sure interrupts were enabled */
>  +    if (EFlags&  EFLAGS_INTERRUPT_MASK)
>  +    {
>  +        /* Check for pending software interrupts and compare with current IRQL */
>  +        PendingIrqlMask = Pcr->IRR&  FindHigherIrqlMask[OldIrql];
>  +        if (PendingIrqlMask)
>            {
>  -            /* Set new PIC mask */
>  -            Mask.Both = Pcr->IDR;
>  -            __outbyte(PIC1_DATA_PORT, Mask.Master);
>  -            __outbyte(PIC2_DATA_PORT, Mask.Slave);
>  -
>  -            /* Clear IRR bit */
>  -            Pcr->IRR ^= (1<<  PendingIrql);
>  +            /* Check if pending IRQL affects hardware state */
>  +            BitScanReverse(&PendingIrql, PendingIrqlMask);
>  +            if (PendingIrql>  DISPATCH_LEVEL)
>  +            {
>  +                /* Set new PIC mask */
>  +                Mask.Both = Pcr->IDR;
>  +                __outbyte(PIC1_DATA_PORT, Mask.Master);
>  +                __outbyte(PIC2_DATA_PORT, Mask.Slave);
>  +
>  +                /* Clear IRR bit */
>  +                Pcr->IRR ^= (1<<  PendingIrql);
>  +            }
>  +
>  +            /* Now handle pending interrupt */
>  +            SWInterruptHandlerTable[PendingIrql]();
>            }
>  -
>  -        /* Now handle pending interrupt */
>  -        SWInterruptHandlerTable[PendingIrql]();
>        }
>
>        /* Restore interrupt state */
>
>

[Cameron Gutman]

>   I would like to ask our kernel gurus if this one is
> correct.
>   
Timo says it is correct. 

Let me also explain the problem that I fixed:

So basically, I asked Sylvain to help me test a patch for supporting PS/2 hotplugging. I
had added an ASSERT(FALSE) in the ISR when we got the magic packet that indicated a
reconnect. But when that was executed, something very strange happened. So the assertion
was triggered when Value == 0xAA. Then, after the assertion, it would jump down to the
reset code and check Value again. This time Value == 0x00, yet nothing set Value between
the assertion and the 2nd check. After a closer look, I found that the ISR had actually
ran again AFTER Kdbg was entered via the assertion. The reason was that Kdbg calls
_disable() then lowers the IRQL to PASSIVE_LEVEL. This triggered all pending software
interrupts (APCs, DPCs, and more), which would preempt Kdbg even while running with
interrupts off, which Kdbg obviously did not expect. Upon discussion with Timo, it appears
that the whole software interrupts thing is basically a hack for the standard PIC. x64,
APIC, and other architectures have no need for this because they use real interrupts
instead of emulated software interrupts.

Hopefully that clears things up a bit.


Regards,
Cameron

[Alex Ionescu]

Timo always says everything is correct.

Unfortunately this is not the case.

_disable and setting IRQL to PASSIVE_LEVEL makes no sense whatsoever. You basically hacked
the HAL to support a bug in KDBG. Good job.

--
Best regards,
Alex Ionescu

On 2011-11-19, at 6:50 AM, Cameron Gutman wrote:

>   
> >   I would like to ask our kernel gurus if this one
> > is correct.
> >   
>  
>  Timo says it is correct. 
>  
>  Let me also explain the problem that I fixed:
>  
>  So basically, I asked Sylvain to help me test a patch for supporting PS/2 hotplugging. I
> had added an ASSERT(FALSE) in the ISR when we got the magic packet that indicated a
> reconnect. But when that was executed, something very strange happened. So the assertion
> was triggered when Value == 0xAA. Then, after the assertion, it would jump down to the
> reset code and check Value again. This time Value == 0x00, yet nothing set Value between
> the assertion and the 2nd check. After a closer look, I found that the ISR had actually
> ran again AFTER Kdbg was entered via the assertion. The reason was that Kdbg calls
> _disable() then lowers the IRQL to PASSIVE_LEVEL. This triggered all pending software
> interrupts (APCs, DPCs, and more), which would preempt Kdbg even while running with
> interrupts off, which Kdbg obviously did not expect. Upon discussion with Timo, it appears
> that the whole software interrupts thing is basically a hack for the standard PIC. x64,
> APIC, and other architectures have no need for this because they use real interrupts
> instead of emulated software interrupts.
>  
>  Hopefully that clears things up a bit.
>  
>  
>  Regards,
>  Cameron
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev

[Cameron Gutman]

>   
>  _disable and setting IRQL to PASSIVE_LEVEL makes no sense whatsoever. You basically
> hacked the HAL to support a bug in KDBG. Good job.
>   
I gathered that what I was supporting was pretty nasty, but either way the behavior that
was going on was completely wrong. I'm fairly sure there are other misuses of
_disable() (and KeLowerIrql) that could result in weird behavior. I'll review them
when I get some time. KDBG certainly needs to be fixed to avoid needing to manually lower
IRQL.


Regards,
Cameron