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

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

[Artyom Tarasenko]

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

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

[tsnsaito]

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>

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

[Artyom Tarasenko]

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



-- 
Regards,
Artyom Tarasenko

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

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

[tsnsaito]

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>

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

[Tsuneo Saito]

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>

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

[Artyom Tarasenko]

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



-- 
Regards,
Artyom Tarasenko

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

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

[Artyom Tarasenko]

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

-- 
Regards,
Artyom Tarasenko

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

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

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

[Artyom Tarasenko]

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



-- 
Regards,
Artyom Tarasenko

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

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

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

[Blue Swirl]

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

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

[Blue Swirl]

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

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

[Artyom Tarasenko]

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



-- 
Regards,
Artyom Tarasenko

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

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

[Blue Swirl]

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.

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

[Artyom Tarasenko]

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.

-- 
Regards,
Artyom Tarasenko

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

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

[Blue Swirl]

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.

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

[Artyom Tarasenko]

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.


-- 
Regards,
Artyom Tarasenko

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

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

[Blue Swirl]

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

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

[Blue Swirl]

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