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

[Helge Deller <deller@gmx.de>]

On 2/8/24 21:43, Richard Henderson wrote:
> 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.

Yep.
I seem to have mixed strange things when writing that.
I've dropped the patch for now.

Thanks!
Helge


> 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~