Re: [PATCH v3 4/6] linux-user/elfload: Introduce MIPS GET_FEATURE_REG_EQU() macro

[Philippe Mathieu-Daudé]

On 12/2/20 12:15 AM, Richard Henderson wrote:
> On 12/1/20 1:28 PM, Philippe Mathieu-Daudé wrote:
>> ISA features are usually denoted in read-only bits from
>> CPU registers. Add the GET_FEATURE_REG_EQU() macro which
>> checks if a CPU register has bits set to a specific value.
>>
>> Use the macro to check the 'Architecture Revision' level
>> of the Config0 register, which is '2' when the Release 6
>> ISA is implemented.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  linux-user/elfload.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>> index b7c6d30723a..9c475fa5f70 100644
>> --- a/linux-user/elfload.c
>> +++ b/linux-user/elfload.c
>> @@ -7,6 +7,7 @@
>>  
>>  #include "qemu.h"
>>  #include "disas/disas.h"
>> +#include "qemu/bitops.h"
>>  #include "qemu/path.h"
>>  #include "qemu/queue.h"
>>  #include "qemu/guest-random.h"
>> @@ -995,17 +996,25 @@ enum {
>>  #define GET_FEATURE_REG_SET(_reg, _mask, _hwcap) \
>>      do { if (cpu->env._reg & (_mask)) { hwcaps |= _hwcap; } } while (0)
>>  
>> +#define GET_FEATURE_REG_EQU(_reg, _start, _length, _val, _hwcap) \
>> +    do { \
>> +        if (extract32(cpu->env._reg, (_start), (_length)) == (_val)) { \
>> +            hwcaps |= _hwcap; \
>> +        } \
>> +    } while (0)
>> +
>>  static uint32_t get_elf_hwcap(void)
>>  {
>>      MIPSCPU *cpu = MIPS_CPU(thread_cpu);
>>      uint32_t hwcaps = 0;
>>  
>> -    GET_FEATURE_INSN(ISA_MIPS32R6 | ISA_MIPS64R6, HWCAP_MIPS_R6);
>> +    GET_FEATURE_REG_EQU(CP0_Config0, CP0C0_AR, 3, 2, HWCAP_MIPS_R6);
> 
> You still get the magic 3.

TBH:

$ wc -l target/mips/cpu.h
1323 target/mips/cpu.h
$ egrep _MASK target/mips/cpu.h | wc -l
19

And mask definitions are not useful to use with extract/deposit:

$ egrep _MASK target/mips/cpu.h
#define MSACSR_FS_MASK  (1 << MSACSR_FS)
#define MSACSR_NX_MASK  (1 << MSACSR_NX)
#define MSACSR_CEF_MASK (0xffff << MSACSR_CEF)
#define MSACSR_RM_MASK  (0x3 << MSACSR_RM)
#define CP0PM_MASK 13
#define CP0SC_PA_MASK   (0x7FULL << CP0SC_PA)
#define CP0SC_AM_MASK   (0x7ULL << CP0SC_AM)
#define CP0SC_EU_MASK   (1ULL << CP0SC_EU)
#define CP0SC_C_MASK    (0x7ULL << CP0SC_C)
...

If you rather I can amend this snippet:

-- >8 --
diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 23f8c6f96cd..2639b0ea06c 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -844,6 +844,7 @@ struct CPUMIPSState {
 #define CP0C0_MT   7     /*  9..7  */
 #define CP0C0_VI   3
 #define CP0C0_K0   0     /*  2..0  */
+#define CP0C0_AR_LENGTH 3
     int32_t CP0_Config1;
 #define CP0C1_M    31
 #define CP0C1_MMU  25    /* 30..25 */
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 5a39a7dc021..a64050713f2 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1021,7 +1021,8 @@ static uint32_t get_elf_hwcap(void)
     MIPSCPU *cpu = MIPS_CPU(thread_cpu);
     uint32_t hwcaps = 0;

-    GET_FEATURE_REG_EQU(CP0_Config0, CP0C0_AR, 3, 2, HWCAP_MIPS_R6);
+    GET_FEATURE_REG_EQU(CP0_Config0, CP0C0_AR, CP0C0_AR_LENGTH,
+                        2, HWCAP_MIPS_R6);
     GET_FEATURE_REG_SET(CP0_Config3, 1 << CP0C3_MSAP, HWCAP_MIPS_MSA);
     GET_FEATURE_INSN(ASE_LMMI, HWCAP_LOONGSON_MMI);
     GET_FEATURE_INSN(ASE_LEXT, HWCAP_LOONGSON_EXT);
---

> This is where hw/registerfields.h would come in handy.  But that is certainly 
> a
> large change to mips' cpu.h.  So I guess this is good enough for now.

Yes, that is my preference too, and I plan to get there eventually
(this is on my TODO list). But from here to there is a long way...

First I'd get rid of the cpu->env.insn_flags duplications.

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

Thanks!

Phil.