Re: [PATCH 06/26] target/mips: Extract load/store helpers to ldst_helper.c

[Philippe Mathieu-Daudé]

On 4/18/21 9:08 PM, Richard Henderson wrote:
> On 4/18/21 9:31 AM, Philippe Mathieu-Daudé wrote:
>> +static inline hwaddr do_translate_address(CPUMIPSState *env,
>> +                                          target_ulong address,
>> +                                          MMUAccessType access_type,
>> +                                          uintptr_t retaddr)
>> +{
>> +    hwaddr paddr;
>> +    CPUState *cs = env_cpu(env);
>> +
>> +    paddr = cpu_mips_translate_address(env, address, access_type);
>> +
>> +    if (paddr == -1LL) {
>> +        cpu_loop_exit_restore(cs, retaddr);
>> +    } else {
>> +        return paddr;
>> +    }
>> +}
> 
> Code motion, so,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> but I think that it's a mistake for cpu_mips_translate_address to split
> the raise_mmu_exception from the cpu_loop_exit_restore.  If you make the
> changes to env to indicate the exception, you *must* then go back to the
> main loop.
> 
> There seems to be exactly one caller, this one, so it should be trivial
> to change, which the lets do_translate_address vanish entirely.

I'm taking note of this comment and will try to address it later.