Re: [PATCH v2 10/23] target/riscv: Use MMU_INDEX() helper

[Helge Deller <deller@gmx.de>]

On 8/6/23 16:30, Richard Henderson wrote:
> On 8/6/23 05:17, Helge Deller wrote:
>> Use the new MMU_INDEX() helper to specify the index of the CPUTLB which
>> should be used.  Additionally, in a follow-up patch this helper allows
>> then to optimize the tcg code generation.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> ---
>>   target/riscv/cpu.h        | 4 ++--
>>   target/riscv/cpu_helper.c | 2 +-
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 6ea22e0eea..6aba1df64a 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -88,7 +88,7 @@ typedef enum {
>>       EXT_STATUS_DIRTY,
>>   } RISCVExtStatus;
>>
>> -#define MMU_USER_IDX 3
>> +#define MMU_USER_IDX MMU_INDEX(3)
>>
>>   #define MAX_RISCV_PMPS (16)
>>
>> @@ -446,7 +446,7 @@ void riscv_cpu_list(void);
>>   void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp);
>>
>>   #define cpu_list riscv_cpu_list
>> -#define cpu_mmu_index riscv_cpu_mmu_index
>> +#define cpu_mmu_index(e,i)      MMU_INDEX(riscv_cpu_mmu_index(e,i))
>>
>>   #ifndef CONFIG_USER_ONLY
>>   void riscv_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> index 9f611d89bb..a8e6950217 100644
>> --- a/target/riscv/cpu_helper.c
>> +++ b/target/riscv/cpu_helper.c
>> @@ -107,7 +107,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
>>   #else
>>       flags = FIELD_DP32(flags, TB_FLAGS, PRIV, env->priv);
>>
>> -    flags |= cpu_mmu_index(env, 0);
>> +    flags |= riscv_cpu_mmu_index(env, 0);
>>       fs = get_field(env->mstatus, MSTATUS_FS);
>>       vs = get_field(env->mstatus, MSTATUS_VS);
>
> This is the sort of non-obvious changes that I hoped to avoid by restricting all changes to accel/tcg/cputlb.c.

True.
And, since I've found some other missing pieces now too (e.g. in hppa)
I'm currently tempted to fully agree with you, that handling this
in accel/tcg/cputlb.c only is the better (and cleaner) solution.

I'll try you approach.

Helge