Re: [PATCH v2 02/12] target/mips/mips-defs: Use ISA_MIPS3 for ISA_MIPS64

[Philippe Mathieu-Daudé]

On 12/16/20 8:08 PM, Richard Henderson wrote:
> On 12/16/20 10:27 AM, Philippe Mathieu-Daudé wrote:
>> MIPS 64-bit ISA is introduced with MIPS3.
>> No need for another bit/definition to check for 64-bit.
>>
>> Suggested-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  target/mips/mips-defs.h | 2 +-
>>  hw/mips/boston.c        | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/mips/mips-defs.h b/target/mips/mips-defs.h
>> index f4d76e562d1..ab621a750d5 100644
>> --- a/target/mips/mips-defs.h
>> +++ b/target/mips/mips-defs.h
>> @@ -19,7 +19,7 @@
>>   */
>>  #define ISA_MIPS1         0x0000000000000001ULL
>>  #define ISA_MIPS2         0x0000000000000002ULL
>> -#define ISA_MIPS3         0x0000000000000004ULL
>> +#define ISA_MIPS3         0x0000000000000004ULL /* 64-bit */
>>  #define ISA_MIPS4         0x0000000000000008ULL
>>  #define ISA_MIPS5         0x0000000000000010ULL
>>  #define ISA_MIPS32        0x0000000000000020ULL
>> diff --git a/hw/mips/boston.c b/hw/mips/boston.c
>> index c3b94c68e1b..f44f681fab5 100644
>> --- a/hw/mips/boston.c
>> +++ b/hw/mips/boston.c
>> @@ -463,7 +463,7 @@ static void boston_mach_init(MachineState *machine)
>>          exit(1);
>>      }
>>  
>> -    is_64b = cpu_type_supports_isa(machine->cpu_type, ISA_MIPS64);
>> +    is_64b = cpu_type_supports_isa(machine->cpu_type, ISA_MIPS3);
> 
> Find this slightly confusing.
> 
> After all of the renaming, I would expect ISA_MIPS64R6 -> ISA_MIPS_R6 |
> ISA_MIPS_64, not ISA_MIPS_R6 | ISA_MIPS3.

Well all the ISA_* definitions now match:
https://images.anandtech.com/doci/8457/MIPS%20ISA%20Evolution.JPG

Except ISA_NANOMIPS32, which is listed in
MD01251-2B-nanoMIPS32PRA-06.09.pdf as an extension, similar to microMIPS:

  MIPS32, microMIPS32, and nanoMIPS32 Operating Modes

  Release 2 of the MIPS32 Architecture added support for 64-bit
  coprocessors (and, in particular, 64-bit floating-point units)
  with 32-bit CPUs. Thus, certain floating-point instructions
  that previously were enabled by 64-bit operations on a MIPS64
  processor now are enabled by new 64-bit floating-point operations.

  Release 3 introduced the microMIPS instruction set, allowing all
  microMIPS processors to implement a 64-bit floating-point unit.

  Release 6 introduces the nanoMIPS instruction set. The nanoMIPS
  instruction set provides access to the same instruction set extensions
  (example, COP1 floating-point instructions) that microMIPS had access
  to.

I'd rather keep one definitions per ISA. Eventually if you want
a definition to check if a CPU is 32/64-bit we can add an alias:

-- >8 --
diff --git a/target/mips/mips-defs.h b/target/mips/mips-defs.h
index 376262fa250..2c3f4277cfe 100644
--- a/target/mips/mips-defs.h
+++ b/target/mips/mips-defs.h
@@ -65,6 +65,8 @@
 #define CPU_LOONGSON2E  (CPU_MIPS3 | INSN_LOONGSON2E)
 #define CPU_LOONGSON2F  (CPU_MIPS3 | INSN_LOONGSON2F | ASE_LMMI)

+#define CPU_MIPS64      (ISA_MIPS3)
+
 /* MIPS Technologies "Release 1" */
 #define CPU_MIPS32R1    (CPU_MIPS2 | ISA_MIPS_R1)
 #define CPU_MIPS64R1    (CPU_MIPS5 | CPU_MIPS32R1)
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index f44f681fab5..9f56099e42f 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -463,7 +463,7 @@ static void boston_mach_init(MachineState *machine)
         exit(1);
     }

-    is_64b = cpu_type_supports_isa(machine->cpu_type, ISA_MIPS3);
+    is_64b = cpu_type_supports_isa(machine->cpu_type, CPU_MIPS64);

     object_initialize_child(OBJECT(machine), "cps", &s->cps,
TYPE_MIPS_CPS);
     object_property_set_str(OBJECT(&s->cps), "cpu-type", machine->cpu_type,
---

But for the Boston case, it is simpler to add an inline function in
cpu.h:

-- >8 --
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -1302,6 +1302,11 @@ static inline bool ase_mt_available(CPUMIPSState
*env)
     return env->CP0_Config3 & (1 << CP0C3_MT);
 }

+static inline bool cpu_type_is_64bit(const char *cpu_type)
+{
+    return cpu_type_supports_isa(cpu_type, CPU_MIPS64);
+}
+
 void cpu_set_exception_base(int vp_index, target_ulong address);

 /* addr.c */
---

Note, I'd still use ISA_MIPS3 in this cpu_type_is_64bit().

Or I could add the ISA_MIPS_64 alias and call it a day...

> 
> 
> r~
>