| Message ID | 316f198de781f7b819456f433d84ead284769a71.1311606610.git.atar4qemu@gmail.com |
|---|---|
| State | New |
| Headers | show |
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. > Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> > --- > 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 <tsnsaito@gmail.com>
Hi, On Thu, Jul 28, 2011 at 12:31 PM, <tsnsaito@gmail.com> 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. >> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> >> --- >> 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 <tsnsaito@gmail.com> >
At Thu, 28 Jul 2011 13:51:08 +0200, Artyom Tarasenko wrote: > On Thu, Jul 28, 2011 at 12:31 PM, <tsnsaito@gmail.com> 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. The interrupts from APB would be reported by cpu_set_irq(), but cpu_set_irq() seems to generate interrupt_vector_n traps. > >> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> > >> --- > >> 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 <tsnsaito@gmail.com> > > > > > > -- > Regards, > Artyom Tarasenko > > solaris/sparc under qemu blog: http://tyom.blogspot.com/ ---- Tsuneo Saito <tsnsaito@gmail.com>
2011/7/28 <tsnsaito@gmail.com>: > > At Thu, 28 Jul 2011 13:51:08 +0200, > Artyom Tarasenko wrote: > > On Thu, Jul 28, 2011 at 12:31 PM, <tsnsaito@gmail.com> 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. Ah, I intended to write: No, interrupt_vector is not implemented yet. I'm very sorry for my engrish :-( > 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. > The interrupts from APB would be reported by cpu_set_irq(), > but cpu_set_irq() seems to generate interrupt_vector_n traps. > > > >> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> > > >> --- > > >> 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 <tsnsaito@gmail.com> > > > > > > > > > > > -- > > Regards, > > Artyom Tarasenko > > > > solaris/sparc under qemu blog: http://tyom.blogspot.com/ > > ---- > Tsuneo Saito <tsnsaito@gmail.com>
On Thu, Jul 28, 2011 at 2:03 PM, <tsnsaito@gmail.com> wrote: > At Thu, 28 Jul 2011 13:51:08 +0200, > Artyom Tarasenko wrote: >> On Thu, Jul 28, 2011 at 12:31 PM, <tsnsaito@gmail.com> 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 <atar4qemu@gmail.com> >> >> --- >> >> 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 <tsnsaito@gmail.com> >> > >> >> >> >> -- >> Regards, >> Artyom Tarasenko >> >> solaris/sparc under qemu blog: http://tyom.blogspot.com/ > > ---- > Tsuneo Saito <tsnsaito@gmail.com> >
On Thu, Jul 28, 2011 at 2:10 PM, Tsuneo Saito <tsnsaito@gmail.com> wrote: > 2011/7/28 <tsnsaito@gmail.com>: >> >> At Thu, 28 Jul 2011 13:51:08 +0200, >> Artyom Tarasenko wrote: >> > On Thu, Jul 28, 2011 at 12:31 PM, <tsnsaito@gmail.com> 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. > > Ah, I intended to write: > No, interrupt_vector is not implemented yet. > I'm very sorry for my engrish :-( Not having English as my native language, I even haven't noticed something wrong here. Despite what they say the International language is not English, but broken English. :) >> 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. >> The interrupts from APB would be reported by cpu_set_irq(), >> but cpu_set_irq() seems to generate interrupt_vector_n traps. >> >> > >> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> >> > >> --- >> > >> 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 <tsnsaito@gmail.com> >> > > >> > >> > >> > >> > -- >> > Regards, >> > Artyom Tarasenko >> > >> > solaris/sparc under qemu blog: http://tyom.blogspot.com/ >> >> ---- >> Tsuneo Saito <tsnsaito@gmail.com> >
At Thu, 28 Jul 2011 14:50:57 +0200, Artyom Tarasenko wrote: > On Thu, Jul 28, 2011 at 2:03 PM, <tsnsaito@gmail.com> wrote: > > At Thu, 28 Jul 2011 13:51:08 +0200, > > Artyom Tarasenko wrote: > >> On Thu, Jul 28, 2011 at 12:31 PM, <tsnsaito@gmail.com> 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 <atar4qemu@gmail.com> > >> >> --- > >> >> 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 <tsnsaito@gmail.com> > >> > > >> > >> > >> > >> -- > >> Regards, > >> Artyom Tarasenko > >> > >> solaris/sparc under qemu blog: http://tyom.blogspot.com/ > > > > ---- > > Tsuneo Saito <tsnsaito@gmail.com> > > > > -- > Regards, > Artyom Tarasenko > > solaris/sparc under qemu blog: http://tyom.blogspot.com/ ---- Tsuneo Saito <tsnsaito@gmail.com>
On Thu, Jul 28, 2011 at 3:40 PM, <tsnsaito@gmail.com> wrote: > At Thu, 28 Jul 2011 14:50:57 +0200, > Artyom Tarasenko wrote: >> On Thu, Jul 28, 2011 at 2:03 PM, <tsnsaito@gmail.com> wrote: >> > At Thu, 28 Jul 2011 13:51:08 +0200, >> > Artyom Tarasenko wrote: >> >> On Thu, Jul 28, 2011 at 12:31 PM, <tsnsaito@gmail.com> 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 <atar4qemu@gmail.com> >> >> >> --- >> >> >> 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 <tsnsaito@gmail.com> >> >> > >> >> >> >> >> >> >> >> -- >> >> Regards, >> >> Artyom Tarasenko >> >> >> >> solaris/sparc under qemu blog: http://tyom.blogspot.com/ >> > >> > ---- >> > Tsuneo Saito <tsnsaito@gmail.com> >> > >> >> -- >> Regards, >> Artyom Tarasenko >> >> solaris/sparc under qemu blog: http://tyom.blogspot.com/ > > ---- > Tsuneo Saito <tsnsaito@gmail.com> >
At Thu, 28 Jul 2011 16:44:23 +0200, Artyom Tarasenko wrote: > On Thu, Jul 28, 2011 at 3:40 PM, <tsnsaito@gmail.com> wrote: > > At Thu, 28 Jul 2011 14:50:57 +0200, > > Artyom Tarasenko wrote: > >> On Thu, Jul 28, 2011 at 2:03 PM, <tsnsaito@gmail.com> wrote: > >> > At Thu, 28 Jul 2011 13:51:08 +0200, > >> > Artyom Tarasenko wrote: > >> >> On Thu, Jul 28, 2011 at 12:31 PM, <tsnsaito@gmail.com> 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 <atar4qemu@gmail.com> > >> >> >> --- > >> >> >> 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 <tsnsaito@gmail.com> > >> >> > > >> >> > >> >> > >> >> > >> >> -- > >> >> Regards, > >> >> Artyom Tarasenko > >> >> > >> >> solaris/sparc under qemu blog: http://tyom.blogspot.com/ > >> > > >> > ---- > >> > Tsuneo Saito <tsnsaito@gmail.com> > >> > > >> > >> -- > >> Regards, > >> Artyom Tarasenko > >> > >> solaris/sparc under qemu blog: http://tyom.blogspot.com/ > > > > ---- > > Tsuneo Saito <tsnsaito@gmail.com> > > > > > > -- > Regards, > Artyom Tarasenko > > solaris/sparc under qemu blog: http://tyom.blogspot.com/ ---- Tsuneo Saito <tsnsaito@gmail.com>
On Mon, Jul 25, 2011 at 8:22 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote: > clear interrupt request if the interrupt priority < CPU pil > clear hardware interrupt request if interrupts are disabled > > Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> > --- > 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)){ Sorry, I don't understand the patch. Where is this '2' coming from? > 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); Why reset the index? The idea is that the interrupt is left pending a change to PIL etc. > } > } > > -- > 1.7.3.4 > >
On Thu, Jul 28, 2011 at 5:44 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote: > On Thu, Jul 28, 2011 at 3:40 PM, <tsnsaito@gmail.com> wrote: >> At Thu, 28 Jul 2011 14:50:57 +0200, >> Artyom Tarasenko wrote: >>> On Thu, Jul 28, 2011 at 2:03 PM, <tsnsaito@gmail.com> wrote: >>> > At Thu, 28 Jul 2011 13:51:08 +0200, >>> > Artyom Tarasenko wrote: >>> >> On Thu, Jul 28, 2011 at 12:31 PM, <tsnsaito@gmail.com> 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 <atar4qemu@gmail.com> >>> >> >> --- >>> >> >> 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 <tsnsaito@gmail.com> >>> >> > >>> >> >>> >> >>> >> >>> >> -- >>> >> Regards, >>> >> Artyom Tarasenko >>> >> >>> >> solaris/sparc under qemu blog: http://tyom.blogspot.com/ >>> > >>> > ---- >>> > Tsuneo Saito <tsnsaito@gmail.com> >>> > >>> >>> -- >>> Regards, >>> Artyom Tarasenko >>> >>> solaris/sparc under qemu blog: http://tyom.blogspot.com/ >> >> ---- >> Tsuneo Saito <tsnsaito@gmail.com> >> > > > > -- > Regards, > Artyom Tarasenko > > solaris/sparc under qemu blog: http://tyom.blogspot.com/ >
On Sat, Jul 30, 2011 at 11:09 AM, Blue Swirl <blauwirbel@gmail.com> wrote: > On Mon, Jul 25, 2011 at 8:22 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote: >> clear interrupt request if the interrupt priority < CPU pil >> clear hardware interrupt request if interrupts are disabled >> >> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> >> --- >> 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)){ > > Sorry, I don't understand the patch. Where is this '2' coming from? We shouldn't interrupt at levels <= psrpil. The bit corresponding to psrpil is (1<< psrpil), the next bit is (2 << 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); > > Why reset the index? The idea is that the interrupt is left pending a > change to PIL etc. But it is kept in env->pil_in and env->softint . Am I missing some scenario where it's not enough? >> } >> } >> >> -- >> 1.7.3.4 >> >> >
On Sat, Jul 30, 2011 at 3:31 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote: > On Sat, Jul 30, 2011 at 11:09 AM, Blue Swirl <blauwirbel@gmail.com> wrote: >> On Mon, Jul 25, 2011 at 8:22 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote: >>> clear interrupt request if the interrupt priority < CPU pil >>> clear hardware interrupt request if interrupts are disabled >>> >>> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> >>> --- >>> 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)){ >> >> Sorry, I don't understand the patch. Where is this '2' coming from? > > We shouldn't interrupt at levels <= psrpil. The bit corresponding to > psrpil is (1<< psrpil), > the next bit is (2 << psrpil). Now I see. The check below "i > env->psrpil" does something similar but it doesn't reset interrupt. How about pil < (1 << (env->psrpil + 1))? I think that makes the purpose clearer. >> >>> 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); >> >> Why reset the index? The idea is that the interrupt is left pending a >> change to PIL etc. > > But it is kept in env->pil_in and env->softint . Am I missing some > scenario where it's not enough? cpu-exec.c:378 checks interrupt_index, not pil_in etc.
On Sat, Jul 30, 2011 at 3:25 PM, Blue Swirl <blauwirbel@gmail.com> wrote: > On Sat, Jul 30, 2011 at 3:31 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote: >> On Sat, Jul 30, 2011 at 11:09 AM, Blue Swirl <blauwirbel@gmail.com> wrote: >>> On Mon, Jul 25, 2011 at 8:22 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote: >>>> clear interrupt request if the interrupt priority < CPU pil >>>> clear hardware interrupt request if interrupts are disabled >>>> >>>> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> >>>> --- >>>> 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)){ >>> >>> Sorry, I don't understand the patch. Where is this '2' coming from? >> >> We shouldn't interrupt at levels <= psrpil. The bit corresponding to >> psrpil is (1<< psrpil), >> the next bit is (2 << psrpil). > > Now I see. The check below "i > env->psrpil" does something similar > but it doesn't reset interrupt. > > How about pil < (1 << (env->psrpil + 1))? I think that makes the > purpose clearer. But it's also one operation more. Shall I just add a comment? >>> >>>> 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); >>> >>> Why reset the index? The idea is that the interrupt is left pending a >>> change to PIL etc. >> >> But it is kept in env->pil_in and env->softint . Am I missing some >> scenario where it's not enough? > > cpu-exec.c:378 checks interrupt_index, not pil_in etc. The scenario this patch is trying to fix: There comes an interrupt N, the PIL is small enough to proceed, proceeding disables the interrupts. Then the interrupt handler deasserts N and enables the interrupts. The interrupt N comes again but is not processed because of old_interrupt != new_interrupt check.
On Sat, Jul 30, 2011 at 11:19 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote: > On Sat, Jul 30, 2011 at 3:25 PM, Blue Swirl <blauwirbel@gmail.com> wrote: >> On Sat, Jul 30, 2011 at 3:31 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote: >>> On Sat, Jul 30, 2011 at 11:09 AM, Blue Swirl <blauwirbel@gmail.com> wrote: >>>> On Mon, Jul 25, 2011 at 8:22 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote: >>>>> clear interrupt request if the interrupt priority < CPU pil >>>>> clear hardware interrupt request if interrupts are disabled >>>>> >>>>> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> >>>>> --- >>>>> 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)){ >>>> >>>> Sorry, I don't understand the patch. Where is this '2' coming from? >>> >>> We shouldn't interrupt at levels <= psrpil. The bit corresponding to >>> psrpil is (1<< psrpil), >>> the next bit is (2 << psrpil). >> >> Now I see. The check below "i > env->psrpil" does something similar >> but it doesn't reset interrupt. >> >> How about pil < (1 << (env->psrpil + 1))? I think that makes the >> purpose clearer. > > But it's also one operation more. Shall I just add a comment? I think the compiler should be able to calculate that 1 << (x + 1) == 2 << x. >>>> >>>>> 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); >>>> >>>> Why reset the index? The idea is that the interrupt is left pending a >>>> change to PIL etc. >>> >>> But it is kept in env->pil_in and env->softint . Am I missing some >>> scenario where it's not enough? >> >> cpu-exec.c:378 checks interrupt_index, not pil_in etc. > > The scenario this patch is trying to fix: > > There comes an interrupt N, the PIL is small enough to proceed, > proceeding disables the interrupts. > Then the interrupt handler deasserts N and enables the interrupts. The > interrupt N comes again but is not processed because of old_interrupt > != new_interrupt check. Deasserting the interrupt should clear pil_in which should flow down to interrupt_index. It's a bug if this does not happen.
On Sat, Jul 30, 2011 at 10:32 PM, Blue Swirl <blauwirbel@gmail.com> wrote: > On Sat, Jul 30, 2011 at 11:19 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote: >> On Sat, Jul 30, 2011 at 3:25 PM, Blue Swirl <blauwirbel@gmail.com> wrote: >>> On Sat, Jul 30, 2011 at 3:31 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote: >>>> On Sat, Jul 30, 2011 at 11:09 AM, Blue Swirl <blauwirbel@gmail.com> wrote: >>>>> On Mon, Jul 25, 2011 at 8:22 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote: >>>>>> clear interrupt request if the interrupt priority < CPU pil >>>>>> clear hardware interrupt request if interrupts are disabled >>>>>> >>>>>> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> >>>>>> --- >>>>>> 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)){ >>>>> >>>>> Sorry, I don't understand the patch. Where is this '2' coming from? >>>> >>>> We shouldn't interrupt at levels <= psrpil. The bit corresponding to >>>> psrpil is (1<< psrpil), >>>> the next bit is (2 << psrpil). >>> >>> Now I see. The check below "i > env->psrpil" does something similar >>> but it doesn't reset interrupt. >>> >>> How about pil < (1 << (env->psrpil + 1))? I think that makes the >>> purpose clearer. >> >> But it's also one operation more. Shall I just add a comment? > > I think the compiler should be able to calculate that 1 << (x + 1) == 2 << x. > Are you sure? $ cat test_smart_shift.c int main (int argc, char *argv[]) { return 1 << (1+argc); } $ sparc64-linux-gnu-gcc-4.4.5 -O2 -S test_smart_shift.c cat test_smart_shift.s .file "test_smart_shift.c" .section ".text" .align 4 .align 32 .global main .type main, #function .proc 04 main: mov 1, %g1 add %o0, 1, %o0 sll %g1, %o0, %o0 jmp %o7+8 sra %o0, 0, %o0 .size main, .-main .ident "GCC: (GNU) 4.4.5" .section .note.GNU-stack,"",@progbits >>>>> >>>>>> 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); >>>>> >>>>> Why reset the index? The idea is that the interrupt is left pending a >>>>> change to PIL etc. >>>> >>>> But it is kept in env->pil_in and env->softint . Am I missing some >>>> scenario where it's not enough? >>> >>> cpu-exec.c:378 checks interrupt_index, not pil_in etc. >> >> The scenario this patch is trying to fix: >> >> There comes an interrupt N, the PIL is small enough to proceed, >> proceeding disables the interrupts. >> Then the interrupt handler deasserts N and enables the interrupts. The >> interrupt N comes again but is not processed because of old_interrupt >> != new_interrupt check. > > Deasserting the interrupt should clear pil_in which should flow down > to interrupt_index. It's a bug if this does not happen. Ok, I'll drop the second part in v2 then.
On Sat, Jul 30, 2011 at 8:58 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote: > On Sat, Jul 30, 2011 at 10:32 PM, Blue Swirl <blauwirbel@gmail.com> wrote: >> On Sat, Jul 30, 2011 at 11:19 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote: >>> On Sat, Jul 30, 2011 at 3:25 PM, Blue Swirl <blauwirbel@gmail.com> wrote: >>>> On Sat, Jul 30, 2011 at 3:31 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote: >>>>> On Sat, Jul 30, 2011 at 11:09 AM, Blue Swirl <blauwirbel@gmail.com> wrote: >>>>>> On Mon, Jul 25, 2011 at 8:22 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote: >>>>>>> clear interrupt request if the interrupt priority < CPU pil >>>>>>> clear hardware interrupt request if interrupts are disabled >>>>>>> >>>>>>> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> >>>>>>> --- >>>>>>> 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)){ >>>>>> >>>>>> Sorry, I don't understand the patch. Where is this '2' coming from? >>>>> >>>>> We shouldn't interrupt at levels <= psrpil. The bit corresponding to >>>>> psrpil is (1<< psrpil), >>>>> the next bit is (2 << psrpil). >>>> >>>> Now I see. The check below "i > env->psrpil" does something similar >>>> but it doesn't reset interrupt. >>>> >>>> How about pil < (1 << (env->psrpil + 1))? I think that makes the >>>> purpose clearer. >>> >>> But it's also one operation more. Shall I just add a comment? >> >> I think the compiler should be able to calculate that 1 << (x + 1) == 2 << x. >> > Are you sure? > > $ cat test_smart_shift.c > int main (int argc, char *argv[]) { > return 1 << (1+argc); > } > > $ sparc64-linux-gnu-gcc-4.4.5 -O2 -S test_smart_shift.c > cat test_smart_shift.s > .file "test_smart_shift.c" > .section ".text" > .align 4 > .align 32 > .global main > .type main, #function > .proc 04 > main: > mov 1, %g1 > add %o0, 1, %o0 > sll %g1, %o0, %o0 > jmp %o7+8 > sra %o0, 0, %o0 > .size main, .-main > .ident "GCC: (GNU) 4.4.5" > .section .note.GNU-stack,"",@progbits Same happens on amd64. Well, the compiler isn't so smart as I thought. Then please use the 2 << version with a comment. >>>>>> >>>>>>> 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); >>>>>> >>>>>> Why reset the index? The idea is that the interrupt is left pending a >>>>>> change to PIL etc. >>>>> >>>>> But it is kept in env->pil_in and env->softint . Am I missing some >>>>> scenario where it's not enough? >>>> >>>> cpu-exec.c:378 checks interrupt_index, not pil_in etc. >>> >>> The scenario this patch is trying to fix: >>> >>> There comes an interrupt N, the PIL is small enough to proceed, >>> proceeding disables the interrupts. >>> Then the interrupt handler deasserts N and enables the interrupts. The >>> interrupt N comes again but is not processed because of old_interrupt >>> != new_interrupt check. >> >> Deasserting the interrupt should clear pil_in which should flow down >> to interrupt_index. It's a bug if this does not happen. > > Ok, I'll drop the second part in v2 then. > > > -- > Regards, > Artyom Tarasenko > > solaris/sparc under qemu blog: http://tyom.blogspot.com/ >
Thanks, applied. I added the comment myself. On Mon, Jul 25, 2011 at 5:22 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote: > clear interrupt request if the interrupt priority < CPU pil > clear hardware interrupt request if interrupts are disabled > > Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> > --- > 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 > >
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); } }
clear interrupt request if the interrupt priority < CPU pil clear hardware interrupt request if interrupts are disabled Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com> --- hw/sun4u.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)