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

[Sebastian Huber <sebastian.huber@embedded-brains.de>]

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