[PATCH 0/4] Fix some MIPS GDB simulator issues

[PATCH 0/4] Fix some MIPS GDB simulator issues

[Sebastian Huber]

Running the RTEMS test suite using the MIPS GDB simulator surfaced some
issues in the simulator which I would like to fix.

Sebastian Huber (4):
  sim: Allow an overdue event to be descheduled
  sim/mips: Do not abort on a HI/LO hazard
  sim/mips: Deliver the reserved instruction exception
  sim/mips: Recognise a software interrupt request

 sim/common/sim-events.c |  6 +++-
 sim/mips/interp.c       | 67 +++++++++++++++++++++++++++++++++++++++--
 sim/mips/mips.igen      | 17 +++++++----
 sim/mips/sim-main.h     |  2 ++
 4 files changed, 82 insertions(+), 10 deletions(-)

-- 
2.51.0

[PATCH 1/4] sim: Allow an overdue event to be descheduled

[Sebastian Huber]

sim_events_deschedule() asserted that a non-empty event queue implies a
non-negative time from event.  That is not an invariant.
sim_events_slip() decrements the time from event unconditionally, so the
time can pass an event which is still queued.  update_time_from_event()
then computes a negative time from event for the head of the queue,
which is overdue rather than pending.  The next tick raises the pending
work flag and sim_events_process() consumes it, so nothing is lost.

An event handler which deschedules another overdue event triggers the
assertion.  The MIPS jmr3904 board reaches this with two timers running:
they queue events for the same time, a branch delay slot slips the time
past both, and the handler of the first deschedules the second.

Assert only the direction which holds: an empty queue gives a negative
time from event.

Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
---
 sim/common/sim-events.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sim/common/sim-events.c b/sim/common/sim-events.c
index f87f133efc3..e908c40f764 100644
--- a/sim/common/sim-events.c
+++ b/sim/common/sim-events.c
@@ -852,7 +852,11 @@ sim_events_deschedule (SIM_DESC sd,
 		       (dead->trace != NULL) ? dead->trace : ""));
 	      sim_events_free (sd, dead);
 	      update_time_from_event (sd);
-	      SIM_ASSERT ((events->time_from_event >= 0) == (events->queue != NULL));
+	      /* sim_events_slip() advances the time past an event
+		 which is still queued. The head of the queue is then
+		 overdue rather than pending.  */
+	      SIM_ASSERT (events->queue != NULL
+			  || events->time_from_event < 0);
 	      return;
 	    }
 	}
-- 
2.51.0

Re: [PATCH 1/4] sim: Allow an overdue event to be descheduled

[Andrew Burgess]

Sebastian Huber <sebastian.huber@embedded-brains.de> writes:

> sim_events_deschedule() asserted that a non-empty event queue implies a
> non-negative time from event.  That is not an invariant.
> sim_events_slip() decrements the time from event unconditionally, so the
> time can pass an event which is still queued.  update_time_from_event()
> then computes a negative time from event for the head of the queue,
> which is overdue rather than pending.  The next tick raises the pending
> work flag and sim_events_process() consumes it, so nothing is lost.
>
> An event handler which deschedules another overdue event triggers the
> assertion.  The MIPS jmr3904 board reaches this with two timers running:
> they queue events for the same time, a branch delay slot slips the time
> past both, and the handler of the first deschedules the second.
>
> Assert only the direction which holds: an empty queue gives a negative
> time from event.
>
> Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
> ---
>  sim/common/sim-events.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/sim/common/sim-events.c b/sim/common/sim-events.c
> index f87f133efc3..e908c40f764 100644
> --- a/sim/common/sim-events.c
> +++ b/sim/common/sim-events.c
> @@ -852,7 +852,11 @@ sim_events_deschedule (SIM_DESC sd,
>  		       (dead->trace != NULL) ? dead->trace : ""));
>  	      sim_events_free (sd, dead);
>  	      update_time_from_event (sd);
> -	      SIM_ASSERT ((events->time_from_event >= 0) == (events->queue != NULL));
> +	      /* sim_events_slip() advances the time past an event
> +		 which is still queued. The head of the queue is then

Two spaces after the period please.  With that fixed:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


> +		 overdue rather than pending.  */
> +	      SIM_ASSERT (events->queue != NULL
> +			  || events->time_from_event < 0);
>  	      return;
>  	    }
>  	}
> -- 
> 2.51.0

[PATCH 2/4] sim/mips: Do not abort on a HI/LO hazard

[Sebastian Huber]

