[RFC PATCH v2 0/3] target/mips: Report unimplemented cache() operations

[RFC PATCH v2 0/3] target/mips: Report unimplemented cache() operations

[Philippe Mathieu-Daudé]

Few patches to improve the handling of the 'cache' opcode.

I'm not sure why helper_cache() is generated for user-only mode.

v1 was:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg727959.html
Supersedes: <20200806122612.17167-1-f4bug@amsat.org>

Philippe Mathieu-Daudé (3):
  target/mips/op_helper: Convert multiple if() to switch case
  target/mips/op_helper: Document Invalidate/Writeback opcodes as no-op
  target/mips/op_helper: Log unimplemented cache opcode

 target/mips/op_helper.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

-- 
2.21.3

[RFC PATCH v2 1/3] target/mips/op_helper: Convert multiple if() to switch case

[Philippe Mathieu-Daudé]

The cache operation is encoded in bits [20:18] of the instruction.
The 'op' argument of helper_cache() contains the bits [20:16].
Extract the 3 bits and parse them using a switch case. This allow
us to handle multiple cache types (the cache type is encoded in
bits [17:16]).

Previously the if() block was only checking the D-Cache (Primary
Data or Unified Primary). Now we also handle the I-Cache (Primary
Instruction), S-Cache (Secondary) and T-Cache (Terciary).

