[Qemu-devel] [PATCH] mips: Correct the handling of writes to CP0.Status for MIPSr6

[Qemu-devel] [PATCH] mips: Correct the handling of writes to CP0.Status for MIPSr6

[Maciej W. Rozycki]

Correct these issues with the handling of CP0.Status for MIPSr6:

* only ignore the bit pattern of 0b11 on writes to CP0.Status.KSU, that 
  is for processors that do implement Supervisor Mode, let the bit
  pattern be written to CP0.Status.UM:R0 freely (of course the value 
  written to read-only CP0.Status.R0 will be discarded anyway); this is 
  in accordance to the relevant architecture specification[1],

* check the newly written pattern rather than the current contents of 
  CP0.Status for the KSU bits being 0b11,

* use meaningful macro names to refer to CP0.Status bits rather than 
  magic numbers.

References:

[1] "MIPS Architecture For Programmers, Volume III: MIPS64 / microMIPS64
    Privileged Resource Architecture", MIPS Technologies, Inc., Document 
    Number: MD00091, Revision 6.00, March 31, 2014, Table 9.45 "Status 
    Register Field Descriptions", pp. 210-211.

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

 Noticed in porting the next change I'm going to post.  NB I have no 
reasonable way to do run-time checks of an r6 configuration, so please 
double check this works for you even though I believe it is obviously 
correct; I did check CP0.Status.KSU rejects the pattern of 0b11 via GDB 
on MIPS64R6-generic with the aid of the next change.

 I suggest making it a policy not to accept new code using magic 
numbers.  Then the existing places can be gradually identified and 
corrected.

 Please apply.

 Thanks,

  Maciej

qemu-mips-r6-status-r0.diff
Index: qemu-git-trunk/target-mips/op_helper.c
===================================================================
--- qemu-git-trunk.orig/target-mips/op_helper.c	2014-11-09 23:44:45.467759913 +0000
+++ qemu-git-trunk/target-mips/op_helper.c	2014-11-09 23:45:27.977531070 +0000
@@ -1423,10 +1423,12 @@ void helper_mtc0_status(CPUMIPSState *en
     uint32_t mask = env->CP0_Status_rw_bitmask;
 
     if (env->insn_flags & ISA_MIPS32R6) {
-        if (extract32(env->CP0_Status, CP0St_KSU, 2) == 0x3) {
+        bool has_supervisor = extract32(mask, CP0St_KSU, 2) == 0x3;
+
+        if (has_supervisor && extract32(arg1, CP0St_KSU, 2) == 0x3) {
             mask &= ~(3 << CP0St_KSU);
         }
-        mask &= ~(0x00180000 & arg1);
+        mask &= ~(((1 << CP0St_SR) | (1 << CP0St_NMI)) & arg1);
     }
 
     val = arg1 & mask;

Re: [Qemu-devel] [PATCH] mips: Correct the handling of writes to CP0.Status for MIPSr6

[Leon Alrae]

On 10/11/2014 13:45, Maciej W. Rozycki wrote:
> Correct these issues with the handling of CP0.Status for MIPSr6:
> 
> * only ignore the bit pattern of 0b11 on writes to CP0.Status.KSU, that 
>   is for processors that do implement Supervisor Mode, let the bit
>   pattern be written to CP0.Status.UM:R0 freely (of course the value 
>   written to read-only CP0.Status.R0 will be discarded anyway); this is 
>   in accordance to the relevant architecture specification[1],
> 
> * check the newly written pattern rather than the current contents of 
>   CP0.Status for the KSU bits being 0b11,
> 
> * use meaningful macro names to refer to CP0.Status bits rather than 
>   magic numbers.
> 
> References:
> 
> [1] "MIPS Architecture For Programmers, Volume III: MIPS64 / microMIPS64
>     Privileged Resource Architecture", MIPS Technologies, Inc., Document 
>     Number: MD00091, Revision 6.00, March 31, 2014, Table 9.45 "Status 
>     Register Field Descriptions", pp. 210-211.
> 
> Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
> ---
> Leon,
> 
>  Noticed in porting the next change I'm going to post.  NB I have no 
> reasonable way to do run-time checks of an r6 configuration, so please 
> double check this works for you even though I believe it is obviously 
> correct; I did check CP0.Status.KSU rejects the pattern of 0b11 via GDB 
> on MIPS64R6-generic with the aid of the next change.
> 
>  I suggest making it a policy not to accept new code using magic 
> numbers.  Then the existing places can be gradually identified and 
> corrected.
> 
>  Please apply.
> 
>  Thanks,
> 
>   Maciej
> 
> qemu-mips-r6-status-r0.diff
> Index: qemu-git-trunk/target-mips/op_helper.c
> ===================================================================
> --- qemu-git-trunk.orig/target-mips/op_helper.c	2014-11-09 23:44:45.467759913 +0000
> +++ qemu-git-trunk/target-mips/op_helper.c	2014-11-09 23:45:27.977531070 +0000
> @@ -1423,10 +1423,12 @@ void helper_mtc0_status(CPUMIPSState *en
>      uint32_t mask = env->CP0_Status_rw_bitmask;
>  
>      if (env->insn_flags & ISA_MIPS32R6) {
> -        if (extract32(env->CP0_Status, CP0St_KSU, 2) == 0x3) {
> +        bool has_supervisor = extract32(mask, CP0St_KSU, 2) == 0x3;
> +
> +        if (has_supervisor && extract32(arg1, CP0St_KSU, 2) == 0x3) {
>              mask &= ~(3 << CP0St_KSU);
>          }
> -        mask &= ~(0x00180000 & arg1);
> +        mask &= ~(((1 << CP0St_SR) | (1 << CP0St_NMI)) & arg1);
>      }
>  
>      val = arg1 & mask;
> 

Thanks for fixing and cleaning this up.

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