check_mf_hilo() calls sim_engine_abort() when a mfhi or mflo reads a
value which the ISA leaves UNPREDICTABLE.  The comment above the helper
states the rule correctly: the result is UNPREDICTABLE, not an error.
Reading an undefined value is not a fault, and the return value of the
helper is discarded at both call sites, so the abort is its only effect.

The abort halts the client and the run loop resumes it on the faulting
instruction, which reads the same register again and aborts again.  An
operating system may save HI and LO on interrupt entry and restores them
unchanged, so it reads them at an arbitrary instruction boundary where
the last writer is whatever the interrupted program did.  Every clock
tick landing in multiply or divide heavy code then deadlocks it.

Warn instead.

Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
---
 sim/mips/mips.igen | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/sim/mips/mips.igen b/sim/mips/mips.igen
index 8203d19f8c4..fc2f81f85d0 100644
--- a/sim/mips/mips.igen
+++ b/sim/mips/mips.igen
@@ -411,12 +411,17 @@
 	    && peer->mf.timestamp < peer->mt.timestamp))
     {
       /* The peer has been written to since the last OP yet we have
-         not */
-      sim_engine_abort (SD, CPU, CIA, "HILO: %s: MF at 0x%08lx following OP at 0x%08lx corrupted by MT at 0x%08lx\n",
-			itable[MY_INDEX].name,
-			(long) CIA,
-			(long) history->op.cia,
-			(long) peer->mt.cia);
+	 not.  The ISA makes the result of this MF UNPREDICTABLE, it does not
+	 make it an error.  An operating system may save HI and LO
+	 unconditionally on interrupt entry and restores them unchanged, so it
+	 reads whatever the interrupted program left behind.  Aborting the
+	 simulation here deadlocks such a system, since the run loop resumes at
+	 the faulting instruction and the MF is executed again.  */
+      sim_io_eprintf (SD, "HILO: %s: MF at 0x%08lx following OP at 0x%08lx corrupted by MT at 0x%08lx\n",
+		      itable[MY_INDEX].name,
+		      (long) CIA,
+		      (long) history->op.cia,
+		      (long) peer->mt.cia);
       ok = 0;
     }
   history->mf.timestamp = time;
-- 
2.51.0

Re: [PATCH 2/4] sim/mips: Do not abort on a HI/LO hazard

[Andrew Burgess]

Sebastian Huber <sebastian.huber@embedded-brains.de> writes:

> check_mf_hilo() calls sim_engine_abort() when a mfhi or mflo reads a
> value which the ISA leaves UNPREDICTABLE.  The comment above the helper
> states the rule correctly: the result is UNPREDICTABLE, not an error.
> Reading an undefined value is not a fault, and the return value of the
> helper is discarded at both call sites, so the abort is its only effect.
>
> The abort halts the client and the run loop resumes it on the faulting
> instruction, which reads the same register again and aborts again.  An
> operating system may save HI and LO on interrupt entry and restores them

'restores' -> 'restore' please.

> unchanged, so it reads them at an arbitrary instruction boundary where
> the last writer is whatever the interrupted program did.  Every clock
> tick landing in multiply or divide heavy code then deadlocks it.
>
> Warn instead.
>
> Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
> ---
>  sim/mips/mips.igen | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/sim/mips/mips.igen b/sim/mips/mips.igen
> index 8203d19f8c4..fc2f81f85d0 100644
> --- a/sim/mips/mips.igen
> +++ b/sim/mips/mips.igen
> @@ -411,12 +411,17 @@
>  	    && peer->mf.timestamp < peer->mt.timestamp))
>      {
>        /* The peer has been written to since the last OP yet we have
> -         not */
> -      sim_engine_abort (SD, CPU, CIA, "HILO: %s: MF at 0x%08lx following OP at 0x%08lx corrupted by MT at 0x%08lx\n",
> -			itable[MY_INDEX].name,
> -			(long) CIA,
> -			(long) history->op.cia,
> -			(long) peer->mt.cia);
> +	 not.  The ISA makes the result of this MF UNPREDICTABLE, it does not
> +	 make it an error.  An operating system may save HI and LO
> +	 unconditionally on interrupt entry and restores them unchanged, so it

Same here: 'restores' -> 'restore' please.

> +	 reads whatever the interrupted program left behind.  Aborting the
> +	 simulation here deadlocks such a system, since the run loop resumes at
> +	 the faulting instruction and the MF is executed again.  */
> +      sim_io_eprintf (SD, "HILO: %s: MF at 0x%08lx following OP at 0x%08lx corrupted by MT at 0x%08lx\n",
> +		      itable[MY_INDEX].name,
> +		      (long) CIA,
> +		      (long) history->op.cia,
> +		      (long) peer->mt.cia);

This is fine, but is there not a risk that this is going to end up
spamming stderr (or whatever) with these warnings?  Would it not be
worth adding some kind of counter:

  {
    static warning_count = 0;
    if (warning_count < 10)
      {
        ++warning_count;
        sim_io_eprintf (SD, ".....");
        if (warning_count == 10)
          sim_io_eprintf (SD, "Future warnings about .... are now silenced\n");
      }
  }

But I'll leave this up to you, I'm happy with the change as is.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


>        ok = 0;
>      }
>    history->mf.timestamp = time;
> -- 
> 2.51.0

