[PATCH 0/7] few fixes for hppa target

[PATCH 0/7] few fixes for hppa target

[Sven Schnelle]

Hi,

here are a few fixes for the hppa target i made while debugging
some wide mode issues.

Sven Schnelle (7):
  target/hppa: ldcw,s uses static shift of 3
  target/hppa: fix shrp for wide mode
  target/hppa: fix access_id check
  target/hppa: exit tb on flush cache instructions
  target/hppa: copy new_spc to iasq_f on be,n instruction
  target/hppa: mask privilege bits in mfia
  target/hppa: fix do_stdby_e()

 roms/SLOF                |  2 +-
 target/hppa/mem_helper.c | 67 +++++++++++++++++++++++++++++++++++-----
 target/hppa/op_helper.c  | 10 +++---
 target/hppa/translate.c  | 13 +++++---
 4 files changed, 74 insertions(+), 18 deletions(-)

-- 
2.43.2

[PATCH 1/7] target/hppa: ldcw,s uses static shift of 3

[Sven Schnelle]

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 target/hppa/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index eb2046c5ad..6a513d7d5c 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -3085,7 +3085,7 @@ static bool trans_ldc(DisasContext *ctx, arg_ldst *a)
         dest = dest_gpr(ctx, a->t);
     }
 
-    form_gva(ctx, &addr, &ofs, a->b, a->x, a->scale ? a->size : 0,
+    form_gva(ctx, &addr, &ofs, a->b, a->x, a->scale ? 3 : 0,
              a->disp, a->sp, a->m, MMU_DISABLED(ctx));
 
     /*
-- 
2.43.2

[PATCH 2/7] target/hppa: fix shrp for wide mode

[Sven Schnelle]

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 target/hppa/translate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index 6a513d7d5c..8ba31567e8 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -3462,7 +3462,7 @@ static bool trans_shrp_sar(DisasContext *ctx, arg_shrp_sar *a)
     /* Install the new nullification.  */
     cond_free(&ctx->null_cond);
     if (a->c) {
-        ctx->null_cond = do_sed_cond(ctx, a->c, false, dest);
+        ctx->null_cond = do_sed_cond(ctx, a->c, a->d, dest);
     }
     return nullify_end(ctx);
 }
@@ -3505,7 +3505,7 @@ static bool trans_shrp_imm(DisasContext *ctx, arg_shrp_imm *a)
     /* Install the new nullification.  */
     cond_free(&ctx->null_cond);
     if (a->c) {
-        ctx->null_cond = do_sed_cond(ctx, a->c, false, dest);
+        ctx->null_cond = do_sed_cond(ctx, a->c, a->d, dest);
     }
     return nullify_end(ctx);
 }
-- 
2.43.2

[PATCH 3/7] target/hppa: fix access_id check

[Sven Schnelle]

PA2.0 provides 8 instead of 4 PID registers.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 roms/SLOF                |  2 +-
 target/hppa/mem_helper.c | 67 +++++++++++++++++++++++++++++++++++-----
 2 files changed, 60 insertions(+), 9 deletions(-)

diff --git a/roms/SLOF b/roms/SLOF
index 3a259df244..6b6c16b4b4 160000
--- a/roms/SLOF
+++ b/roms/SLOF
@@ -1 +1 @@
-Subproject commit 3a259df2449fc4a4e43ab5f33f0b2c66484b4bc3
+Subproject commit 6b6c16b4b40763507cf1f518096f3c3883c5cf2d
diff --git a/target/hppa/mem_helper.c b/target/hppa/mem_helper.c
index 80f51e753f..e4e3f6cdbe 100644
--- a/target/hppa/mem_helper.c
+++ b/target/hppa/mem_helper.c
@@ -152,6 +152,59 @@ static HPPATLBEntry *hppa_alloc_tlb_ent(CPUHPPAState *env)
     return ent;
 }
 
