[Qemu-devel] [PATCH] MIPS: Correct MIPS16/microMIPS branch size calculation

[Qemu-devel] [PATCH] MIPS: Correct MIPS16/microMIPS branch size calculation

[Maciej W. Rozycki]

From: Nathan Froyd <froydnj@codesourcery.com>

 Nathan's original terse comment:

"Use MIPS_HFLAG_B16 to determine the address of a jump instruction when we 
need to restart a delay slot instruction."

and was not accompanied by a test case nor I have one offhand.

 However this change appears obviously correct to me, and the same 
calculation is already used in exception_resume_pc applied to ordinary, 
Debug and NMI exceptions.  This code on the other hand applies to reset 
exceptions and instruction restarts in the context of I/O.

Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
---

 Sent on behalf of Nathan, who's since left the company.  Please apply.

  Maciej

qemu-mips-b16.diff
Index: qemu-git-trunk/exec.c
===================================================================
--- qemu-git-trunk.orig/exec.c	2012-06-04 05:34:18.655419589 +0100
+++ qemu-git-trunk/exec.c	2012-06-04 05:42:53.295516541 +0100
@@ -4235,7 +4235,7 @@ void cpu_io_recompile(CPUArchState *env,
        branch.  */
 #if defined(TARGET_MIPS)
     if ((env->hflags & MIPS_HFLAG_BMASK) != 0 && n > 1) {
-        env->active_tc.PC -= 4;
+        env->active_tc.PC -= (env->hflags & MIPS_HFLAG_B16 ? 2 : 4);
         env->icount_decr.u16.low++;
         env->hflags &= ~MIPS_HFLAG_BMASK;
     }
Index: qemu-git-trunk/target-mips/translate.c
===================================================================
--- qemu-git-trunk.orig/target-mips/translate.c	2012-06-04 05:42:49.475411277 +0100
+++ qemu-git-trunk/target-mips/translate.c	2012-06-04 05:42:53.295516541 +0100
@@ -12796,7 +12796,8 @@ void cpu_state_reset(CPUMIPSState *env)
     if (env->hflags & MIPS_HFLAG_BMASK) {
         /* If the exception was raised from a delay slot,
            come back to the jump.  */
-        env->CP0_ErrorEPC = env->active_tc.PC - 4;
+        env->CP0_ErrorEPC = (env->active_tc.PC
+                             - (env->hflags & MIPS_HFLAG_B16 ? 2 : 4));
     } else {
         env->CP0_ErrorEPC = env->active_tc.PC;
     }

Re: [Qemu-devel] [PATCH] MIPS: Correct MIPS16/microMIPS branch size calculation

[Richard Henderson]

On 2012-06-07 18:06, Maciej W. Rozycki wrote:
> "Use MIPS_HFLAG_B16 to determine the address of a jump instruction when we 
> need to restart a delay slot instruction."
> 
> and was not accompanied by a test case nor I have one offhand.
> 
>  However this change appears obviously correct to me, and the same 
> calculation is already used in exception_resume_pc applied to ordinary, 
> Debug and NMI exceptions.  This code on the other hand applies to reset 
> exceptions and instruction restarts in the context of I/O.

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

Re: [Qemu-devel] [PATCH] MIPS: Correct MIPS16/microMIPS branch size calculation

[Peter Maydell]

On 8 June 2012 02:06, Maciej W. Rozycki <macro@codesourcery.com> wrote:
> From: Nathan Froyd <froydnj@codesourcery.com>
>
>  Nathan's original terse comment:
>
> "Use MIPS_HFLAG_B16 to determine the address of a jump instruction when we
> need to restart a delay slot instruction."
>
> and was not accompanied by a test case nor I have one offhand.
>
>  However this change appears obviously correct to me, and the same
> calculation is already used in exception_resume_pc applied to ordinary,
> Debug and NMI exceptions.  This code on the other hand applies to reset
> exceptions and instruction restarts in the context of I/O.

I would prefer the commit messages to be standalone justifications
for the changes. Part of cleaning up somebody else's patches for
submission to the list should include writing good commit messages
if the original patches were overly brief in that area.

-- PMM

[Qemu-devel] [PATCH v2] mips: Correct MIPS16/microMIPS branch size calculation

[Maciej W. Rozycki]

Correct MIPS16/microMIPS branch size calculation in PC adjustment 
needed:

- to set the value of CP0.ErrorEPC at the entry to the reset exception,

- for the purpose of branch reexecution in the context of device I/O.

Follow the approach taken in `exception_resume_pc' for ordinary, Debug 
and NMI exceptions.

MIPS16 and microMIPS branches can be 2 or 4 bytes in size and that has 
to be reflected in calculation.  Original MIPS ISA branches, which is 
where this code originates from, are always 4 bytes long, just as all 
original MIPS ISA instructions.

Signed-off-by: Nathan Froyd <froydnj@codesourcery.com>
Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
---
 Another change that has waited for too long, with the original 
discussion archived here:

http://lists.nongnu.org/archive/html/qemu-devel/2012-06/msg01230.html

Resending with what hopefully is a better description and updated to 
reflect the move of `cpu_io_recompile' from exec.c to translate-all.c.

 Please apply,

  Maciej

qemu-mips-b16.diff
Index: qemu-git-trunk/target-mips/translate.c
===================================================================
--- qemu-git-trunk.orig/target-mips/translate.c	2014-11-07 18:34:30.927788566 +0000
+++ qemu-git-trunk/target-mips/translate.c	2014-11-07 18:34:35.428958223 +0000
@@ -19452,7 +19452,8 @@ void cpu_state_reset(CPUMIPSState *env)
     if (env->hflags & MIPS_HFLAG_BMASK) {
         /* If the exception was raised from a delay slot,
            come back to the jump.  */
-        env->CP0_ErrorEPC = env->active_tc.PC - 4;
+        env->CP0_ErrorEPC = (env->active_tc.PC
+                             - (env->hflags & MIPS_HFLAG_B16 ? 2 : 4));
     } else {
         env->CP0_ErrorEPC = env->active_tc.PC;
     }
Index: qemu-git-trunk/translate-all.c
===================================================================
--- qemu-git-trunk.orig/translate-all.c	2014-11-07 17:33:13.037575065 +0000
+++ qemu-git-trunk/translate-all.c	2014-11-07 18:34:35.428958223 +0000
@@ -1534,7 +1534,7 @@ void cpu_io_recompile(CPUState *cpu, uin
        branch.  */
 #if defined(TARGET_MIPS)
     if ((env->hflags & MIPS_HFLAG_BMASK) != 0 && n > 1) {
-        env->active_tc.PC -= 4;
+        env->active_tc.PC -= (env->hflags & MIPS_HFLAG_B16 ? 2 : 4);
         cpu->icount_decr.u16.low++;
         env->hflags &= ~MIPS_HFLAG_BMASK;
     }

Re: [Qemu-devel] [PATCH v2] mips: Correct MIPS16/microMIPS branch size calculation

[Leon Alrae]

On 07/11/2014 20:05, Maciej W. Rozycki wrote:
> Correct MIPS16/microMIPS branch size calculation in PC adjustment 
> needed:
> 
> - to set the value of CP0.ErrorEPC at the entry to the reset exception,
> 
> - for the purpose of branch reexecution in the context of device I/O.
> 
> Follow the approach taken in `exception_resume_pc' for ordinary, Debug 
> and NMI exceptions.
> 
> MIPS16 and microMIPS branches can be 2 or 4 bytes in size and that has 
> to be reflected in calculation.  Original MIPS ISA branches, which is 
> where this code originates from, are always 4 bytes long, just as all 
> original MIPS ISA instructions.
> 
> Signed-off-by: Nathan Froyd <froydnj@codesourcery.com>
> Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
> ---
>  Another change that has waited for too long, with the original 
> discussion archived here:
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2012-06/msg01230.html
> 
> Resending with what hopefully is a better description and updated to 
> reflect the move of `cpu_io_recompile' from exec.c to translate-all.c.
> 
>  Please apply,
> 
>   Maciej
> 
> qemu-mips-b16.diff
> Index: qemu-git-trunk/target-mips/translate.c
> ===================================================================
> --- qemu-git-trunk.orig/target-mips/translate.c	2014-11-07 18:34:30.927788566 +0000
> +++ qemu-git-trunk/target-mips/translate.c	2014-11-07 18:34:35.428958223 +0000
> @@ -19452,7 +19452,8 @@ void cpu_state_reset(CPUMIPSState *env)
>      if (env->hflags & MIPS_HFLAG_BMASK) {
>          /* If the exception was raised from a delay slot,
>             come back to the jump.  */
> -        env->CP0_ErrorEPC = env->active_tc.PC - 4;
> +        env->CP0_ErrorEPC = (env->active_tc.PC
> +                             - (env->hflags & MIPS_HFLAG_B16 ? 2 : 4));
>      } else {
>          env->CP0_ErrorEPC = env->active_tc.PC;
>      }
> Index: qemu-git-trunk/translate-all.c
> ===================================================================
> --- qemu-git-trunk.orig/translate-all.c	2014-11-07 17:33:13.037575065 +0000
> +++ qemu-git-trunk/translate-all.c	2014-11-07 18:34:35.428958223 +0000
> @@ -1534,7 +1534,7 @@ void cpu_io_recompile(CPUState *cpu, uin
>         branch.  */
>  #if defined(TARGET_MIPS)
>      if ((env->hflags & MIPS_HFLAG_BMASK) != 0 && n > 1) {
> -        env->active_tc.PC -= 4;
> +        env->active_tc.PC -= (env->hflags & MIPS_HFLAG_B16 ? 2 : 4);
>          cpu->icount_decr.u16.low++;
>          env->hflags &= ~MIPS_HFLAG_BMASK;
>      }
> 

Reviewed-by: Leon Alrae <leon.alrae@imgtec.com>