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