+static uint32_t get_pid(CPUHPPAState *env, int num)
+{
+    const struct pid_map {
+        int reg;
+        bool shift;
+    } *pid;
+
+    const struct pid_map pids64[] = {
+        { .reg = 8,  .shift = true  },
+        { .reg = 8,  .shift = false },
+        { .reg = 9,  .shift = true  },
+        { .reg = 9,  .shift = false },
+        { .reg = 12, .shift = true  },
+        { .reg = 12, .shift = false },
+        { .reg = 13, .shift = true  },
+        { .reg = 13, .shift = false }
+    };
+
+    const struct pid_map pids32[] = {
+        { .reg = 8,  .shift = false  },
+        { .reg = 9,  .shift = false  },
+        { .reg = 12, .shift = false  },
+        { .reg = 13, .shift = false  },
+    };
+
+    if (hppa_is_pa20(env)) {
+        pid = pids64 + num;
+    } else {
+        pid = pids32 + num;
+    }
+    uint64_t cr = env->cr[pid->reg];
+    if (pid->shift) {
+        cr >>= 32;
+    } else {
+        cr &= 0xffffffff;
+    }
+    return cr;
+}
+
+#define ACCESS_ID_MASK 0xffff
+
+static bool match_prot_id(CPUHPPAState *env, uint32_t access_id, uint32_t *_pid)
+{
+    for (int i = 0; i < 8; i++) {
+        uint32_t pid = get_pid(env, i);
+        if ((access_id & ACCESS_ID_MASK) == ((pid >> 1) & ACCESS_ID_MASK)) {
+            *_pid = pid;
+            return true;
+        }
+    }
+    return false;
+}
+
 int hppa_get_physical_address(CPUHPPAState *env, vaddr addr, int mmu_idx,
                               int type, hwaddr *pphys, int *pprot,
                               HPPATLBEntry **tlb_entry)
@@ -227,15 +280,13 @@ int hppa_get_physical_address(CPUHPPAState *env, vaddr addr, int mmu_idx,
     /* access_id == 0 means public page and no check is performed */
     if (ent->access_id && MMU_IDX_TO_P(mmu_idx)) {
         /* If bits [31:1] match, and bit 0 is set, suppress write.  */
-        int match = ent->access_id * 2 + 1;
-
-        if (match == env->cr[CR_PID1] || match == env->cr[CR_PID2] ||
-            match == env->cr[CR_PID3] || match == env->cr[CR_PID4]) {
-            prot &= PAGE_READ | PAGE_EXEC;
-            if (type == PAGE_WRITE) {
-                ret = EXCP_DMPI;
-                goto egress;
+        uint32_t pid;
+        if (match_prot_id(env, ent->access_id, &pid)) {
+            if ((pid & 1) && (prot & PROT_WRITE)) {
+                prot &= ~PROT_WRITE;
             }
+        } else {
+            prot = 0;
         }
     }
 
-- 
2.43.2

[PATCH 4/7] target/hppa: exit tb on flush cache instructions

[Sven Schnelle]

When the guest modifies the tb it is currently executing from,
it executes a fic instruction. Exit the tb on such instruction,
otherwise we might execute stale code.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 target/hppa/translate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index 8ba31567e8..58d7ec1ade 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -1940,6 +1940,7 @@ static void do_page_zero(DisasContext *ctx)
 static bool trans_nop(DisasContext *ctx, arg_nop *a)
 {
     cond_free(&ctx->null_cond);
+    ctx->base.is_jmp = DISAS_IAQ_N_STALE;
     return true;
 }
 
@@ -2290,6 +2291,7 @@ static bool trans_nop_addrx(DisasContext *ctx, arg_ldst *a)
         save_gpr(ctx, a->b, dest);
     }
     cond_free(&ctx->null_cond);
+    ctx->base.is_jmp = DISAS_IAQ_N_STALE;
     return true;
 }
 
-- 
2.43.2

[PATCH 5/7] target/hppa: copy new_spc to iasq_f on be,n instruction

[Sven Schnelle]

Otherwise the first instruction at the new location gets executed from
the old space.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 target/hppa/translate.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index 58d7ec1ade..a09112e4ae 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -3777,6 +3777,9 @@ static bool trans_be(DisasContext *ctx, arg_be *a)
         }
         copy_iaoq_entry(ctx, cpu_iaoq_b, -1, tmp);
         tcg_gen_mov_i64(cpu_iasq_b, new_spc);
