Re: [Qemu-devel] [PATCH] fix disabling interrupts in sun4u

[Blue Swirl]

On Sat, Jul 30, 2011 at 11:19 PM, Artyom Tarasenko <address@hidden> wrote:
> On Sat, Jul 30, 2011 at 3:25 PM, Blue Swirl <address@hidden> wrote:
>> On Sat, Jul 30, 2011 at 3:31 PM, Artyom Tarasenko <address@hidden> wrote:
>>> On Sat, Jul 30, 2011 at 11:09 AM, Blue Swirl <address@hidden> wrote:
>>>> On Mon, Jul 25, 2011 at 8:22 PM, Artyom Tarasenko <address@hidden> wrote:
>>>>> clear interrupt request if the interrupt priority < CPU pil
>>>>> clear hardware interrupt request if interrupts are disabled
>>>>>
>>>>> Signed-off-by: Artyom Tarasenko <address@hidden>
>>>>> ---
>>>>>  hw/sun4u.c |    6 ++++--
>>>>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/sun4u.c b/hw/sun4u.c
>>>>> index d7dcaf0..7f95aeb 100644
>>>>> --- a/hw/sun4u.c
>>>>> +++ b/hw/sun4u.c
>>>>> @@ -255,7 +255,7 @@ void cpu_check_irqs(CPUState *env)
>>>>>         pil |= 1 << 14;
>>>>>     }
>>>>>
>>>>> -    if (!pil) {
>>>>> +    if (pil < (2 << env->psrpil)){
>>>>
>>>> Sorry, I don't understand the patch. Where is this '2' coming from?
>>>
>>> We shouldn't interrupt at levels <= psrpil. The bit corresponding to
>>> psrpil is (1<< psrpil),
>>> the next bit is (2 << psrpil).
>>
>> Now I see. The check below "i > env->psrpil" does something similar
>> but it doesn't  reset interrupt.
>>
>> How about pil < (1 << (env->psrpil + 1))? I think that makes the
>> purpose clearer.
>
> But it's also one operation more. Shall I just add a comment?

I think the compiler should be able to calculate that 1 << (x + 1) == 2 << x.

>>>>
>>>>>         if (env->interrupt_request & CPU_INTERRUPT_HARD) {
>>>>>             CPUIRQ_DPRINTF("Reset CPU IRQ (current interrupt %x)\n",
>>>>>                            env->interrupt_index);
>>>>> @@ -287,10 +287,12 @@ void cpu_check_irqs(CPUState *env)
>>>>>                 break;
>>>>>             }
>>>>>         }
>>>>> -    } else {
>>>>> +    } else if (env->interrupt_request & CPU_INTERRUPT_HARD) {
>>>>>         CPUIRQ_DPRINTF("Interrupts disabled, pil=%08x pil_in=%08x 
>>>>> softint=%08x "
>>>>>                        "current interrupt %x\n",
>>>>>                        pil, env->pil_in, env->softint, 
>>>>> env->interrupt_index);
>>>>> +        env->interrupt_index = 0;
>>>>> +        cpu_reset_interrupt(env, CPU_INTERRUPT_HARD);
>>>>
>>>> Why reset the index? The idea is that the interrupt is left pending a
>>>> change to PIL etc.
>>>
>>> But it is kept in  env->pil_in and env->softint . Am I missing some
>>> scenario where it's not enough?
>>
>> cpu-exec.c:378 checks interrupt_index, not pil_in etc.
>
> The scenario this patch is trying to fix:
>
> There comes an interrupt N, the PIL is small enough to proceed,
> proceeding disables the interrupts.
> Then the interrupt handler deasserts N and enables the interrupts. The
> interrupt N comes again but is not processed because of old_interrupt
> != new_interrupt check.

Deasserting the interrupt should clear pil_in which should flow down
to interrupt_index. It's a bug if this does not happen.