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

[Blue Swirl]

On Sat, Jul 30, 2011 at 8:58 PM, Artyom Tarasenko <address@hidden> wrote:
> 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

Same happens on amd64. Well, the compiler isn't so smart as I thought.
Then please use the 2 << version with 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.
>>
>> 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/
>