Re: [Qemu-devel] [PATCH v1 05/21] RISC-V CPU Helpers

[Richard Henderson]

On 01/10/2018 02:35 AM, Stefan O'Rear wrote:
> On Tue, Jan 2, 2018 at 11:12 PM, Richard Henderson
> <address@hidden> wrote:
>>> +    case CSR_MISA: {
>>> +        if (!(val_to_write & (1L << ('F' - 'A')))) {
>>> +            val_to_write &= ~(1L << ('D' - 'A'));
>>> +        }
>>> +
>>> +        /* allow MAFDC bits in MISA to be modified */
>>> +        target_ulong mask = 0;
>>> +        mask |= 1L << ('M' - 'A');
>>> +        mask |= 1L << ('A' - 'A');
>>> +        mask |= 1L << ('F' - 'A');
>>> +        mask |= 1L << ('D' - 'A');
>>> +        mask |= 1L << ('C' - 'A');
>>> +        mask &= env->misa_mask;
>>> +
>>> +        env->misa = (val_to_write & mask) | (env->misa & ~mask);
>>
>> Does this not affect the set of instructions that are allowable?  If so, 
>> you'd
>> want something like
>>
>>     new_misa = (val_to_write & mask) | (env->misa & ~mask);
>>     if (env->misa != new_misa) {
>>         env->misa = new_misa;
>>         tb_flush(CPU(riscv_env_get_cpu(env)));
>>     }
>>
>> so that we start with all new translations, which would then check the new
>> value of misa, and would then raise INST_ADDR_MIS (or not).
> 
> This does not seem quite right.  misa can legally differ between
> cores/threads, but tb_flush is a global operation.  The way this is
> supposed to work is that the relevant misa bits are extracted into
> tb_flags:
> 
> static inline void cpu_riscv_set_tb_flags(CPURISCVState *env)
> {
>     env->tb_flags = 0;
>     if (env->misa & MISA_A) {
>        env->tb_flags |= RISCV_TF_MISA_A;
>     }
> 
>     if (env->misa & MISA_D) {
>        env->tb_flags |= RISCV_TF_MISA_D;
>     }
> 
>     if (env->misa & MISA_F) {
>         env->tb_flags |= RISCV_TF_MISA_F;
>     }
> 
>     if (env->misa & MISA_M) {
>         env->tb_flags |= RISCV_TF_MISA_M;
>     }
> 
>     if (env->misa & MISA_C) {
>         env->tb_flags |= RISCV_TF_MISA_C;
>     }
> 
>     env->tb_flags |= cpu_mmu_index(env, true) << RISCV_TF_IAT_SHIFT;
>     env->tb_flags |= cpu_mmu_index(env, false) << RISCV_TF_DAT_SHIFT;
> }
> 
> static inline void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
>                                         target_ulong *cs_base, uint32_t 
> *flags)
> {
>     *pc = env->pc;
>     *cs_base = 0;
>     *flags = env->tb_flags;
> }
> 
> but this code appears to be missing in the tree submitted for upstreaming?

Ah hah.  Yes, this is another completely valid way to accomplish this.

I am also glad that you are thinking about the computational overhead of
cpu_get_tb_cpu_state.  With lookup_and_goto_ptr, it is in the hot path of
indirect branching.


r~