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

[Artyom Tarasenko]

On Sat, Jul 30, 2011 at 10:32 PM, Blue Swirl <address@hidden> wrote:
> 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.
>
Are you sure?

$ cat test_smart_shift.c
int main (int argc, char *argv[]) {
 return 1 << (1+argc);
}

$ sparc64-linux-gnu-gcc-4.4.5 -O2 -S  test_smart_shift.c
  cat test_smart_shift.s
        .file   "test_smart_shift.c"
        .section        ".text"
        .align 4
        .align 32
        .global main
        .type   main, #function
        .proc   04
main:
        mov     1, %g1
        add     %o0, 1, %o0
        sll     %g1, %o0, %o0
        jmp     %o7+8
         sra    %o0, 0, %o0
        .size   main, .-main
        .ident  "GCC: (GNU) 4.4.5"
        .section        .note.GNU-stack,"",@progbits


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

Ok, I'll drop the second part in v2 then.


-- 
Regards,
Artyom Tarasenko

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