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

[Artyom Tarasenko]

On Thu, Jul 28, 2011 at 3:40 PM,  <address@hidden> wrote:
> At Thu, 28 Jul 2011 14:50:57 +0200,
> Artyom Tarasenko wrote:
>> 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.
>
> That's right.
> tick/stick/hstick must raise interrupt_level_n traps.
>
>> 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).
>
> I understand what is the problem that your patch is going to fix.
> Thanks for the explanation.
>

Sorry, I haven't implied that you don't. Meant to say "As you pointed
out, the problem exists independently...".

>> Do you have objections to this patch in its current form?
>
> No, I don't have any objections.
>
>> > 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).
>
> Sorry, I can't keep up with this vector thing...
> Does the CPU receive hardware interrupts as interrupt_vector traps
> (trap type=0x60) regardless of the kind of the interrupt controller,
> doesn't it?

It does indeed, but it also stores the interrupt vector identifying
the initiator device, in a CPU register readable with asi 0x7f .
What would APB pass to the cpu_set_irq? I see the three following variants:

a) it passes the PCI interrupt id, which is translated to the
interrupt vector in cpu_set_irq()
b) it passes the vector. This implies that 2048 (0-0x7ff) CPU
interrupts have to be allocated.
c) hack combining a+b: allocate only the interrupts known to be used
and translate an internal interrupt id to a vector.

The variant "a" is bad because it doesn't allow support for different
chipsets. The variant "b" is bad because qemu has to allocate way too
many interrupts. Only few of them will be used actually. The variant
"c" is bad, well, because it's a hack.

That's why I suggest using another interface between APB and CPU.

Have I overlooked something?

>> 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/
>
> ----
> Tsuneo Saito <address@hidden>
>



-- 
Regards,
Artyom Tarasenko

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