Re: [PATCH 03/13] target/hppa: Fix PSW_W and PSW_E bits in rsm, ssm and mtsm

[Richard Henderson <richard.henderson@linaro.org>]

On 2/7/24 08:20, deller@kernel.org wrote:
>   #define PSW_E            0x04000000
> +#define PSW_E_BIT                37 /* PA2.0 only */
>   #define PSW_W            0x08000000 /* PA2.0 only */
> +#define PSW_W_BIT                36 /* PA2.0 only */
...
> +target_ulong HELPER(get_system_mask)(CPUHPPAState *env)
> +{
> +    target_ulong psw = env->psw;
> +
> +    /* mask out invalid bits */
> +    target_ulong psw_new = psw & PSW_SM;
> +
> +    /* ssm/rsm instructions number PSW_W and PSW_E differently */
> +    psw_new &= ~PSW_W;
> +    if (psw & PSW_W) {
> +        psw_new |= 1ull << (63 - PSW_W_BIT);
> +    }

Um, this has changed nothing, since 1 << (63 - 36) == 0x8000000 == PSW_W.

The conversion of PSW_SM_W to PSW_W happens in expand_sm_imm().
There's a comment there about keeping unimplemented bits disabled, including PSW_E. 
Perhaps this is the wrong layer in which to do this?

In any case, what is the actual problem that you're seeing?  Because it *isn't* that we 
were not considering the different placement of the bits, as your commit message suggests.

> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index 53ec57ee86..10fdc0813d 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -2163,13 +2163,20 @@ static bool trans_rsm(DisasContext *ctx, arg_rsm *a)
>       nullify_over(ctx);
>   
>       tmp = tcg_temp_new_i64();
> -    tcg_gen_ld_i64(tmp, tcg_env, offsetof(CPUHPPAState, psw));
> -    tcg_gen_andi_i64(tmp, tmp, ~a->i);
> -    gen_helper_swap_system_mask(tmp, tcg_env, tmp);
> -    save_gpr(ctx, a->t, tmp);
> +    if (a->t != 0) {
> +        gen_helper_get_system_mask(tmp, tcg_env);
> +        save_gpr(ctx, a->t, tmp);
> +    }

If a->t == 0, tmp is uninitialized...

> +
> +    if (a->i) {
> +        tcg_gen_ld_i64(tmp, tcg_env, offsetof(CPUHPPAState, psw));

... but read here.

> @@ -2183,11 +2190,17 @@ static bool trans_ssm(DisasContext *ctx, arg_ssm *a)
>       nullify_over(ctx);
>   
>       tmp = tcg_temp_new_i64();
> -    tcg_gen_ld_i64(tmp, tcg_env, offsetof(CPUHPPAState, psw));
> -    tcg_gen_ori_i64(tmp, tmp, a->i);
> -    gen_helper_swap_system_mask(tmp, tcg_env, tmp);
> -    save_gpr(ctx, a->t, tmp);
> +    if (a->t != 0) {
> +        gen_helper_get_system_mask(tmp, tcg_env);
> +        save_gpr(ctx, a->t, tmp);
> +    }
> +
> +    if (a->i) {
> +        tcg_gen_ld_i64(tmp, tcg_env, offsetof(CPUHPPAState, psw));
> +        tcg_gen_ori_i64(tmp, tmp, a->i);

Likewise.


r~