diff mbox series

[PATCH-for-10.1,2/4] target/sparc: Restrict SPARC64 specific features

Message ID 20250325123927.74939-3-philmd@linaro.org
State New
Headers show
Series target/sparc: Spring cleanups around CPU features & LEON3 | expand

Commit Message

Philippe Mathieu-Daudé March 25, 2025, 12:39 p.m. UTC 
Following commit 554abe47c7b ("target/sparc: Partition cpu
features"), avoid compiling SPARC64 specific code on 32-bit
binary.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/sparc/cpu-feature.h.inc | 20 ++++++++++++--------
 target/sparc/translate.c       | 10 ++++++++--
 2 files changed, 20 insertions(+), 10 deletions(-)

Comments

Richard Henderson March 25, 2025, 2:07 p.m. UTC | #1 
On 3/25/25 05:39, Philippe Mathieu-Daudé wrote:
> @@ -2750,12 +2752,16 @@ static bool trans_SETHI(DisasContext *dc, arg_SETHI *a)
>   static bool do_tcc(DisasContext *dc, int cond, int cc,
>                      int rs1, bool imm, int rs2_or_imm)
>   {
> -    int mask = ((dc->def->features & CPU_FEATURE_HYPV) && supervisor(dc)
> -                ? UA2005_HTRAP_MASK : V8_TRAP_MASK);
> +    int mask = 0;
>       DisasCompare cmp;
>       TCGLabel *lab;
>       TCGv_i32 trap;
>   
> +#ifdef TARGET_SPARC64
> +    mask = ((dc->def->features & CPU_FEATURE_HYPV) && supervisor(dc))
> +           ? UA2005_HTRAP_MASK : V8_TRAP_MASK;
> +#endif

This is wrong.  The conversion could have been

   int mask = V8_TRAP_MASK;
#ifdef
   if (HYPV && super) {
     mask = UA2005_HTRAP_MASK;
   }
#endif

but that's an excellent reason not to have the ifdef at all.

If you want to hide the features from sparc32, so they don't show up on the command-line, 
fine.  But I think you want to introduce

#define CPU_FEATURE_HYPV 0

etc for sparc32 to automatically fail such tests as these without further modification.


r~
Philippe Mathieu-Daudé March 25, 2025, 2:41 p.m. UTC | #2 
On 25/3/25 15:07, Richard Henderson wrote:
> On 3/25/25 05:39, Philippe Mathieu-Daudé wrote:
>> @@ -2750,12 +2752,16 @@ static bool trans_SETHI(DisasContext *dc, 
>> arg_SETHI *a)
>>   static bool do_tcc(DisasContext *dc, int cond, int cc,
>>                      int rs1, bool imm, int rs2_or_imm)
>>   {
>> -    int mask = ((dc->def->features & CPU_FEATURE_HYPV) && supervisor(dc)
>> -                ? UA2005_HTRAP_MASK : V8_TRAP_MASK);
>> +    int mask = 0;
>>       DisasCompare cmp;
>>       TCGLabel *lab;
>>       TCGv_i32 trap;
>> +#ifdef TARGET_SPARC64
>> +    mask = ((dc->def->features & CPU_FEATURE_HYPV) && supervisor(dc))
>> +           ? UA2005_HTRAP_MASK : V8_TRAP_MASK;
>> +#endif
> 
> This is wrong.  The conversion could have been
> 
>    int mask = V8_TRAP_MASK;
> #ifdef
>    if (HYPV && super) {
>      mask = UA2005_HTRAP_MASK;
>    }
> #endif

Oh indeed. I guess I got confused by the parenthesis.

> but that's an excellent reason not to have the ifdef at all.
> 
> If you want to hide the features from sparc32, so they don't show up on 
> the command-line, fine.  But I think you want to introduce
> 
> #define CPU_FEATURE_HYPV 0
> 
> etc for sparc32 to automatically fail such tests as these without 
> further modification.

Hmm maybe not a big win after all since you said sparc32 and sparc64
will likely be considered as distinct architectures (in terms of QEMU
target implementations).
diff mbox series

Patch

diff --git a/target/sparc/cpu-feature.h.inc b/target/sparc/cpu-feature.h.inc
index be810052376..7b7b94a0562 100644
--- a/target/sparc/cpu-feature.h.inc
+++ b/target/sparc/cpu-feature.h.inc
@@ -1,12 +1,8 @@ 
 FEATURE(FLOAT128)
-FEATURE(MUL)
-FEATURE(DIV)
-FEATURE(VIS1)
-FEATURE(VIS2)
-FEATURE(FSMULD)
-FEATURE(HYPV)
-FEATURE(CMT)
-FEATURE(GL)
+FEATURE(MUL)            /* Mandatory in v9 */
+FEATURE(DIV)            /* Mandatory in v9 */
+FEATURE(FSMULD)         /* Mandatory in v9 */
+
 FEATURE(TA0_SHUTDOWN) /* Shutdown on "ta 0x0" */
 FEATURE(ASR17)
 FEATURE(CACHE_CTRL)
@@ -16,3 +12,11 @@  FEATURE(FMAF)
 FEATURE(VIS3)
 FEATURE(IMA)
 FEATURE(VIS4)
+
+#ifdef TARGET_SPARC64
+FEATURE(HYPV)
+FEATURE(CMT)
+FEATURE(GL)
+FEATURE(VIS1)
+FEATURE(VIS2)
+#endif
diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index bfe63649db2..53b145848b9 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -1850,10 +1850,12 @@  static void gen_st_asi(DisasContext *dc, DisasASI *da, TCGv src, TCGv addr)
         if (TARGET_LONG_BITS == 32) {
             gen_exception(dc, TT_ILL_INSN);
             break;
+#ifdef TARGET_SPARC64
         } else if (!(dc->def->features & CPU_FEATURE_HYPV)) {
             /* Pre OpenSPARC CPUs don't have these */
             gen_exception(dc, TT_ILL_INSN);
             break;
+#endif
         }
         /* In OpenSPARC T1+ CPUs TWINX ASIs in store are ST_BLKINIT_ ASIs */
         /* fall through */
@@ -2750,12 +2752,16 @@  static bool trans_SETHI(DisasContext *dc, arg_SETHI *a)
 static bool do_tcc(DisasContext *dc, int cond, int cc,
                    int rs1, bool imm, int rs2_or_imm)
 {
-    int mask = ((dc->def->features & CPU_FEATURE_HYPV) && supervisor(dc)
-                ? UA2005_HTRAP_MASK : V8_TRAP_MASK);
+    int mask = 0;
     DisasCompare cmp;
     TCGLabel *lab;
     TCGv_i32 trap;
 
+#ifdef TARGET_SPARC64
+    mask = ((dc->def->features & CPU_FEATURE_HYPV) && supervisor(dc))
+           ? UA2005_HTRAP_MASK : V8_TRAP_MASK;
+#endif
+
     /* Trap never.  */
     if (cond == 0) {
         return advance_pc(dc);