Reported-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/op_helper.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
index 9552b280e0..92c399d8d4 100644
--- a/target/mips/op_helper.c
+++ b/target/mips/op_helper.c
@@ -1574,15 +1574,22 @@ void helper_msa_st_d(CPUMIPSState *env, uint32_t wd,
 void helper_cache(CPUMIPSState *env, target_ulong addr, uint32_t op)
 {
 #ifndef CONFIG_USER_ONLY
+    uint32_t cache_operation = extract32(op, 2, 3);
     target_ulong index = addr & 0x1fffffff;
-    if (op == 9) {
+
+    switch (cache_operation) {
+    case 0b010:
         /* Index Store Tag */
         memory_region_dispatch_write(env->itc_tag, index, env->CP0_TagLo,
                                      MO_64, MEMTXATTRS_UNSPECIFIED);
-    } else if (op == 5) {
+        break;
+    case 0b001:
         /* Index Load Tag */
         memory_region_dispatch_read(env->itc_tag, index, &env->CP0_TagLo,
                                     MO_64, MEMTXATTRS_UNSPECIFIED);
+        break;
+    default:
+        break;
     }
 #endif
 }
-- 
2.21.3

Re: [RFC PATCH v2 1/3] target/mips/op_helper: Convert multiple if() to switch case

[Jiaxun Yang]

在 2020/8/14 上午2:15, Philippe Mathieu-Daudé 写道:
> The cache operation is encoded in bits [20:18] of the instruction.
> The 'op' argument of helper_cache() contains the bits [20:16].
> Extract the 3 bits and parse them using a switch case. This allow
> us to handle multiple cache types (the cache type is encoded in
> bits [17:16]).
>
> Previously the if() block was only checking the D-Cache (Primary
> Data or Unified Primary). Now we also handle the I-Cache (Primary
> Instruction), S-Cache (Secondary) and T-Cache (Terciary).
>
> Reported-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>

Thanks~

> ---
>   target/mips/op_helper.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
> index 9552b280e0..92c399d8d4 100644
> --- a/target/mips/op_helper.c
> +++ b/target/mips/op_helper.c
> @@ -1574,15 +1574,22 @@ void helper_msa_st_d(CPUMIPSState *env, uint32_t wd,
>   void helper_cache(CPUMIPSState *env, target_ulong addr, uint32_t op)
>   {
>   #ifndef CONFIG_USER_ONLY
> +    uint32_t cache_operation = extract32(op, 2, 3);
>       target_ulong index = addr & 0x1fffffff;
> -    if (op == 9) {
> +
> +    switch (cache_operation) {
> +    case 0b010:
>           /* Index Store Tag */
>           memory_region_dispatch_write(env->itc_tag, index, env->CP0_TagLo,
>                                        MO_64, MEMTXATTRS_UNSPECIFIED);
> -    } else if (op == 5) {
> +        break;
> +    case 0b001:
>           /* Index Load Tag */
>           memory_region_dispatch_read(env->itc_tag, index, &env->CP0_TagLo,
>                                       MO_64, MEMTXATTRS_UNSPECIFIED);
> +        break;
> +    default:
> +        break;
>       }
>   #endif
>   }

[RFC PATCH v2 2/3] target/mips/op_helper: Document Invalidate/Writeback opcodes as no-op

[Philippe Mathieu-Daudé]

QEMU does not model caches, so there is not much to do with the
Invalidate/Writeback opcodes. Make it explicit adding a comment.

Suggested-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/op_helper.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
index 92c399d8d4..2496d1dd71 100644
--- a/target/mips/op_helper.c
+++ b/target/mips/op_helper.c
@@ -1578,16 +1578,19 @@ void helper_cache(CPUMIPSState *env, target_ulong addr, uint32_t op)
     target_ulong index = addr & 0x1fffffff;
 
     switch (cache_operation) {
-    case 0b010:
-        /* Index Store Tag */
+    case 0b010: /* Index Store Tag */
         memory_region_dispatch_write(env->itc_tag, index, env->CP0_TagLo,
                                      MO_64, MEMTXATTRS_UNSPECIFIED);
         break;
-    case 0b001:
-        /* Index Load Tag */
+    case 0b001: /* Index Load Tag */
         memory_region_dispatch_read(env->itc_tag, index, &env->CP0_TagLo,
                                     MO_64, MEMTXATTRS_UNSPECIFIED);
         break;
+    case 0b000: /* Index Invalidate */
+    case 0b100: /* Hit Invalidate */
+    case 0b110: /* Hit Writeback */
+        /* no-op */
+        break;
     default:
         break;
     }
-- 
2.21.3

Re: [RFC PATCH v2 2/3] target/mips/op_helper: Document Invalidate/Writeback opcodes as no-op

[Richard Henderson]

On 8/13/20 11:15 AM, Philippe Mathieu-Daudé wrote:
>      switch (cache_operation) {
> -    case 0b010:
> -        /* Index Store Tag */
> +    case 0b010: /* Index Store Tag */
>          memory_region_dispatch_write(env->itc_tag, index, env->CP0_TagLo,
>                                       MO_64, MEMTXATTRS_UNSPECIFIED);
>          break;
> -    case 0b001:
> -        /* Index Load Tag */
> +    case 0b001: /* Index Load Tag */
>          memory_region_dispatch_read(env->itc_tag, index, &env->CP0_TagLo,
>                                      MO_64, MEMTXATTRS_UNSPECIFIED);
>          break;

Merge these lines back to patch 1.  With that, both 1/ and 2/,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC PATCH v2 2/3] target/mips/op_helper: Document Invalidate/Writeback opcodes as no-op

[Jiaxun Yang]

在 2020/8/14 上午2:15, Philippe Mathieu-Daudé 写道:
> QEMU does not model caches, so there is not much to do with the
> Invalidate/Writeback opcodes. Make it explicit adding a comment.
>
> Suggested-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>

Thanks~

> ---
>   target/mips/op_helper.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
> index 92c399d8d4..2496d1dd71 100644
> --- a/target/mips/op_helper.c
> +++ b/target/mips/op_helper.c
> @@ -1578,16 +1578,19 @@ void helper_cache(CPUMIPSState *env, target_ulong addr, uint32_t op)
>       target_ulong index = addr & 0x1fffffff;
>   
>       switch (cache_operation) {
> -    case 0b010:
> -        /* Index Store Tag */
> +    case 0b010: /* Index Store Tag */
>           memory_region_dispatch_write(env->itc_tag, index, env->CP0_TagLo,
>                                        MO_64, MEMTXATTRS_UNSPECIFIED);
>           break;
> -    case 0b001:
> -        /* Index Load Tag */
> +    case 0b001: /* Index Load Tag */
>           memory_region_dispatch_read(env->itc_tag, index, &env->CP0_TagLo,
>                                       MO_64, MEMTXATTRS_UNSPECIFIED);
>           break;
> +    case 0b000: /* Index Invalidate */
> +    case 0b100: /* Hit Invalidate */
> +    case 0b110: /* Hit Writeback */
> +        /* no-op */
> +        break;
>       default:
>           break;
>       }

[RFC PATCH v2 3/3] target/mips/op_helper: Log unimplemented cache opcode

[Philippe Mathieu-Daudé]

In case the guest uses a cache opcode we are not expecting,
log it to give us a chance to notice it, in case we should
actually do something.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/op_helper.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
index 2496d1dd71..a3b27f39db 100644
--- a/target/mips/op_helper.c
+++ b/target/mips/op_helper.c
@@ -1574,6 +1574,13 @@ void helper_msa_st_d(CPUMIPSState *env, uint32_t wd,
 void helper_cache(CPUMIPSState *env, target_ulong addr, uint32_t op)
 {
 #ifndef CONFIG_USER_ONLY
+    static const char *type_name[] = {
+        "Primary Instruction",
+        "Primary Data or Unified Primary",
+        "Tertiary",
+        "Secondary"
+    };
+    uint32_t cache_type = extract32(op, 0, 2);
     uint32_t cache_operation = extract32(op, 2, 3);
     target_ulong index = addr & 0x1fffffff;
 
@@ -1592,6 +1599,8 @@ void helper_cache(CPUMIPSState *env, target_ulong addr, uint32_t op)
         /* no-op */
         break;
     default:
+        qemu_log_mask(LOG_UNIMP, "cache operation:%u (type: %s cache)\n",
+                      cache_operation, type_name[cache_type]);
         break;
     }
 #endif
-- 
2.21.3

Re: [RFC PATCH v2 3/3] target/mips/op_helper: Log unimplemented cache opcode

[Richard Henderson]

On 8/13/20 11:15 AM, Philippe Mathieu-Daudé wrote:
>  #ifndef CONFIG_USER_ONLY
> +    static const char *type_name[] = {

const char * const

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC PATCH v2 0/3] target/mips: Report unimplemented cache() operations

[Philippe Mathieu-Daudé]

On 8/13/20 8:15 PM, Philippe Mathieu-Daudé wrote:
> Few patches to improve the handling of the 'cache' opcode.
> 
> I'm not sure why helper_cache() is generated for user-only mode.
> 
> v1 was:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg727959.html
> Supersedes: <20200806122612.17167-1-f4bug@amsat.org>
> 
> Philippe Mathieu-Daudé (3):
>    target/mips/op_helper: Convert multiple if() to switch case
>    target/mips/op_helper: Document Invalidate/Writeback opcodes as no-op
>    target/mips/op_helper: Log unimplemented cache opcode

Thanks, added to mips-next (addressing Richard comments).