+        if (a->n) {
+            tcg_gen_mov_i64(cpu_iasq_f, new_spc);
+        }
         nullify_set(ctx, a->n);
     }
     tcg_gen_lookup_and_goto_ptr();
-- 
2.43.2

[PATCH 6/7] target/hppa: mask privilege bits in mfia

[Sven Schnelle]

mfia should return only the iaoq bits without privilege
bits.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 target/hppa/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index a09112e4ae..e47f8f9f47 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -1962,7 +1962,7 @@ static bool trans_mfia(DisasContext *ctx, arg_mfia *a)
 {
     unsigned rt = a->t;
     TCGv_i64 tmp = dest_gpr(ctx, rt);
-    tcg_gen_movi_i64(tmp, ctx->iaoq_f);
+    tcg_gen_movi_i64(tmp, ctx->iaoq_f & ~3ULL);
     save_gpr(ctx, rt, tmp);
 
     cond_free(&ctx->null_cond);
-- 
2.43.2

[PATCH 7/7] target/hppa: fix do_stdby_e()

[Sven Schnelle]

stdby,e,m was writing data from the wrong half of the register
into memory for cases 0-3.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 target/hppa/op_helper.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/target/hppa/op_helper.c b/target/hppa/op_helper.c
index 480fe80844..6cf49f33b7 100644
--- a/target/hppa/op_helper.c
+++ b/target/hppa/op_helper.c
@@ -281,17 +281,17 @@ static void do_stdby_e(CPUHPPAState *env, target_ulong addr, uint64_t val,
     case 3:
         /* The 3 byte store must appear atomic.  */
         if (parallel) {
-            atomic_store_mask32(env, addr - 3, val, 0xffffff00u, ra);
+            atomic_store_mask32(env, addr - 3, val >> 32, 0xffffff00u, ra);
         } else {
-            cpu_stw_data_ra(env, addr - 3, val >> 16, ra);
-            cpu_stb_data_ra(env, addr - 1, val >> 8, ra);
+            cpu_stw_data_ra(env, addr - 3, val >> 48, ra);
+            cpu_stb_data_ra(env, addr - 1, val >> 40, ra);
         }
         break;
     case 2:
-        cpu_stw_data_ra(env, addr - 2, val >> 16, ra);
+        cpu_stw_data_ra(env, addr - 2, val >> 48, ra);
         break;
     case 1:
-        cpu_stb_data_ra(env, addr - 1, val >> 24, ra);
+        cpu_stb_data_ra(env, addr - 1, val >> 56, ra);
         break;
     default:
         /* Nothing is stored, but protection is checked and the
-- 
2.43.2

Re: [PATCH 2/7] target/hppa: fix shrp for wide mode

[Helge Deller]

On 3/17/24 23:14, Sven Schnelle wrote:
> Signed-off-by: Sven Schnelle <svens@stackframe.org>

Reviewed-by: Helge Deller <deller@gmx.de>

Helge

> ---
>   target/hppa/translate.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index 6a513d7d5c..8ba31567e8 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -3462,7 +3462,7 @@ static bool trans_shrp_sar(DisasContext *ctx, arg_shrp_sar *a)
>       /* Install the new nullification.  */
>       cond_free(&ctx->null_cond);
>       if (a->c) {
> -        ctx->null_cond = do_sed_cond(ctx, a->c, false, dest);
> +        ctx->null_cond = do_sed_cond(ctx, a->c, a->d, dest);
>       }
>       return nullify_end(ctx);
>   }
> @@ -3505,7 +3505,7 @@ static bool trans_shrp_imm(DisasContext *ctx, arg_shrp_imm *a)
>       /* Install the new nullification.  */
>       cond_free(&ctx->null_cond);
>       if (a->c) {
> -        ctx->null_cond = do_sed_cond(ctx, a->c, false, dest);
> +        ctx->null_cond = do_sed_cond(ctx, a->c, a->d, dest);
>       }
>       return nullify_end(ctx);
>   }

