[PATCH 0/5] mips: Sanitize Multi-Threading ASE

[PATCH 0/5] mips: Sanitize Multi-Threading ASE

[Philippe Mathieu-Daudé]

Reviewing the MIPS code, ASE after ASE.
Time for MT ASE.

- Introduce/use ase_mt_available() helper to check
  if MT ASE is present
- Avoid setting MT specific registers if MT ASE is absent

Philippe Mathieu-Daudé (5):
  target/mips: Remove mips_def_t unused argument from mvp_init()
  target/mips: Introduce ase_mt_available() helper
  target/mips: Do not initialize MT registers if MT ASE absent
  hw/mips/malta: Do not initialize MT registers if MT ASE absent
  hw/mips/malta: Rewrite CP0_MVPConf0 access using deposit()

 target/mips/cpu.h                |  7 +++++++
 hw/mips/cps.c                    |  3 +--
 hw/mips/malta.c                  | 10 ++++++++--
 target/mips/cp0_helper.c         |  2 +-
 target/mips/cpu.c                |  2 +-
 target/mips/helper.c             |  2 +-
 target/mips/translate.c          |  4 ++--
 target/mips/translate_init.c.inc |  6 +++++-
 8 files changed, 26 insertions(+), 10 deletions(-)

-- 
2.26.2

[PATCH 1/5] target/mips: Remove mips_def_t unused argument from mvp_init()

[Philippe Mathieu-Daudé]

mvp_init() doesn't require any CPU definition (beside the
information accessible via CPUMIPSState). Remove the unused
argument.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/translate.c          | 2 +-
 target/mips/translate_init.c.inc | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index c64a1bc42e1..0db032fc5fb 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -31767,7 +31767,7 @@ void cpu_mips_realize_env(CPUMIPSState *env)
     mmu_init(env, env->cpu_model);
 #endif
     fpu_init(env, env->cpu_model);
-    mvp_init(env, env->cpu_model);
+    mvp_init(env);
 }
 
 bool cpu_supports_cps_smp(const char *cpu_type)
diff --git a/target/mips/translate_init.c.inc b/target/mips/translate_init.c.inc
index 79f75ed863c..5a926bc6df3 100644
--- a/target/mips/translate_init.c.inc
+++ b/target/mips/translate_init.c.inc
@@ -989,7 +989,7 @@ static void fpu_init (CPUMIPSState *env, const mips_def_t *def)
     memcpy(&env->active_fpu, &env->fpus[0], sizeof(env->active_fpu));
 }
 
-static void mvp_init (CPUMIPSState *env, const mips_def_t *def)
+static void mvp_init(CPUMIPSState *env)
 {
     env->mvp = g_malloc0(sizeof(CPUMIPSMVPContext));
 
-- 
2.26.2

Re: [PATCH 1/5] target/mips: Remove mips_def_t unused argument from mvp_init()

[Richard Henderson]

On 12/4/20 4:26 PM, Philippe Mathieu-Daudé wrote:
> mvp_init() doesn't require any CPU definition (beside the
> information accessible via CPUMIPSState). Remove the unused
> argument.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/mips/translate.c          | 2 +-
>  target/mips/translate_init.c.inc | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

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

r~

[PATCH 2/5] target/mips: Introduce ase_mt_available() helper

[Philippe Mathieu-Daudé]

Instead of accessing CP0_Config3 directly and checking
the 'Multi-Threading Present' bit, introduce an helper
to simplify code review.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/cpu.h        | 7 +++++++
 hw/mips/cps.c            | 3 +--
 target/mips/cp0_helper.c | 2 +-
 target/mips/cpu.c        | 2 +-
 target/mips/helper.c     | 2 +-
 target/mips/translate.c  | 2 +-
 6 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 2639b0ea06c..82c60a34751 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -1289,6 +1289,13 @@ int cpu_mips_signal_handler(int host_signum, void *pinfo, void *puc);
 
 bool cpu_supports_cps_smp(const char *cpu_type);
 bool cpu_supports_isa(const char *cpu_type, uint64_t isa);
+
+/* Check presence of multi-threading ASE implementation */
+static inline bool ase_mt_available(CPUMIPSState *env)
+{
+    return env->CP0_Config3 & (1 << CP0C3_MT);
+}
+
 void cpu_set_exception_base(int vp_index, target_ulong address);
 
 /* mips_int.c */
diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index 962b1b0b87c..7a0d289efaf 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -58,8 +58,7 @@ static void main_cpu_reset(void *opaque)
 
 static bool cpu_mips_itu_supported(CPUMIPSState *env)
 {
-    bool is_mt = (env->CP0_Config5 & (1 << CP0C5_VP)) ||
-                 (env->CP0_Config3 & (1 << CP0C3_MT));
+    bool is_mt = (env->CP0_Config5 & (1 << CP0C5_VP)) || ase_mt_available(env);
 
     return is_mt && !kvm_enabled();
 }
