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

[Artyom Tarasenko]

On Thu, Jul 28, 2011 at 2:03 PM,  <address@hidden> wrote:
> At Thu, 28 Jul 2011 13:51:08 +0200,
> Artyom Tarasenko wrote:
>> On Thu, Jul 28, 2011 at 12:31 PM,  <address@hidden> wrote:
>> > Hi,
>> >
>> > At Mon, 25 Jul 2011 19:22:38 +0200,
>> > Artyom Tarasenko wrote:
>> >
>> >> clear interrupt request if the interrupt priority < CPU pil
>> >> clear hardware interrupt request if interrupts are disabled
>> >
>> > Not directly related to the fix, but I'd like to note a problem
>> > of hw/sun4u.c interrupt code:
>> >
>> > The interrupt code probably mixes hardware interrupts and
>> > software interrupts.
>> > %pil is for software interrupts (interrupt_level_n traps).
>> > %pil can not mask hardware interrupts (interrupt_vector traps);
>> > the CPU raises interrupt_vector traps even on %pil=15.
>> > But in cpu_check_irqs() and cpu_set_irq(), hardware interrupts
>> > seem to be masked by %pil.
>>
>> The interrupt_vector traps are currently not implemented, are they?
>> So it's hard to tell whether they are masked.
>
> Yes, interrupt_vector is not implemented yet.
> I failed to explain the problem.
> The problem is that cpu_set_irqs() should raise interrupt_vector
> traps but it raises interrupt_level_n traps actually.
> sun4uv_init() calls qemu_allocate_irqs() with cpu_set_irq as
> the 1st argument.  The allocated irqs (the irq variable) are
> passed to pci_apb_init().  APB should generate interrupt_vector
> traps (hardware interrupts), not the interrupt_vector_n traps.

Yes, this is true. But it's more complicated than this: cpu_check_irqs
also checks tick/stick/hstick interrupts. They should produce the
interrupt_level_n traps as they currently do.

The patch merely fixes the problem of hanging on a interrupt_vector_n
trap if the trap handler uses pil for interrupt masking. The problem
exists independently from interrupt_vector trap generation (as you
pointed out).

Do you have objections to this patch in its current form?

> The interrupts from APB would be reported by cpu_set_irq(),
> but cpu_set_irq() seems to generate interrupt_vector_n traps.

For me it's not obvious. The interrupt vector not just one line, but
the vector, which is written in the corresponding CPU register (also
missing in the current qemu implementation). On the real hardware the
vector is created by the IOMMU (PBM/APB/...). If qemu intends to
support multiple chipsets, we should keep it the way it's done on the
real hardware (for instance the interrupt vectors for on-board devices
on Ultra-1 and E6500 are not the same).

I'd suggest APB shall use some other interface for communicating
interrupts to the CPU. Something like
cpu_receive_ivec(interrupt_vector).

>> >> 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)){
>> >>          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);
>> >>      }
>> >>  }
>> >>
>> >> --
>> >> 1.7.3.4
>> >
>> >
>> > ----
>> > Tsuneo Saito <address@hidden>
>> >
>>
>>
>>
>> --
>> Regards,
>> Artyom Tarasenko
>>
>> solaris/sparc under qemu blog: http://tyom.blogspot.com/
>
> ----
> Tsuneo Saito <address@hidden>
>

-- 
Regards,
Artyom Tarasenko

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