Re: [PATCH] target/tricore: Remove unused fields from CPUTriCoreState

[Bastian Koppelmann]

Hi Richard,

On Tue, Jan 17, 2023 at 11:01:37AM -1000, Richard Henderson wrote:
> On 1/17/23 08:42, Philippe Mathieu-Daudé wrote:
> > Remove dead code:
> > - unused fields in CPUTriCoreState
> > - (unexisting) tricore_def_t structure
> > - forward declaration of tricore_boot_info structure
> >    (declared in "hw/tricore/tricore.h", used once in
> >     hw/tricore/tricore_testboard.c).
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> Given this compiles,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> It did surprise me though, since I would have expected something to use
> hflags.  It turns out the only thing that uses TRICORE_HFLAG_* is the kernel
> vs user-mode bits.

I'm not sure of the purpose of the hflags. I assume they are hidden flags that
hold some hidden state of the emulated CPU. However are privilege levels really
hidden state? At least in the TriCore any guest software can read PSW at any
privilege level and see its own privilige level. Is there a typical usecase when
I would use hflags?

Let's say I want to implement privilege levels using hflags anyways. In the MIPS
targets I see hflags as part of the DisasCtx and CPUState structs. There are
also flags in the TranslationBlock struct. I assume that CPUState holds the
"real" value (synced with the TriCore PSW reg) and the other two hold copies.
When we translate a TB we copy hflags from the CPUState into the
TranslationBlock using cpu_get_tb_cpu_state() first. During target translation
we copy it from the TranslationBlock to DisasCtx using tr_init_disas_context().
>From DisasCtx it is used to make sure only supervisor mode can write CSFRs. Is
this correct so far?

I do wonder why there is the extra step via the flags of a TranslationBlock. Why
can't we sync directly using CPUState? I first thought to differentiated the
TranslationBlocks for the different privilege level. However isn't that done
using the softmmu_idx? Thus if the guest software changes privilege level,
we change the softmmu_idx and have to retranslate code for the new
privilege level. Therefor we run target translation again and can sync DisasCtx
from CPUState.

> 
> Bastian, there is code missing from cpu_get_tb_cpu_state, to copy
> env->PSW[11:10] to *flags.  At present it would seem that all code effectively
> runs in kernel mode.

Yes, everything runs in the highest privilege mode. I'll look into properly
implementing privilege levels, but for now we can remove this dead code.

Thanks & cheers,
Bastian