| Message ID | 20230117184217.83305-1-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/tricore: Remove unused fields from CPUTriCoreState | expand |
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. 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. r~
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
On Tue, Jan 17, 2023 at 07:42:17PM +0100, 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> > --- > target/tricore/cpu.h | 11 ----------- > 1 file changed, 11 deletions(-) Reviewed-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de> I assume this goes through qemu-trivial, so I will not pick this up. Cheers, Bastian
On 1/17/23 23:03, Bastian Koppelmann wrote: > 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? I'm sure env->hflags was copied from i386 or mips. It would be used to cache cpu state that is not easy to recompute. There doesn't seem to be any of that in TriCore. > I do wonder why there is the extra step via the flags of a TranslationBlock. Why > can't we sync directly using CPUState? We need the data in the TranslationBlock in order for the data to be hashed and compared, so that we do not attempt to run the same code with the wrong privilege. > I first thought to differentiated the > TranslationBlocks for the different privilege level. However isn't that done > using the softmmu_idx? No, it's not. The only thing that's used to select TranslationBlocks is the data retrieved by cpu_get_tb_cpu_state. > 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. Ok. r~
diff --git a/target/tricore/cpu.h b/target/tricore/cpu.h index 3b9c533a7c..47d0ffb745 100644 --- a/target/tricore/cpu.h +++ b/target/tricore/cpu.h @@ -25,10 +25,6 @@ #include "qemu/cpu-float.h" #include "tricore-defs.h" -struct tricore_boot_info; - -typedef struct tricore_def_t tricore_def_t; - typedef struct CPUArchState { /* GPR Register */ uint32_t gpr_a[16]; @@ -179,16 +175,9 @@ typedef struct CPUArchState { uint32_t M3CNT; /* Floating Point Registers */ float_status fp_status; - /* QEMU */ - int error_code; - uint32_t hflags; /* CPU State */ /* Internal CPU feature flags. */ uint64_t features; - - const tricore_def_t *cpu_model; - void *irq[8]; - struct QEMUTimer *timer; /* Internal timer */ } CPUTriCoreState; /**
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> --- target/tricore/cpu.h | 11 ----------- 1 file changed, 11 deletions(-)