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

[Blue Swirl]

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.

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