[PATCH 3/4] sim/mips: Deliver the reserved instruction exception

[Sebastian Huber]

The ReservedInstruction and CoProcessorUnusable cases of
signal_exception() compute CAUSE, SR and EPC and set PC to the exception
handler address, and then assign PC = EPC, which discards all of it.
The client resumes on the faulting instruction rather than in its
handler, so it raises the same exception again and never advances.  The
address error cases immediately above deliberately do not do this and
say so in a comment.

The consequence is that a target which handles the exception never runs.
A compiler emits rdhwr to read the thread pointer for every access to a
thread-local object on every MIPS target, because the instruction exists
since MIPS32r2 and the operating system is expected to emulate it where
the processor lacks it.  The R3900 lacks it.

Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
---
 sim/mips/interp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sim/mips/interp.c b/sim/mips/interp.c
index a2757ec5ef7..5dbd1482b99 100644
--- a/sim/mips/interp.c
+++ b/sim/mips/interp.c
@@ -1992,7 +1992,8 @@ signal_exception (SIM_DESC sd,
 
        case ReservedInstruction:
        case CoProcessorUnusable:
-	 PC = EPC;
+	 /* Leave PC at the exception handler address.  This allows emulating
+	    an instruction the CPU lacks.  */
 	 sim_engine_halt (SD, CPU, NULL, PC,
 			  sim_stopped, SIM_SIGILL);
 
-- 
2.51.0

Re: [PATCH 3/4] sim/mips: Deliver the reserved instruction exception

[Andrew Burgess]

Sebastian Huber <sebastian.huber@embedded-brains.de> writes:

> The ReservedInstruction and CoProcessorUnusable cases of
> signal_exception() compute CAUSE, SR and EPC and set PC to the exception
> handler address, and then assign PC = EPC, which discards all of it.
> The client resumes on the faulting instruction rather than in its
> handler, so it raises the same exception again and never advances.  The
> address error cases immediately above deliberately do not do this and
> say so in a comment.
>
> The consequence is that a target which handles the exception never runs.
> A compiler emits rdhwr to read the thread pointer for every access to a
> thread-local object on every MIPS target, because the instruction exists
> since MIPS32r2 and the operating system is expected to emulate it where
> the processor lacks it.  The R3900 lacks it.
>
> Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>

Please remove the 'Signed-off-by' tag from all commits.  This tag has no
meaning for the GDB project, but might in the future.  Sorry I missed
giving this feedback on the earlier patches.

Otherwise:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

> ---
>  sim/mips/interp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/sim/mips/interp.c b/sim/mips/interp.c
> index a2757ec5ef7..5dbd1482b99 100644
> --- a/sim/mips/interp.c
> +++ b/sim/mips/interp.c
> @@ -1992,7 +1992,8 @@ signal_exception (SIM_DESC sd,
>  
>         case ReservedInstruction:
>         case CoProcessorUnusable:
> -	 PC = EPC;
> +	 /* Leave PC at the exception handler address.  This allows emulating
> +	    an instruction the CPU lacks.  */
>  	 sim_engine_halt (SD, CPU, NULL, PC,
>  			  sim_stopped, SIM_SIGILL);
>  
> -- 
> 2.51.0

[PATCH 4/4] sim/mips: Recognise a software interrupt request

[Sebastian Huber]

Add support for the Cause.IP0 and Cause.IP1 software generated
interrupts defined by the MIPS Architecture For Programmers Volume III:
The MIPS Privileged Resource Architecture.

Deliver it from the event queue rather than in place, because
signal_exception() leaves the handler address in the program counter for
an interrupt and the instruction being executed would overwrite it.

Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
---
 sim/mips/interp.c   | 64 +++++++++++++++++++++++++++++++++++++++++++--
 sim/mips/sim-main.h |  2 ++
 2 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/sim/mips/interp.c b/sim/mips/interp.c
index 5dbd1482b99..fddb96d00ed 100644
--- a/sim/mips/interp.c
+++ b/sim/mips/interp.c
@@ -295,6 +295,58 @@ static const OPTION mips_options[] =
 
 int interrupt_pending;
 
+/* An interrupt is requested while the interrupts are enabled and a pending
+   bit of the Cause register meets its mask bit in the Status register:
+
+     Status.IE = 1, Status.EXL = 0, Status.ERL = 0, Cause.IP & Status.IM != 0
+
+   MIPS Architecture For Programmers Volume III: The MIPS Privileged Resource
+   Architecture, the Interrupts chapter.  The R3000 generation, which the
+   R3900 belongs to, has no exception level and disables the interrupts by
+   shifting the interrupt enable stack of its Status register instead, so only
+   the current enable takes part; see the IDT R30xx Family Software Reference
+   Manual, the Status register of the CPU control chapter.  */
+static int
+interrupt_requested (sim_cpu *cpu)
+{
+  if ((SR & status_IE) == 0)
+    return 0;
+
+#ifndef SUBTARGET_R3900
+  if ((SR & (status_EXL | status_ERL)) != 0)
+    return 0;
+#endif
+
+  /* Only the software interrupts.  A hardware interrupt keeps its pending bit
+     set until its device is served. The device model delivers it.  */
+  return ((CAUSE >> cause_IPSW_shift) & (SR >> status_IM_shift)
+	  & cause_IPSW_mask) != 0;
+}
+
+static void
+software_interrupt_event (SIM_DESC sd, void *data)
+{
+  sim_cpu *cpu = STATE_CPU (sd, 0);
+  address_word cia = CPU_PC_GET (cpu);
+
+  /* Recheck, because the write which scheduled this may have been undone in
+     the meantime.  */
+  if (interrupt_requested (cpu))
+    SignalExceptionInterrupt (0);
+}
+
+/* Deliver a pending interrupt at the next instruction boundary.  It cannot be
+   delivered here: signal_exception() leaves the handler address in the program
+   counter for an interrupt and the instruction which is being executed would
+   overwrite it.  This is why the hardware interrupts arrive through the event
+   queue as well.  */
+static void
+check_interrupts (SIM_DESC sd, sim_cpu *cpu)
+{
+  if (interrupt_requested (cpu))
+    sim_events_schedule (sd, 1, software_interrupt_event, NULL);
+}
+
 void
 interrupt_event (SIM_DESC sd, void *data)
 {
@@ -2269,14 +2321,20 @@ decode_coproc (SIM_DESC sd,
 		if (op == cp0_mfc0 || op == cp0_dmfc0)
 		  GPR[rt] = SR;
 		else
-		  SR = GPR[rt];
+		  {
+		    SR = GPR[rt];
+		    check_interrupts (sd, cpu);
+		  }
 		break;
 		/* 13 = Cause              R4000   VR4100  VR4300 */
 	      case 13:
 		if (op == cp0_mfc0 || op == cp0_dmfc0)
 		  GPR[rt] = CAUSE;
 		else
-		  CAUSE = GPR[rt];
+		  {
+		    CAUSE = GPR[rt];
+		    check_interrupts (sd, cpu);
+		  }
 		break;
 		/* 14 = EPC                R4000   VR4100  VR4300 */
 	      case 14:
@@ -2391,6 +2449,7 @@ decode_coproc (SIM_DESC sd,
 	      {
 		PC = EPC;
 		SR &= ~status_EXL;
+		check_interrupts (sd, cpu);
 	      }
 	  }
         else if (op == cp0_rfe && sel == 0x10)
@@ -2401,6 +2460,7 @@ decode_coproc (SIM_DESC sd,
 
 	    /* shift IE/KU history bits right */
 	    SR = LSMASKED32(SR, 31, 4) | LSINSERTED32(LSEXTRACTED32(SR, 5, 2), 3, 0);
+	    check_interrupts (sd, cpu);
 
 	    /* TODO: CACHE register */
 #endif /* SUBTARGET_R3900 */
diff --git a/sim/mips/sim-main.h b/sim/mips/sim-main.h
index b6cb4e12258..3af02e49c0e 100644
--- a/sim/mips/sim-main.h
+++ b/sim/mips/sim-main.h
@@ -557,6 +557,8 @@ struct mips_sim_state {
 #define cause_SW1       (1 << 9)        /* Software interrupt 1 */
 #define cause_IP_mask   (0x3f)          /* Interrupt pending field */
 #define cause_IP_shift  (10)
+#define cause_IPSW_mask (0x3)           /* Software interrupt pending, IP1:IP0 */
+#define cause_IPSW_shift (8)
 
 #define cause_set_EXC(x)  CAUSE = (CAUSE & ~cause_EXC_mask)  | ((x << cause_EXC_shift)  & cause_EXC_mask)
 #define cause_set_EXC2(x) CAUSE = (CAUSE & ~cause_EXC2_mask) | ((x << cause_EXC2_shift) & cause_EXC2_mask)
-- 
2.51.0

Re: [PATCH 4/4] sim/mips: Recognise a software interrupt request

[Andrew Burgess]

Sebastian Huber <sebastian.huber@embedded-brains.de> writes:

> Add support for the Cause.IP0 and Cause.IP1 software generated
> interrupts defined by the MIPS Architecture For Programmers Volume III:
> The MIPS Privileged Resource Architecture.
>
> Deliver it from the event queue rather than in place, because
> signal_exception() leaves the handler address in the program counter for
> an interrupt and the instruction being executed would overwrite it.
>
> Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
> ---
>  sim/mips/interp.c   | 64 +++++++++++++++++++++++++++++++++++++++++++--
>  sim/mips/sim-main.h |  2 ++
>  2 files changed, 64 insertions(+), 2 deletions(-)
>
> diff --git a/sim/mips/interp.c b/sim/mips/interp.c
> index 5dbd1482b99..fddb96d00ed 100644
> --- a/sim/mips/interp.c
> +++ b/sim/mips/interp.c
> @@ -295,6 +295,58 @@ static const OPTION mips_options[] =
>  
>  int interrupt_pending;
>  
> +/* An interrupt is requested while the interrupts are enabled and a pending
> +   bit of the Cause register meets its mask bit in the Status register:
> +
> +     Status.IE = 1, Status.EXL = 0, Status.ERL = 0, Cause.IP & Status.IM != 0
> +
> +   MIPS Architecture For Programmers Volume III: The MIPS Privileged Resource
> +   Architecture, the Interrupts chapter.  The R3000 generation, which the
> +   R3900 belongs to, has no exception level and disables the interrupts by
> +   shifting the interrupt enable stack of its Status register instead, so only
> +   the current enable takes part; see the IDT R30xx Family Software Reference
> +   Manual, the Status register of the CPU control chapter.  */
> +static int
> +interrupt_requested (sim_cpu *cpu)
> +{
> +  if ((SR & status_IE) == 0)
> +    return 0;
> +
> +#ifndef SUBTARGET_R3900
> +  if ((SR & (status_EXL | status_ERL)) != 0)
> +    return 0;
> +#endif
> +
> +  /* Only the software interrupts.  A hardware interrupt keeps its pending bit
> +     set until its device is served. The device model delivers it.  */

Two spaces after 'served.' please.

> +  return ((CAUSE >> cause_IPSW_shift) & (SR >> status_IM_shift)
> +	  & cause_IPSW_mask) != 0;
> +}
> +
> +static void
> +software_interrupt_event (SIM_DESC sd, void *data)
> +{
> +  sim_cpu *cpu = STATE_CPU (sd, 0);
> +  address_word cia = CPU_PC_GET (cpu);
> +
> +  /* Recheck, because the write which scheduled this may have been undone in
> +     the meantime.  */
> +  if (interrupt_requested (cpu))
> +    SignalExceptionInterrupt (0);
> +}
> +
> +/* Deliver a pending interrupt at the next instruction boundary.  It cannot be
> +   delivered here: signal_exception() leaves the handler address in the program
> +   counter for an interrupt and the instruction which is being executed would
> +   overwrite it.  This is why the hardware interrupts arrive through the event
> +   queue as well.  */
> +static void
> +check_interrupts (SIM_DESC sd, sim_cpu *cpu)
> +{
> +  if (interrupt_requested (cpu))
> +    sim_events_schedule (sd, 1, software_interrupt_event, NULL);
> +}
> +
>  void
>  interrupt_event (SIM_DESC sd, void *data)
>  {
> @@ -2269,14 +2321,20 @@ decode_coproc (SIM_DESC sd,
>  		if (op == cp0_mfc0 || op == cp0_dmfc0)
>  		  GPR[rt] = SR;
>  		else
> -		  SR = GPR[rt];
> +		  {
> +		    SR = GPR[rt];
> +		    check_interrupts (sd, cpu);
> +		  }
>  		break;
>  		/* 13 = Cause              R4000   VR4100  VR4300 */
>  	      case 13:
>  		if (op == cp0_mfc0 || op == cp0_dmfc0)
>  		  GPR[rt] = CAUSE;
>  		else
> -		  CAUSE = GPR[rt];
> +		  {
> +		    CAUSE = GPR[rt];
> +		    check_interrupts (sd, cpu);
> +		  }
>  		break;
>  		/* 14 = EPC                R4000   VR4100  VR4300 */
>  	      case 14:
> @@ -2391,6 +2449,7 @@ decode_coproc (SIM_DESC sd,
>  	      {
>  		PC = EPC;
>  		SR &= ~status_EXL;
> +		check_interrupts (sd, cpu);
>  	      }

This is the ERET case for handling the situation where the ERL bit is
cleared.  If I reproduce your patched code, but with more context, we
see this:

  /* ERET */
  if (SR & status_ERL)
    {
      /* Oops, not yet available */
      sim_io_printf(sd,"Warning: ERET when SR[ERL] set not handled yet");
      PC = EPC;
      SR &= ~status_ERL;
    }
  else
    {
      PC = EPC;
      SR &= ~status_EXL;
      check_interrupts (sd, cpu);
    }

Now clearly the `if` block is broken, we're setting PC from the wrong
place I think.  But if this block _was_ ever fixed then we're going to
need a check_interrupts call on that path too, right?

My suggestion is that we move the check_interrupts call after the `else`
block, like this:

  /* ERET */
  if (SR & status_ERL)
    {
      /* Oops, not yet available */
      sim_io_printf(sd,"Warning: ERET when SR[ERL] set not handled yet");
      PC = EPC;
      SR &= ~status_ERL;
    }
  else
    {
      PC = EPC;
      SR &= ~status_EXL;
    }
  check_interrupts (sd, cpu);

This doesn't fix the `if` block, but if someone ever does fix that path,
then the check_interrupts call will be in place ready for them.

What do you think?

If you're happy to accept the two changes then:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew



>  	  }
>          else if (op == cp0_rfe && sel == 0x10)
> @@ -2401,6 +2460,7 @@ decode_coproc (SIM_DESC sd,
>  
>  	    /* shift IE/KU history bits right */
>  	    SR = LSMASKED32(SR, 31, 4) | LSINSERTED32(LSEXTRACTED32(SR, 5, 2), 3, 0);
> +	    check_interrupts (sd, cpu);
>  
>  	    /* TODO: CACHE register */
>  #endif /* SUBTARGET_R3900 */
> diff --git a/sim/mips/sim-main.h b/sim/mips/sim-main.h
> index b6cb4e12258..3af02e49c0e 100644
> --- a/sim/mips/sim-main.h
> +++ b/sim/mips/sim-main.h
> @@ -557,6 +557,8 @@ struct mips_sim_state {
>  #define cause_SW1       (1 << 9)        /* Software interrupt 1 */
>  #define cause_IP_mask   (0x3f)          /* Interrupt pending field */
>  #define cause_IP_shift  (10)
> +#define cause_IPSW_mask (0x3)           /* Software interrupt pending, IP1:IP0 */
> +#define cause_IPSW_shift (8)
>  
>  #define cause_set_EXC(x)  CAUSE = (CAUSE & ~cause_EXC_mask)  | ((x << cause_EXC_shift)  & cause_EXC_mask)
>  #define cause_set_EXC2(x) CAUSE = (CAUSE & ~cause_EXC2_mask) | ((x << cause_EXC2_shift) & cause_EXC2_mask)
> -- 
> 2.51.0

Re: [PATCH 4/4] sim/mips: Recognise a software interrupt request

[Sebastian Huber]

----- Am 18. Aug 2026 um 12:42 schrieb Andrew Burgess aburgess@redhat.com:

> Sebastian Huber <sebastian.huber@embedded-brains.de> writes:
> 
>> Add support for the Cause.IP0 and Cause.IP1 software generated
>> interrupts defined by the MIPS Architecture For Programmers Volume III:
>> The MIPS Privileged Resource Architecture.
>>
>> Deliver it from the event queue rather than in place, because
>> signal_exception() leaves the handler address in the program counter for
>> an interrupt and the instruction being executed would overwrite it.
>>
>> Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
>> ---
>>  sim/mips/interp.c   | 64 +++++++++++++++++++++++++++++++++++++++++++--
>>  sim/mips/sim-main.h |  2 ++
>>  2 files changed, 64 insertions(+), 2 deletions(-)
>>
>> diff --git a/sim/mips/interp.c b/sim/mips/interp.c
>> index 5dbd1482b99..fddb96d00ed 100644
>> --- a/sim/mips/interp.c
>> +++ b/sim/mips/interp.c
>> @@ -295,6 +295,58 @@ static const OPTION mips_options[] =
>>  
>>  int interrupt_pending;
>>  
>> +/* An interrupt is requested while the interrupts are enabled and a pending
>> +   bit of the Cause register meets its mask bit in the Status register:
>> +
>> +     Status.IE = 1, Status.EXL = 0, Status.ERL = 0, Cause.IP & Status.IM != 0
>> +
>> +   MIPS Architecture For Programmers Volume III: The MIPS Privileged Resource
>> +   Architecture, the Interrupts chapter.  The R3000 generation, which the
>> +   R3900 belongs to, has no exception level and disables the interrupts by
>> +   shifting the interrupt enable stack of its Status register instead, so only
>> +   the current enable takes part; see the IDT R30xx Family Software Reference
>> +   Manual, the Status register of the CPU control chapter.  */
>> +static int
>> +interrupt_requested (sim_cpu *cpu)
>> +{
>> +  if ((SR & status_IE) == 0)
>> +    return 0;
>> +
>> +#ifndef SUBTARGET_R3900
>> +  if ((SR & (status_EXL | status_ERL)) != 0)
>> +    return 0;
>> +#endif
>> +
>> +  /* Only the software interrupts.  A hardware interrupt keeps its pending bit
>> +     set until its device is served. The device model delivers it.  */
> 
> Two spaces after 'served.' please.
> 
>> +  return ((CAUSE >> cause_IPSW_shift) & (SR >> status_IM_shift)
>> +	  & cause_IPSW_mask) != 0;
>> +}
>> +
>> +static void
>> +software_interrupt_event (SIM_DESC sd, void *data)
>> +{
>> +  sim_cpu *cpu = STATE_CPU (sd, 0);
>> +  address_word cia = CPU_PC_GET (cpu);
>> +
>> +  /* Recheck, because the write which scheduled this may have been undone in
>> +     the meantime.  */
>> +  if (interrupt_requested (cpu))
>> +    SignalExceptionInterrupt (0);
>> +}
>> +
>> +/* Deliver a pending interrupt at the next instruction boundary.  It cannot be
>> +   delivered here: signal_exception() leaves the handler address in the program
>> +   counter for an interrupt and the instruction which is being executed would
>> +   overwrite it.  This is why the hardware interrupts arrive through the event
>> +   queue as well.  */
>> +static void
>> +check_interrupts (SIM_DESC sd, sim_cpu *cpu)
>> +{
>> +  if (interrupt_requested (cpu))
>> +    sim_events_schedule (sd, 1, software_interrupt_event, NULL);
>> +}
>> +
>>  void
>>  interrupt_event (SIM_DESC sd, void *data)
>>  {
>> @@ -2269,14 +2321,20 @@ decode_coproc (SIM_DESC sd,
>>  		if (op == cp0_mfc0 || op == cp0_dmfc0)
>>  		  GPR[rt] = SR;
>>  		else
>> -		  SR = GPR[rt];
>> +		  {
>> +		    SR = GPR[rt];
>> +		    check_interrupts (sd, cpu);
>> +		  }
>>  		break;
>>  		/* 13 = Cause              R4000   VR4100  VR4300 */
>>  	      case 13:
>>  		if (op == cp0_mfc0 || op == cp0_dmfc0)
>>  		  GPR[rt] = CAUSE;
>>  		else
>> -		  CAUSE = GPR[rt];
>> +		  {
>> +		    CAUSE = GPR[rt];
>> +		    check_interrupts (sd, cpu);
>> +		  }
>>  		break;
>>  		/* 14 = EPC                R4000   VR4100  VR4300 */
>>  	      case 14:
>> @@ -2391,6 +2449,7 @@ decode_coproc (SIM_DESC sd,
>>  	      {
>>  		PC = EPC;
>>  		SR &= ~status_EXL;
>> +		check_interrupts (sd, cpu);
>>  	      }
> 
> This is the ERET case for handling the situation where the ERL bit is
> cleared.  If I reproduce your patched code, but with more context, we
> see this:
> 
>  /* ERET */
>  if (SR & status_ERL)
>    {
>      /* Oops, not yet available */
>      sim_io_printf(sd,"Warning: ERET when SR[ERL] set not handled yet");
>      PC = EPC;
>      SR &= ~status_ERL;
>    }
>  else
>    {
>      PC = EPC;
>      SR &= ~status_EXL;
>      check_interrupts (sd, cpu);
>    }
> 
> Now clearly the `if` block is broken, we're setting PC from the wrong
> place I think.  But if this block _was_ ever fixed then we're going to
> need a check_interrupts call on that path too, right?
> 
> My suggestion is that we move the check_interrupts call after the `else`
> block, like this:
> 
>  /* ERET */
>  if (SR & status_ERL)
>    {
>      /* Oops, not yet available */
>      sim_io_printf(sd,"Warning: ERET when SR[ERL] set not handled yet");
>      PC = EPC;
>      SR &= ~status_ERL;
>    }
>  else
>    {
>      PC = EPC;
>      SR &= ~status_EXL;
>    }
>  check_interrupts (sd, cpu);
> 
> This doesn't fix the `if` block, but if someone ever does fix that path,
> then the check_interrupts call will be in place ready for them.
> 
> What do you think?

Thanks for your review. I used the RTEMS test suite as my main driver for the changes.

Yes, I think that moving the check after the `if` block makes sense.

> 
> If you're happy to accept the two changes then:
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>

I am only an occasional contributor, so I am not sure if I understood it correctly.

1. I remove the Signed-off-by from all four patches.

2. I fix all your review comments.

3. I run my tests again.

4. I add the Approved-By: Andrew Burgess <aburgess@redhat.com> to all four commits.

5. I don't send a v2 version to the patches list.

6. I check in the updated patch set directly.

Kind regards,
Sebastian

-- 
embedded brains GmbH & Co. KG
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/

Re: [PATCH 4/4] sim/mips: Recognise a software interrupt request

[Maciej W. Rozycki]

On Tue, 18 Aug 2026, Andrew Burgess wrote:

> Now clearly the `if` block is broken, we're setting PC from the wrong
> place I think.

 Yep, the correct one would be ErrorEPC, unimplemented in sim AFAICT just 
as proper support for status_ERL, which has numerous implications beyond 
just ERET.

  Maciej

Re: [PATCH 4/4] sim/mips: Recognise a software interrupt request

[Sebastian Huber]

----- Am 19. Aug 2026 um 2:03 schrieb Maciej W. Rozycki macro@orcam.me.uk:

> On Tue, 18 Aug 2026, Andrew Burgess wrote:
> 
>> Now clearly the `if` block is broken, we're setting PC from the wrong
>> place I think.
> 
> Yep, the correct one would be ErrorEPC, unimplemented in sim AFAICT just
> as proper support for status_ERL, which has numerous implications beyond
> just ERET.

I this something I should address within my patch set or is moving the check_interrupts() after the if/else block enough?

-- 
embedded brains GmbH & Co. KG
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/

Re: [PATCH 4/4] sim/mips: Recognise a software interrupt request

[Maciej W. Rozycki]

On Wed, 19 Aug 2026, Sebastian Huber wrote:

> >> Now clearly the `if` block is broken, we're setting PC from the wrong
> >> place I think.
> > 
> > Yep, the correct one would be ErrorEPC, unimplemented in sim AFAICT just
> > as proper support for status_ERL, which has numerous implications beyond
> > just ERET.
> 
> I this something I should address within my patch set or is moving the 
> check_interrupts() after the if/else block enough?

 I have glanced over your change and I conclude that the idea here is to 
call check_interrupts() whenever CP0 Status or Config registers have been 
changed such as to possibly raise a software interrupt.  Given how sim has 
been structured this seems a reasonable approach to me.  For this I think 
just moving the call past the block is exactly what is needed.

 Also check_interrupts() needs to be called from DERET emulation, since 
clearing CP0 Debug.DM (Debug_DM) also re-enables interrupts (subject to 
CP0 Status conditions), and Debug_DM needs to be checked against in 
`interrupt_requested'.

 FWIW I think this would best be called `software_interrupt_requested' or 
suchlike to avoid the ambiguity.  While one can read the comment, there's 
no need to force them doing so to figure out the intent.

 Then there's the preexisting issue with `interrupt_event' that does not 
check any of this beyond just CP0 Status.IE (status_IE).  While it seems 
broken to me anyway, as I fail to see a place where `interrupt_pending' 
would be set, let's not let breakage accumulate.  So I think bits from 
your `interrupt_requested' need to be factored out and used at both 
places, possibly as an introductory change.

 It's not clear to me how the argument to SignalExceptionInterrupt() is 
supposed to be used, but it's not interpreted anyway AFAICT, so let's just 
leave it as it is.

 As to adding proper support for ErrorEPC/status_ERL, you are obviously 
welcome to if you feel so inclined and in particular have a use for, but I 
wouldn't consider it a prerequisite for your patchset.  In real hardware 
it's a feature used primarily in the context of handling CPU soft reset, 
NMI and cache error exceptions, which I think are secondary ISA features 
when it comes to sim.

  Maciej