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

Sebastian Huber <[email protected]> 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 <[email protected]>
> ---
>  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 <[email protected]>

Thanks,
Andrew


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