From: Helge Deller <deller@gmx.de>
The ssm and rsm instructions number the PSW_W and PSW_E bits differently
than how they are actually in the PSW. Both bits are relevant on 64-bit
CPUs only.
Fix the existing ssm and rsm instructions to handle the bits correctly.
For that drop the swap_system_mask() helper function in favour of new
helper functions get_system_mask, set_system_mask and mtsm_system_mask.
get_system_mask() returns the PSW bits how they should be returned in
the target register of the ssm and rsm instructions.
set_system_mask() sets the PSW bits without any further modification.
Note that the a->i constant value of ssm and rsm have already been
converted to match the physical PSW bits by expand_sm_imm() in the
instruction decoder.
The mtsm instruction is different, as it takes the new PSW from a
register at runtime, and as such the PSW.E and PSW.W bits are moved at
runtime to the right bit positions before setting the PSW.
Signed-off-by: Helge Deller <deller@gmx.de>
---
target/hppa/cpu.h | 2 ++
target/hppa/helper.h | 4 +++-
target/hppa/sys_helper.c | 46 +++++++++++++++++++++++++++++++++++++---
target/hppa/translate.c | 38 +++++++++++++++++++++------------
4 files changed, 73 insertions(+), 17 deletions(-)
diff --git a/target/hppa/cpu.h b/target/hppa/cpu.h
index 7a181e8f33..06b65f2258 100644
--- a/target/hppa/cpu.h
+++ b/target/hppa/cpu.h
@@ -122,7 +122,9 @@
#define PSW_T 0x01000000
#define PSW_S 0x02000000
#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 */
#define PSW_Z 0x40000000 /* PA1.x only */
#define PSW_Y 0x80000000 /* PA1.x only */
diff --git a/target/hppa/helper.h b/target/hppa/helper.h
index 1bdbcd8f98..abffd3f531 100644
--- a/target/hppa/helper.h
+++ b/target/hppa/helper.h
@@ -92,7 +92,9 @@ DEF_HELPER_1(rfi_r, void, env)
DEF_HELPER_FLAGS_2(write_interval_timer, TCG_CALL_NO_RWG, void, env, tl)
DEF_HELPER_FLAGS_2(write_eirr, TCG_CALL_NO_RWG, void, env, tl)
DEF_HELPER_FLAGS_2(write_eiem, TCG_CALL_NO_RWG, void, env, tl)
-DEF_HELPER_FLAGS_2(swap_system_mask, TCG_CALL_NO_RWG, tl, env, tl)
+DEF_HELPER_FLAGS_1(get_system_mask, TCG_CALL_NO_RWG, tl, env)
+DEF_HELPER_FLAGS_2(set_system_mask, TCG_CALL_NO_RWG, void, env, tl)
+DEF_HELPER_FLAGS_2(mtsm_system_mask, TCG_CALL_NO_RWG, void, env, tl)
DEF_HELPER_FLAGS_3(itlba_pa11, TCG_CALL_NO_RWG, void, env, tl, tl)
DEF_HELPER_FLAGS_3(itlbp_pa11, TCG_CALL_NO_RWG, void, env, tl, tl)
DEF_HELPER_FLAGS_3(idtlbt_pa20, TCG_CALL_NO_RWG, void, env, tl, tl)
diff --git a/target/hppa/sys_helper.c b/target/hppa/sys_helper.c
index a59245eed3..88ba99f0d4 100644
--- a/target/hppa/sys_helper.c
+++ b/target/hppa/sys_helper.c
@@ -58,7 +58,27 @@ void HELPER(reset)(CPUHPPAState *env)
helper_excp(env, EXCP_HLT);
}
-target_ulong HELPER(swap_system_mask)(CPUHPPAState *env, target_ulong nsm)
+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);
+ }
+ psw_new &= ~PSW_E;
+ if (psw & PSW_E) {
+ psw_new |= 1ull << (63 - PSW_E_BIT);
+ }
+
+ return psw_new;
+}
+
+void HELPER(set_system_mask)(CPUHPPAState *env, target_ulong nsm)
{
target_ulong psw = env->psw;
/*
@@ -70,8 +90,28 @@ target_ulong HELPER(swap_system_mask)(CPUHPPAState *env, target_ulong nsm)
* machines set the Q bit from 0 to 1 without an exception,
* so let this go without comment.
*/
- env->psw = (psw & ~PSW_SM) | (nsm & PSW_SM);
- return psw & PSW_SM;
+
+ cpu_hppa_put_psw(env, (psw & ~PSW_SM) | (nsm & PSW_SM));
+}
+
+void HELPER(mtsm_system_mask)(CPUHPPAState *env, target_ulong nsm)
+{
+ target_ulong psw_new;
+
+ /* mask out invalid bits */
+ psw_new = nsm & PSW_SM;
+
+ /* set PSW_E and PSW_W */
+ psw_new &= ~PSW_W;
+ if (nsm & (1ull << (63 - PSW_W_BIT))) {
+ psw_new |= PSW_W;
+ }
+ psw_new &= ~PSW_E;
+ if (nsm & (1ull << (63 - PSW_E_BIT))) {
+ psw_new |= PSW_E;
+ }
+
+ helper_set_system_mask(env, psw_new);
}
void HELPER(rfi)(CPUHPPAState *env)
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->i) {
+ tcg_gen_ld_i64(tmp, tcg_env, offsetof(CPUHPPAState, psw));
+ tcg_gen_andi_i64(tmp, tmp, ~a->i);
+ gen_helper_set_system_mask(tcg_env, tmp);
+
+ /* Exit, check e.g. for new interrupts */
+ ctx->base.is_jmp = DISAS_IAQ_N_STALE_EXIT;
+ }
- /* Exit the TB to recognize new interrupts, e.g. PSW_M. */
- ctx->base.is_jmp = DISAS_IAQ_N_STALE_EXIT;
return nullify_end(ctx);
#endif
}
@@ -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);
+ gen_helper_set_system_mask(tcg_env, tmp);
+ }
/* Exit the TB to recognize new interrupts, e.g. PSW_I. */
ctx->base.is_jmp = DISAS_IAQ_N_STALE_EXIT;
return nullify_end(ctx);
@@ -2198,12 +2211,11 @@ static bool trans_mtsm(DisasContext *ctx, arg_mtsm *a)
{
CHECK_MOST_PRIVILEGED(EXCP_PRIV_OPR);
#ifndef CONFIG_USER_ONLY
- TCGv_i64 tmp, reg;
+ TCGv_i64 reg;
nullify_over(ctx);
reg = load_gpr(ctx, a->r);
- tmp = tcg_temp_new_i64();
- gen_helper_swap_system_mask(tmp, tcg_env, reg);
+ gen_helper_mtsm_system_mask(tcg_env, reg);
/* Exit the TB to recognize new interrupts. */
ctx->base.is_jmp = DISAS_IAQ_N_STALE_EXIT;
--
2.43.0On 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~
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~
© 2016 - 2024 Red Hat, Inc.