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

[tsnsaito]

At Thu, 28 Jul 2011 16:44:23 +0200,
Artyom Tarasenko wrote:
> 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:
> >> 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.

Thanks, I understand the reason for introducing a diffent interface
for device interrupts.
It might be difficult to implement the interface as the set_irq callback.

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

----
Tsuneo Saito <address@hidden>