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

[Blue Swirl]

On Thu, Jul 28, 2011 at 5:44 PM, Artyom Tarasenko <address@hidden> 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:
>>> >> > 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?

d) use interrupts as a bit field, allocate 11 interrupts for each bit
in the vector
e) use a single interrupt but store the vector in the interrupt
controller where CPU can read it when ASI 0x7f is used
f) implement interrupts with message passing capabilities as discussed
earlier with APIC

cpu_receive_ivec(CPUState *env, int vector) would work, but this means
that the interrupt controller needs a pointer to CPUState which is
ugly.

Doing this the other way around like in e) option, 0x7f ASI access
would fetch the vector from controller. This would mean that CPUState
would include a pointer to the interrupt controller. This is not so
clean either but matches i386.

Also, APB is not really the interrupt controller, there is RIC chip in
between on Ultrasparc machines.

f) would be neatest but it's lots of work.

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