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

[Artyom Tarasenko]

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?

>>>
>>>>         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.

-- 
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/