Re: [PATCH v2 1/5] target/riscv: Add a virtualised MMU Mode

[Richard Henderson]

On 10/28/20 1:51 PM, Alistair Francis wrote:
>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>> index 3eb3a034db..453e4c6d8a 100644
>>> --- a/target/riscv/cpu_helper.c
>>> +++ b/target/riscv/cpu_helper.c
>>> @@ -30,6 +30,10 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
>>>  #ifdef CONFIG_USER_ONLY
>>>      return 0;
>>>  #else
>>> +    if (riscv_cpu_virt_enabled(env)) {
>>> +        return env->priv | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
>>> +    }
>>
>> This is wrong.  You only want to set this flag in response to one of the
>> hypervisor special instructions.  This is setting it any time virt is 
>> enabled.
> 
> Isn't that ok though? I thought this was the correct approach.

No.

Consider: The *presence* of this bit means a change of behaviour in
get_physical_address.

Things are mostly working for you because you then mask this bit off when
installing it to TBFLAGS.  Which you then use during translate without adding
it back on, except for the one place you need it.

The things that won't work are generic bits of code which use e.g.
cpu_ldub_data(), which calls cpu_mmu_index(), change behaviour.  Which you
don't want.


r~