Re: [PATCH 1/7] target/hppa: ldcw,s uses static shift of 3

[Richard Henderson]

On 3/17/24 12:14, Sven Schnelle wrote:
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
>   target/hppa/translate.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index eb2046c5ad..6a513d7d5c 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -3085,7 +3085,7 @@ static bool trans_ldc(DisasContext *ctx, arg_ldst *a)
>           dest = dest_gpr(ctx, a->t);
>       }
>   
> -    form_gva(ctx, &addr, &ofs, a->b, a->x, a->scale ? a->size : 0,
> +    form_gva(ctx, &addr, &ofs, a->b, a->x, a->scale ? 3 : 0,
>                a->disp, a->sp, a->m, MMU_DISABLED(ctx));
>   
>       /*

Whoops, broken since day one.

Fixes: 96d6407f363 ("target-hppa: Implement loads and stores")
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 2/7] target/hppa: fix shrp for wide mode

[Richard Henderson]

On 3/17/24 12:14, Sven Schnelle wrote:
> Signed-off-by: Sven Schnelle<svens@stackframe.org>
> ---
>   target/hppa/translate.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Fixes: f7b775a9c075 ("target/hppa: Implement SHRPD")
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 3/7] target/hppa: fix access_id check

[Helge Deller]

On 3/17/24 23:14, Sven Schnelle wrote:
> PA2.0 provides 8 instead of 4 PID registers.
>
> Signed-off-by: Sven Schnelle <svens@stackframe.org>

Reviewed-by: Helge Deller <deller@gmx.de>
with a few comments below...

Helge

> ---
>   roms/SLOF                |  2 +-
>   target/hppa/mem_helper.c | 67 +++++++++++++++++++++++++++++++++++-----
>   2 files changed, 60 insertions(+), 9 deletions(-)
>
> diff --git a/roms/SLOF b/roms/SLOF
> index 3a259df244..6b6c16b4b4 160000
> --- a/roms/SLOF
> +++ b/roms/SLOF
> @@ -1 +1 @@
> -Subproject commit 3a259df2449fc4a4e43ab5f33f0b2c66484b4bc3
> +Subproject commit 6b6c16b4b40763507cf1f518096f3c3883c5cf2d

this doesn't belong here.


> diff --git a/target/hppa/mem_helper.c b/target/hppa/mem_helper.c
> index 80f51e753f..e4e3f6cdbe 100644
> --- a/target/hppa/mem_helper.c
> +++ b/target/hppa/mem_helper.c
> @@ -152,6 +152,59 @@ static HPPATLBEntry *hppa_alloc_tlb_ent(CPUHPPAState *env)
>       return ent;
>   }
>
> +static uint32_t get_pid(CPUHPPAState *env, int num)
> +{
> +    const struct pid_map {
> +        int reg;
> +        bool shift;

does it makes sense to condense it, e.g.:
  +        unsigned char reg:7,
  +        unsigned char shift:1;

Helge


> +    } *pid;
> +
> +    const struct pid_map pids64[] = {
> +        { .reg = 8,  .shift = true  },
> +        { .reg = 8,  .shift = false },
> +        { .reg = 9,  .shift = true  },
> +        { .reg = 9,  .shift = false },
> +        { .reg = 12, .shift = true  },
> +        { .reg = 12, .shift = false },
> +        { .reg = 13, .shift = true  },
> +        { .reg = 13, .shift = false }
> +    };
> +
> +    const struct pid_map pids32[] = {
> +        { .reg = 8,  .shift = false  },
> +        { .reg = 9,  .shift = false  },
> +        { .reg = 12, .shift = false  },
> +        { .reg = 13, .shift = false  },
> +    };
> +
> +    if (hppa_is_pa20(env)) {
> +        pid = pids64 + num;
> +    } else {
> +        pid = pids32 + num;
> +    }
> +    uint64_t cr = env->cr[pid->reg];
> +    if (pid->shift) {
> +        cr >>= 32;
> +    } else {
> +        cr &= 0xffffffff;
> +    }
> +    return cr;
> +}
> +
> +#define ACCESS_ID_MASK 0xffff
> +
> +static bool match_prot_id(CPUHPPAState *env, uint32_t access_id, uint32_t *_pid)
> +{
> +    for (int i = 0; i < 8; i++) {
> +        uint32_t pid = get_pid(env, i);
> +        if ((access_id & ACCESS_ID_MASK) == ((pid >> 1) & ACCESS_ID_MASK)) {
> +            *_pid = pid;
> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
>   int hppa_get_physical_address(CPUHPPAState *env, vaddr addr, int mmu_idx,
>                                 int type, hwaddr *pphys, int *pprot,
>                                 HPPATLBEntry **tlb_entry)
> @@ -227,15 +280,13 @@ int hppa_get_physical_address(CPUHPPAState *env, vaddr addr, int mmu_idx,
>       /* access_id == 0 means public page and no check is performed */
>       if (ent->access_id && MMU_IDX_TO_P(mmu_idx)) {
>           /* If bits [31:1] match, and bit 0 is set, suppress write.  */
> -        int match = ent->access_id * 2 + 1;
> -
> -        if (match == env->cr[CR_PID1] || match == env->cr[CR_PID2] ||
> -            match == env->cr[CR_PID3] || match == env->cr[CR_PID4]) {
> -            prot &= PAGE_READ | PAGE_EXEC;
> -            if (type == PAGE_WRITE) {
> -                ret = EXCP_DMPI;
> -                goto egress;
> +        uint32_t pid;
> +        if (match_prot_id(env, ent->access_id, &pid)) {
> +            if ((pid & 1) && (prot & PROT_WRITE)) {
> +                prot &= ~PROT_WRITE;
>               }
> +        } else {
> +            prot = 0;
>           }
>       }
>

Re: [PATCH 4/7] target/hppa: exit tb on flush cache instructions

[Helge Deller]

On 3/17/24 23:14, Sven Schnelle wrote:
> When the guest modifies the tb it is currently executing from,
> it executes a fic instruction. Exit the tb on such instruction,
> otherwise we might execute stale code.
>
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
>   target/hppa/translate.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index 8ba31567e8..58d7ec1ade 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -1940,6 +1940,7 @@ static void do_page_zero(DisasContext *ctx)
>   static bool trans_nop(DisasContext *ctx, arg_nop *a)
>   {
>       cond_free(&ctx->null_cond);
> +    ctx->base.is_jmp = DISAS_IAQ_N_STALE;
>       return true;
>   }
>
> @@ -2290,6 +2291,7 @@ static bool trans_nop_addrx(DisasContext *ctx, arg_ldst *a)
>           save_gpr(ctx, a->b, dest);
>       }
>       cond_free(&ctx->null_cond);
> +    ctx->base.is_jmp = DISAS_IAQ_N_STALE;

I wonder if it makes sense to rename trans_nop() and trans_nop_addrx()
to something like trans_cache_flush() and trans_cache_flush_addrx() ?

Other than that:
Reviewed-by: Helge Deller <deller@gmx.de>

Helge

Re: [PATCH 6/7] target/hppa: mask privilege bits in mfia

[Helge Deller]

On 3/17/24 23:14, Sven Schnelle wrote:
> mfia should return only the iaoq bits without privilege
> bits.
>
> Signed-off-by: Sven Schnelle <svens@stackframe.org>

Reviewed-by: Helge Deller <deller@gmx.de>

Helge


> ---
>   target/hppa/translate.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index a09112e4ae..e47f8f9f47 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -1962,7 +1962,7 @@ static bool trans_mfia(DisasContext *ctx, arg_mfia *a)
>   {
>       unsigned rt = a->t;
>       TCGv_i64 tmp = dest_gpr(ctx, rt);
> -    tcg_gen_movi_i64(tmp, ctx->iaoq_f);
> +    tcg_gen_movi_i64(tmp, ctx->iaoq_f & ~3ULL);
>       save_gpr(ctx, rt, tmp);
>
>       cond_free(&ctx->null_cond);

Re: [PATCH 3/7] target/hppa: fix access_id check

[Richard Henderson]

On 3/17/24 12:14, Sven Schnelle wrote:
> +static bool match_prot_id(CPUHPPAState *env, uint32_t access_id, uint32_t *_pid)
> +{
> +    for (int i = 0; i < 8; i++) {
> +        uint32_t pid = get_pid(env, i);

There are only 4 pid's for pa1.x.

> +static uint32_t get_pid(CPUHPPAState *env, int num)
> +{
> +    const struct pid_map {
> +        int reg;
> +        bool shift;
> +    } *pid;
> +
> +    const struct pid_map pids64[] = {
> +        { .reg = 8,  .shift = true  },
> +        { .reg = 8,  .shift = false },
> +        { .reg = 9,  .shift = true  },
> +        { .reg = 9,  .shift = false },
> +        { .reg = 12, .shift = true  },
> +        { .reg = 12, .shift = false },
> +        { .reg = 13, .shift = true  },
> +        { .reg = 13, .shift = false }
> +    };
> +
> +    const struct pid_map pids32[] = {
> +        { .reg = 8,  .shift = false  },
> +        { .reg = 9,  .shift = false  },
> +        { .reg = 12, .shift = false  },
> +        { .reg = 13, .shift = false  },
> +    };
> +
> +    if (hppa_is_pa20(env)) {

This predicate is fairly expensive -- you don't want to put it deep inside a loop.
The table is very predictable.  Moreover, you don't need to test these in any particular 
order.

>          /* If bits [31:1] match, and bit 0 is set, suppress write.  */
> -        int match = ent->access_id * 2 + 1;
> -
> -        if (match == env->cr[CR_PID1] || match == env->cr[CR_PID2] ||
> -            match == env->cr[CR_PID3] || match == env->cr[CR_PID4]) {
> -            prot &= PAGE_READ | PAGE_EXEC;
> -            if (type == PAGE_WRITE) {
> -                ret = EXCP_DMPI;
> -                goto egress;
> +        uint32_t pid;
> +        if (match_prot_id(env, ent->access_id, &pid)) {
> +            if ((pid & 1) && (prot & PROT_WRITE)) {
> +                prot &= ~PROT_WRITE;
>              }
> +        } else {
> +            prot = 0;
>          }

You're losing the data memory protection id trap.

Therefore I suggest

/* Return the set of protections allowed by a PID match. */
static int match_prot_id_1(uint32_t access_id, uint32_t prot_id)
{
     if (((access_id ^ (prot_id >> 1) & ACCESS_ID_MASK) == 0) {
         return (prot_id & 1
                 ? PROT_EXEC | PROT_READ
                 : PROT_EXEC | PROT_READ | PROT_WRITE);
     }
     return 0;
}

static int match_prot_id32(CPUHPPAState *env, uint32_t access_id)
{
     int r, i;
     for (i = CR_PID1; i <= CR_PID4; ++i) {
         r = match_prot_id_1(access_id, env->cr[i]);
         if (r) {
             return r;
         }
     }
     return 0;
}

static int match_prot_id64(CPUHPPAState *env, uint32_t access_id)
{
     int r, i;
     for (i = CR_PID1; i <= CR_PID4; ++i) {
         r = match_prot_id_1(access_id, env->cr[i]);
         if (r) {
             return r;
         }
         r = match_prot_id_1(access_id, env->cr[i] >> 32);
         if (r) {
             return r;
         }
     }
     return 0;
}

---

     if (ent->access_id && MMU_IDX_TO_P(mmu_idx)) {
         int access_prot = (hppa_is_pa20(env)
                            ? match_prot_id64(env, ent->access_id)
                            : match_prot_id32(env, ent->access_id));
         if (prot & ~access_prot) {
             ret = EXCP_DMPI;
             goto egress;
         }
     }

At this point there are now a couple of hppa_is_pa20() calls within 
hppa_get_physical_address, which could be unified to a single local bool.


r~

Re: [PATCH 4/7] target/hppa: exit tb on flush cache instructions

[Richard Henderson]

On 3/17/24 12:14, Sven Schnelle wrote:
> When the guest modifies the tb it is currently executing from,
> it executes a fic instruction. Exit the tb on such instruction,
> otherwise we might execute stale code.
> 
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
>   target/hppa/translate.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index 8ba31567e8..58d7ec1ade 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -1940,6 +1940,7 @@ static void do_page_zero(DisasContext *ctx)
>   static bool trans_nop(DisasContext *ctx, arg_nop *a)
>   {
>       cond_free(&ctx->null_cond);
> +    ctx->base.is_jmp = DISAS_IAQ_N_STALE;
>       return true;
>   }
>   
> @@ -2290,6 +2291,7 @@ static bool trans_nop_addrx(DisasContext *ctx, arg_ldst *a)
>           save_gpr(ctx, a->b, dest);
>       }
>       cond_free(&ctx->null_cond);
> +    ctx->base.is_jmp = DISAS_IAQ_N_STALE;
>       return true;
>   }
>   

You should create new functions for fic,

static bool trans_fic(DisasContext *ctx, arg_nop *a)
{
     ctx->base.is_jmp = DISAS_IAQ_N_STALE;
     return trans_nop(ctx, a);
}

because fid and pdc also use trans_nop/trans_nop_addrx.


r~

Re: [PATCH 5/7] target/hppa: copy new_spc to iasq_f on be,n instruction

[Richard Henderson]

On 3/17/24 12:14, Sven Schnelle wrote:
> Otherwise the first instruction at the new location gets executed from
> the old space.
> 
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
>   target/hppa/translate.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index 58d7ec1ade..a09112e4ae 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -3777,6 +3777,9 @@ static bool trans_be(DisasContext *ctx, arg_be *a)
>           }
>           copy_iaoq_entry(ctx, cpu_iaoq_b, -1, tmp);
>           tcg_gen_mov_i64(cpu_iasq_b, new_spc);
> +        if (a->n) {
> +            tcg_gen_mov_i64(cpu_iasq_f, new_spc);
> +        }
>           nullify_set(ctx, a->n);
>       }
>       tcg_gen_lookup_and_goto_ptr();


Without use_nullify_skip(), we're going to execute the next instruction even if we know it 
is nullified (a->n).  This is usually because there's a page crossing or breakpoint, and 
we need to take the exception that might be raised there.

So, we advance the queue:

         copy_iaoq_entry(ctx, cpu_iaoq_f, ctx->iaoq_b, cpu_iaoq_b);
         if (ctx->iaoq_b == -1) {
             tcg_gen_mov_i64(cpu_iasq_f, cpu_iasq_b);
         }

then put the branch destination at the back of the queue:

         copy_iaoq_entry(ctx, cpu_iaoq_b, -1, tmp);
         tcg_gen_mov_i64(cpu_iasq_b, new_spc);

Note that iaoq_b is always -1 on a space change.

So your change does not look correct.
What is the issue that you saw?


r~

Re: [PATCH 6/7] target/hppa: mask privilege bits in mfia

[Richard Henderson]

On 3/17/24 12:14, Sven Schnelle wrote:
> mfia should return only the iaoq bits without privilege
> bits.
> 
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
>   target/hppa/translate.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Fixes: 98a9cb792c8 ("target-hppa: Implement system and memory-management insns")
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 7/7] target/hppa: fix do_stdby_e()

[Richard Henderson]

On 3/17/24 12:14, Sven Schnelle wrote:
> stdby,e,m was writing data from the wrong half of the register
> into memory for cases 0-3.
> 
> Signed-off-by: Sven Schnelle<svens@stackframe.org>
> ---
>   target/hppa/op_helper.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)

Fixes: 25460fc5a71 ("target/hppa: Implement STDBY")
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 5/7] target/hppa: copy new_spc to iasq_f on be,n instruction

[Sven Schnelle]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 3/17/24 12:14, Sven Schnelle wrote:
>> Otherwise the first instruction at the new location gets executed from
>> the old space.
>> Signed-off-by: Sven Schnelle <svens@stackframe.org>
>> ---
>>   target/hppa/translate.c | 3 +++
>>   1 file changed, 3 insertions(+)
>> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
>> index 58d7ec1ade..a09112e4ae 100644
>> --- a/target/hppa/translate.c
>> +++ b/target/hppa/translate.c
>> @@ -3777,6 +3777,9 @@ static bool trans_be(DisasContext *ctx, arg_be *a)
>>           }
>>           copy_iaoq_entry(ctx, cpu_iaoq_b, -1, tmp);
>>           tcg_gen_mov_i64(cpu_iasq_b, new_spc);
>> +        if (a->n) {
>> +            tcg_gen_mov_i64(cpu_iasq_f, new_spc);
>> +        }
>>           nullify_set(ctx, a->n);
>>       }
>>       tcg_gen_lookup_and_goto_ptr();
>
>
> Without use_nullify_skip(), we're going to execute the next
> instruction even if we know it is nullified (a->n).  This is usually
> because there's a page crossing or breakpoint, and we need to take the
> exception that might be raised there.
>
> So, we advance the queue:
>
>         copy_iaoq_entry(ctx, cpu_iaoq_f, ctx->iaoq_b, cpu_iaoq_b);
>         if (ctx->iaoq_b == -1) {
>             tcg_gen_mov_i64(cpu_iasq_f, cpu_iasq_b);
>         }
>
> then put the branch destination at the back of the queue:
>
>         copy_iaoq_entry(ctx, cpu_iaoq_b, -1, tmp);
>         tcg_gen_mov_i64(cpu_iasq_b, new_spc);
>
> Note that iaoq_b is always -1 on a space change.
>
> So your change does not look correct.
> What is the issue that you saw?

I was running the CPU instruction tests from HP Offline Diagnostics, and
some be instruction was ending up in the wrong space. I don't have the
details anymore. I'd say we drop the patch for now and I test it again
later and provide more details.

Re: [PATCH 3/7] target/hppa: fix access_id check

[Sven Schnelle]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 3/17/24 12:14, Sven Schnelle wrote:
>>          /* If bits [31:1] match, and bit 0 is set, suppress write.  */
>> -        int match = ent->access_id * 2 + 1;
>> -
>> -        if (match == env->cr[CR_PID1] || match == env->cr[CR_PID2] ||
>> -            match == env->cr[CR_PID3] || match == env->cr[CR_PID4]) {
>> -            prot &= PAGE_READ | PAGE_EXEC;
>> -            if (type == PAGE_WRITE) {
>> -                ret = EXCP_DMPI;
>> -                goto egress;
>> +        uint32_t pid;
>> +        if (match_prot_id(env, ent->access_id, &pid)) {
>> +            if ((pid & 1) && (prot & PROT_WRITE)) {
>> +                prot &= ~PROT_WRITE;
>>              }
>> +        } else {
>> +            prot = 0;
>>          }
>
> You're losing the data memory protection id trap.

Oops, indeed.

> Therefore I suggest
> [..]
> At this point there are now a couple of hppa_is_pa20() calls within
> hppa_get_physical_address, which could be unified to a single local
> bool.

Thanks, i'll take your version and update the patch.