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, [e-mail] 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=544...
> ==============================================================================
> --- 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
> [e-mail]
> 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