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

[tsnsaito]

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.

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

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