diff --git a/target/mips/cp0_helper.c b/target/mips/cp0_helper.c
index caaaefcc8ad..9718c93d18c 100644
--- a/target/mips/cp0_helper.c
+++ b/target/mips/cp0_helper.c
@@ -1166,7 +1166,7 @@ void helper_mtc0_entryhi(CPUMIPSState *env, target_ulong arg1)
     old = env->CP0_EntryHi;
     val = (arg1 & mask) | (old & ~mask);
     env->CP0_EntryHi = val;
-    if (env->CP0_Config3 & (1 << CP0C3_MT)) {
+    if (ase_mt_available(env)) {
         sync_c0_entryhi(env, env->current_tc);
     }
     /* If the ASID changes, flush qemu's TLB.  */
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 76d50b00b42..c03e5acf5bc 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -74,7 +74,7 @@ static bool mips_cpu_has_work(CPUState *cs)
     }
 
     /* MIPS-MT has the ability to halt the CPU.  */
-    if (env->CP0_Config3 & (1 << CP0C3_MT)) {
+    if (ase_mt_available(env)) {
         /*
          * The QEMU model will issue an _WAKE request whenever the CPUs
          * should be woken up.
diff --git a/target/mips/helper.c b/target/mips/helper.c
index cc46ea887e5..608fe1512a3 100644
--- a/target/mips/helper.c
+++ b/target/mips/helper.c
@@ -419,7 +419,7 @@ void cpu_mips_store_status(CPUMIPSState *env, target_ulong val)
         tlb_flush(env_cpu(env));
     }
 #endif
-    if (env->CP0_Config3 & (1 << CP0C3_MT)) {
+    if (ase_mt_available(env)) {
         sync_c0_status(env, env, env->current_tc);
     } else {
         compute_hflags(env);
diff --git a/target/mips/translate.c b/target/mips/translate.c
index 0db032fc5fb..ee45dce9a50 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -31921,7 +31921,7 @@ void cpu_state_reset(CPUMIPSState *env)
 
     cpu_mips_store_count(env, 1);
 
-    if (env->CP0_Config3 & (1 << CP0C3_MT)) {
+    if (ase_mt_available(env)) {
         int i;
 
         /* Only TC0 on VPE 0 starts as active.  */
-- 
2.26.2

Re: [PATCH 2/5] target/mips: Introduce ase_mt_available() helper

[Richard Henderson]

On 12/4/20 4:26 PM, Philippe Mathieu-Daudé wrote:
> Instead of accessing CP0_Config3 directly and checking
> the 'Multi-Threading Present' bit, introduce an helper
> to simplify code review.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/mips/cpu.h        | 7 +++++++
>  hw/mips/cps.c            | 3 +--
>  target/mips/cp0_helper.c | 2 +-
>  target/mips/cpu.c        | 2 +-
>  target/mips/helper.c     | 2 +-
>  target/mips/translate.c  | 2 +-
>  6 files changed, 12 insertions(+), 6 deletions(-)

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

r~

[PATCH 3/5] target/mips: Do not initialize MT registers if MT ASE absent

[Philippe Mathieu-Daudé]

Do not initialize MT-related config registers if the MT ASE
is not present. As some functions access the 'mvp' structure,
we still zero-allocate it.

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

diff --git a/target/mips/translate_init.c.inc b/target/mips/translate_init.c.inc
index 5a926bc6df3..f72fee3b40a 100644
--- a/target/mips/translate_init.c.inc
+++ b/target/mips/translate_init.c.inc
@@ -993,6 +993,10 @@ static void mvp_init(CPUMIPSState *env)
 {
     env->mvp = g_malloc0(sizeof(CPUMIPSMVPContext));
 
+    if (!ase_mt_available(env)) {
+        return;
+    }
+
     /* MVPConf1 implemented, TLB sharable, no gating storage support,
        programmable cache partitioning implemented, number of allocatable
        and shareable TLB entries, MVP has allocatable TCs, 2 VPEs
-- 
2.26.2

Re: [PATCH 3/5] target/mips: Do not initialize MT registers if MT ASE absent

[Richard Henderson]

On 12/4/20 4:26 PM, Philippe Mathieu-Daudé wrote:
> Do not initialize MT-related config registers if the MT ASE
> is not present. As some functions access the 'mvp' structure,
> we still zero-allocate it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/mips/translate_init.c.inc | 4 ++++
>  1 file changed, 4 insertions(+)

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

r~

[PATCH 4/5] hw/mips/malta: Do not initialize MT registers if MT ASE absent

[Philippe Mathieu-Daudé]

Do not initialize MT-related config register if the MT ASE
is not present.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/mips/malta.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index 9d1a3b50b7a..350b92b4d79 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1134,8 +1134,10 @@ static void malta_mips_config(MIPSCPU *cpu)
     CPUMIPSState *env = &cpu->env;
     CPUState *cs = CPU(cpu);
 
-    env->mvp->CP0_MVPConf0 |= ((smp_cpus - 1) << CP0MVPC0_PVPE) |
+    if (ase_mt_available(env)) {
+        env->mvp->CP0_MVPConf0 |= ((smp_cpus - 1) << CP0MVPC0_PVPE) |
                          ((smp_cpus * cs->nr_threads - 1) << CP0MVPC0_PTC);
+    }
 }
 
 static void main_cpu_reset(void *opaque)
-- 
2.26.2

Re: [PATCH 4/5] hw/mips/malta: Do not initialize MT registers if MT ASE absent

[Richard Henderson]

On 12/4/20 4:26 PM, Philippe Mathieu-Daudé wrote:
> Do not initialize MT-related config register if the MT ASE
> is not present.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/mips/malta.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

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

r~

[PATCH 5/5] hw/mips/malta: Rewrite CP0_MVPConf0 access using deposit()

[Philippe Mathieu-Daudé]

PTC field has 8 bits, PVPE has 4. We plan to use the
"hw/registerfields.h" API with MIPS CPU definitions
(target/mips/cpu.h). Meanwhile we use magic 8 and 4.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
We want to move that to mips_cpu_reset() later,
because this is not Malta specific but cpu-specific.
However SMP 'cpus' come from MachineState ("hw/boards.h").
So meanwhile this is early review.
---
 hw/mips/malta.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index 350b92b4d79..c35fbf97272 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -24,6 +24,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/units.h"
+#include "qemu/bitops.h"
 #include "qemu-common.h"
 #include "cpu.h"
 #include "hw/clock.h"
@@ -1135,8 +1136,11 @@ static void malta_mips_config(MIPSCPU *cpu)
     CPUState *cs = CPU(cpu);
 
     if (ase_mt_available(env)) {
-        env->mvp->CP0_MVPConf0 |= ((smp_cpus - 1) << CP0MVPC0_PVPE) |
-                         ((smp_cpus * cs->nr_threads - 1) << CP0MVPC0_PTC);
+        env->mvp->CP0_MVPConf0 = deposit32(env->mvp->CP0_MVPConf0,
+                                           CP0MVPC0_PTC, 8,
+                                           smp_cpus * cs->nr_threads - 1);
+        env->mvp->CP0_MVPConf0 = deposit32(env->mvp->CP0_MVPConf0,
+                                           CP0MVPC0_PVPE, 4, smp_cpus - 1);
     }
 }
 
-- 
2.26.2

Re: [PATCH 5/5] hw/mips/malta: Rewrite CP0_MVPConf0 access using deposit()

[Richard Henderson]

On 12/4/20 4:26 PM, Philippe Mathieu-Daudé wrote:
> PTC field has 8 bits, PVPE has 4. We plan to use the
> "hw/registerfields.h" API with MIPS CPU definitions
> (target/mips/cpu.h). Meanwhile we use magic 8 and 4.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> We want to move that to mips_cpu_reset() later,
> because this is not Malta specific but cpu-specific.
> However SMP 'cpus' come from MachineState ("hw/boards.h").
> So meanwhile this is early review.
> ---
>  hw/mips/malta.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

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

r~

Re: [PATCH 0/5] mips: Sanitize Multi-Threading ASE

[Philippe Mathieu-Daudé]

On 12/4/20 11:26 PM, Philippe Mathieu-Daudé wrote:
> Reviewing the MIPS code, ASE after ASE.
> Time for MT ASE.
> 
> - Introduce/use ase_mt_available() helper to check
>   if MT ASE is present
> - Avoid setting MT specific registers if MT ASE is absent
> 
> Philippe Mathieu-Daudé (5):
>   target/mips: Remove mips_def_t unused argument from mvp_init()
>   target/mips: Introduce ase_mt_available() helper
>   target/mips: Do not initialize MT registers if MT ASE absent
>   hw/mips/malta: Do not initialize MT registers if MT ASE absent
>   hw/mips/malta: Rewrite CP0_MVPConf0 access using deposit()
> 
>  target/mips/cpu.h                |  7 +++++++
>  hw/mips/cps.c                    |  3 +--
>  hw/mips/malta.c                  | 10 ++++++++--
>  target/mips/cp0_helper.c         |  2 +-
>  target/mips/cpu.c                |  2 +-
>  target/mips/helper.c             |  2 +-
>  target/mips/translate.c          |  4 ++--
>  target/mips/translate_init.c.inc |  6 +++++-
>  8 files changed, 26 insertions(+), 10 deletions(-)

Thanks, applied to mips-next.