Re: [PATCH 4/6] target/tricore: Refactor PCXI/ICR register fields - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <philmd@linaro.org>
To: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>,
	qemu-devel@nongnu.org
Subject: Re: [PATCH 4/6] target/tricore: Refactor PCXI/ICR register fields
Date: Fri, 19 May 2023 17:02:48 +0200	[thread overview]
Message-ID: <906dbffa-c677-1192-fedc-5664e61f8255@linaro.org> (raw)
In-Reply-To: <20230519133650.575600-5-kbastian@mail.uni-paderborn.de>

Hi Bastian,

On 19/5/23 15:36, Bastian Koppelmann wrote:
> starting from ISA version 1.6.1 (previously known as 1.6P/E), some
> bitfields in PCXI and ICR have changed. We also refactor these
> registers using the register fields API.
> 
> Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1453
> ---
>   target/tricore/cpu.h       | 39 ++++++++++++-----
>   target/tricore/helper.c    | 45 ++++++++++++++++++++
>   target/tricore/op_helper.c | 85 +++++++++++++++++++-------------------
>   target/tricore/translate.c | 12 +++++-
>   4 files changed, 125 insertions(+), 56 deletions(-)

> diff --git a/target/tricore/translate.c b/target/tricore/translate.c
> index 2646cb3eb5..db62604102 100644
> --- a/target/tricore/translate.c
> +++ b/target/tricore/translate.c
> @@ -7847,12 +7847,20 @@ static void decode_sys_interrupts(DisasContext *ctx)
>           /* raise EXCP_DEBUG */
>           break;
>       case OPC2_32_SYS_DISABLE:
> -        tcg_gen_andi_tl(cpu_ICR, cpu_ICR, ~MASK_ICR_IE_1_3);
> +        if (has_feature(ctx, TRICORE_FEATURE_161)) {
> +            tcg_gen_andi_tl(cpu_ICR, cpu_ICR, ~R_ICR_IE_161_MASK);
> +        } else {
> +            tcg_gen_andi_tl(cpu_ICR, cpu_ICR, ~R_ICR_IE_13_MASK);
> +        }
>           break;
>       case OPC2_32_SYS_DSYNC:
>           break;
>       case OPC2_32_SYS_ENABLE:
> -        tcg_gen_ori_tl(cpu_ICR, cpu_ICR, MASK_ICR_IE_1_3);
> +        if (has_feature(ctx, TRICORE_FEATURE_161)) {
> +            tcg_gen_ori_tl(cpu_ICR, cpu_ICR, R_ICR_IE_161_MASK);
> +        } else {
> +            tcg_gen_ori_tl(cpu_ICR, cpu_ICR, R_ICR_IE_13_MASK);
> +        }
>           break;
>       case OPC2_32_SYS_ISYNC:
>           break;

Could it be clearer to add a 'icr_ie_mask' field in CPUTriCoreState,
initialized once in tricore_cpu_realizefn() and avoid this if/else/ in
translation here, simply using the initialized mask?