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

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

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