On Sat, Jul 30, 2011 at 10:32 PM, Blue Swirl <blauwir...@gmail.com> wrote:
> On Sat, Jul 30, 2011 at 11:19 PM, Artyom Tarasenko <atar4q...@gmail.com>
> wrote:
>> On Sat, Jul 30, 2011 at 3:25 PM, Blue Swirl <blauwir...@gmail.com> wrote:
>>> On Sat, Jul 30, 2011 at 3:31 PM, Artyom Tarasenko <atar4q...@gmail.com>
>>> wrote:
>>>> On Sat, Jul 30, 2011 at 11:09 AM, Blue Swirl <blauwir...@gmail.com> wrote:
>>>>> On Mon, Jul 25, 2011 at 8:22 PM, Artyom Tarasenko <atar4q...@gmail.com>
>>>>> wrote:
>>>>>> clear interrupt request if the interrupt priority < CPU pil
>>>>>> clear hardware interrupt request if interrupts are disabled
>>>>>>
>>>>>> Signed-off-by: Artyom Tarasenko <atar4q...@gmail.com>
>>>>>> ---
>>>>>> 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/
