Re: [PATCH v4 1/2] target/riscv: separate priv from mmu_idx

[Richard Henderson]

On 3/23/23 18:20, Wu, Fei wrote:
> I lack some background here, why should tb_flags be preferred if env has
> the same info? Then for reading from tb_flags, we need to add it to
> tb_flags first.

We read from tb_flags in translate because that proves we've added the data to tb_flags 
for the TB hash.  It avoids errors like the one for vstart.

> Correct me if I'm wrong. The only strong reason we add some flag to
> tb_flags is that it causes codegen generate different code because of
> this flag change, and it looks like priv is not one of them, neither is
> mmu_idx, the generated code does use mmu_idx, but no different code
> generated for it.

PRIV definitely affects the generated code: for a supervisor instruction, such as 
REQUIRE_PRIV_MS, we emit gen_exception_illegal() if PRIV == U.

MMU_IDX definitely affects the generated code, because that immediate value makes its way 
into the memory offsets in the softmmu lookup sequence.  Have a look at the output of -d 
op_opt,out_asm.

> I think here we have some other reasons to include more, e.g. reference
> env can be error-prone in some way. So there are minimal flags must be
> in tb_flags, but we think it's better to add more?

We add the ones required for efficiency of execution.

We had not originally added PRIV, because the (original) few instructions in 
trans_privileged.c.inc all call helpers anyway, so it was easy enough to check PRIV in the 
helper.

Since then more uses have grown.  We *could* turn those into helper functions as well, but 
every other guest arch includes the privilege level in tb_flags, and it seems natural to 
do so.  Only if you completely run out of bits would I consider working hard to eliminate 
that one.


r~