[PATCH] target/ppc: BHRB avoid using host pointer in translated code

[PATCH] target/ppc: BHRB avoid using host pointer in translated code

[Nicholas Piggin]

Calculate the BHRB base from arithmetic on the tcg_env target ptr.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Hi Glenn,

I think I have to squash this into the BHRB series. 32-bit host
compile shows up a size mismatch warning... I think it's not quite
right to be using host pointer directly in target code. The change
of offset and mask to 32-bit is needed due to to seemingly missing
tl->ptr conversion helpers, but 32-bit is okay for those anyway.

Thanks,
Nick

 target/ppc/cpu.h       |  5 ++---
 target/ppc/cpu_init.c  |  1 -
 target/ppc/machine.c   |  2 +-
 target/ppc/translate.c | 45 +++++++++++++++++++++---------------------
 4 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index eaa24f2c95..6b050ea628 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1325,10 +1325,9 @@ struct CPUArchState {
 #ifdef TARGET_PPC64
     /* Branch History Rolling Buffer (BHRB) resources */
     target_ulong bhrb_num_entries;
-    target_ulong bhrb_base;
     target_ulong bhrb_filter;
-    target_ulong bhrb_offset;
-    target_ulong bhrb_offset_mask;
+    uint32_t bhrb_offset_mask;
+    uint32_t bhrb_offset;
     uint64_t bhrb[BHRB_MAX_NUM_ENTRIES];
 #endif
 
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 2494527765..262b1d7852 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6117,7 +6117,6 @@ static void bhrb_init_state(CPUPPCState *env, target_long num_entries_log2)
             num_entries_log2 = BHRB_MAX_NUM_ENTRIES_LOG2;
         }
         env->bhrb_num_entries = 1 << num_entries_log2;
-        env->bhrb_base = (target_long)&env->bhrb[0];
         env->bhrb_offset_mask = (env->bhrb_num_entries * sizeof(uint64_t)) - 1;
     }
 }
diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index 731dd8df35..3541cd83cd 100644
--- a/target/ppc/machine.c
+++ b/target/ppc/machine.c
@@ -724,7 +724,7 @@ static const VMStateDescription vmstate_bhrb = {
     .minimum_version_id = 1,
     .needed = bhrb_needed,
     .fields = (VMStateField[]) {
-        VMSTATE_UINTTL(env.bhrb_offset, PowerPCCPU),
+        VMSTATE_UINT32(env.bhrb_offset, PowerPCCPU),
         VMSTATE_UINT64_ARRAY(env.bhrb, PowerPCCPU, BHRB_MAX_NUM_ENTRIES),
         VMSTATE_END_OF_LIST()
     }
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 81afc892de..05f0f1ac52 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -4167,21 +4167,24 @@ static void gen_rvwinkle(DisasContext *ctx)
 #endif /* defined(CONFIG_USER_ONLY) */
 }
 
-static inline TCGv gen_write_bhrb(TCGv base, TCGv offset, TCGv mask, TCGv value)
+static TCGv_i32 gen_write_bhrb(TCGv_i32 offset, TCGv_i32 mask, TCGv_i64 value)
 {
-    TCGv tmp = tcg_temp_new();
+    TCGv_ptr ptr = tcg_temp_new_ptr();
+    TCGv_i32 tmp = tcg_temp_new_i32();
 
-    /* add base and offset to get address of bhrb entry */
-    tcg_gen_add_tl(tmp, base, offset);
+    /* add base and offset to tcg_env to get address of bhrb entry */
+    tcg_gen_addi_i32(tmp, offset, offsetof(CPUPPCState, bhrb));
+    tcg_gen_ext_i32_ptr(ptr, tmp);
+    tcg_gen_add_ptr(ptr, ptr, tcg_env);
 
     /* store value into bhrb at bhrb_offset */
-    tcg_gen_st_i64(value, (TCGv_ptr)tmp, 0);
+    tcg_gen_st_i64(value, ptr, 0);
 
     /* add 8 to current bhrb_offset */
-    tcg_gen_addi_tl(offset, offset, 8);
+    tcg_gen_addi_i32(offset, offset, 8);
 
     /* apply offset mask */
-    tcg_gen_and_tl(offset, offset, mask);
+    tcg_gen_and_i32(offset, offset, mask);
 
     return offset;
 }
@@ -4193,10 +4196,9 @@ static inline void gen_update_branch_history(DisasContext *ctx,
                                              target_long inst_type)
 {
 #if defined(TARGET_PPC64)
-    TCGv base;
     TCGv tmp;
-    TCGv offset;
-    TCGv mask;
+    TCGv_i32 offset;
+    TCGv_i32 mask;
     TCGLabel *no_update;
 
     if (ctx->has_cfar) {
@@ -4216,32 +4218,31 @@ static inline void gen_update_branch_history(DisasContext *ctx,
     tcg_gen_andi_tl(tmp, tmp, inst_type);
     tcg_gen_brcondi_tl(TCG_COND_EQ, tmp, 0, no_update);
 
-    base = tcg_temp_new();
-    offset = tcg_temp_new();
-    mask = tcg_temp_new();
-
-    /* load bhrb base address */
-    tcg_gen_ld_tl(base, tcg_env, offsetof(CPUPPCState, bhrb_base));
+    offset = tcg_temp_new_i32();
+    mask = tcg_temp_new_i32();
 
     /* load current bhrb_offset */
-    tcg_gen_ld_tl(offset, tcg_env, offsetof(CPUPPCState, bhrb_offset));
+    tcg_gen_ld_i32(offset, tcg_env, offsetof(CPUPPCState, bhrb_offset));
 
     /* load a BHRB offset mask */
-    tcg_gen_ld_tl(mask, tcg_env, offsetof(CPUPPCState, bhrb_offset_mask));
+    tcg_gen_ld_i32(mask, tcg_env, offsetof(CPUPPCState, bhrb_offset_mask));
 
-    offset = gen_write_bhrb(base, offset, mask, tcg_constant_i64(nip));
+    offset = gen_write_bhrb(offset, mask, tcg_constant_i64(nip));
 
     /* Also record the target address for XL-Form branches */
     if (inst_type & BHRB_TYPE_XL_FORM) {
+        TCGv_i64 t = tcg_temp_new_i64();
+
+        tcg_gen_extu_tl_i64(t, target);
 
         /* Set the 'T' bit for target entries */
-        tcg_gen_ori_tl(tmp, target, 0x2);
+        tcg_gen_ori_i64(t, target, 0x2);
 
-        offset = gen_write_bhrb(base, offset, mask, tmp);
+        offset = gen_write_bhrb(offset, mask, t);
     }
 
     /* save updated bhrb_offset for next time */
-    tcg_gen_st_tl(offset, tcg_env, offsetof(CPUPPCState, bhrb_offset));
+    tcg_gen_st_i32(offset, tcg_env, offsetof(CPUPPCState, bhrb_offset));
 
     gen_set_label(no_update);
 #endif
-- 
2.42.0

Re: [PATCH] target/ppc: BHRB avoid using host pointer in translated code

[Peter Maydell]

On Thu, 15 Feb 2024 at 17:16, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Calculate the BHRB base from arithmetic on the tcg_env target ptr.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> Hi Glenn,
>
> I think I have to squash this into the BHRB series. 32-bit host
> compile shows up a size mismatch warning... I think it's not quite
> right to be using host pointer directly in target code. The change
> of offset and mask to 32-bit is needed due to to seemingly missing
> tl->ptr conversion helpers, but 32-bit is okay for those anyway.

There's nothing inherently wrong with it (depending on what the
pointer is pointing to!), but you need to use the right type.
target_ulong and the _tl suffix are for the type which
depends on the size of the target's 'long'. The TCG type which is
"size of a host pointer" is TCG_TYPE_PTR, and you want the _ptr
suffix functions and to pass it around with TCGv_ptr.

thanks
-- PMM

Re: [PATCH] target/ppc: BHRB avoid using host pointer in translated code

[Richard Henderson]

On 2/15/24 07:15, Nicholas Piggin wrote:
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 731dd8df35..3541cd83cd 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -724,7 +724,7 @@ static const VMStateDescription vmstate_bhrb = {
>       .minimum_version_id = 1,
>       .needed = bhrb_needed,
>       .fields = (VMStateField[]) {
> -        VMSTATE_UINTTL(env.bhrb_offset, PowerPCCPU),
> +        VMSTATE_UINT32(env.bhrb_offset, PowerPCCPU),

This requires a version bump.

> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 81afc892de..05f0f1ac52 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -4167,21 +4167,24 @@ static void gen_rvwinkle(DisasContext *ctx)
>   #endif /* defined(CONFIG_USER_ONLY) */
>   }
>   
> -static inline TCGv gen_write_bhrb(TCGv base, TCGv offset, TCGv mask, TCGv value)
> +static TCGv_i32 gen_write_bhrb(TCGv_i32 offset, TCGv_i32 mask, TCGv_i64 value)
>   {
> -    TCGv tmp = tcg_temp_new();
> +    TCGv_ptr ptr = tcg_temp_new_ptr();
> +    TCGv_i32 tmp = tcg_temp_new_i32();
>   
> -    /* add base and offset to get address of bhrb entry */
> -    tcg_gen_add_tl(tmp, base, offset);
> +    /* add base and offset to tcg_env to get address of bhrb entry */
> +    tcg_gen_addi_i32(tmp, offset, offsetof(CPUPPCState, bhrb));
> +    tcg_gen_ext_i32_ptr(ptr, tmp);
> +    tcg_gen_add_ptr(ptr, ptr, tcg_env);
>   
>       /* store value into bhrb at bhrb_offset */
> -    tcg_gen_st_i64(value, (TCGv_ptr)tmp, 0);
> +    tcg_gen_st_i64(value, ptr, 0);

Better to add the constant with the store offset.

   tcg_gen_ext_i32_ptr(ptr, offset);
   tcg_gen_add_ptr(ptr, ptr, tcg_env);
   tcg_gen_st_i64(value, ptr, offsetof(bhrb));


r~

Re: [PATCH] target/ppc: BHRB avoid using host pointer in translated code

[Nicholas Piggin]

On Fri Feb 16, 2024 at 5:42 AM AEST, Richard Henderson wrote:
> On 2/15/24 07:15, Nicholas Piggin wrote:
> > diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> > index 731dd8df35..3541cd83cd 100644
> > --- a/target/ppc/machine.c
> > +++ b/target/ppc/machine.c
> > @@ -724,7 +724,7 @@ static const VMStateDescription vmstate_bhrb = {
> >       .minimum_version_id = 1,
> >       .needed = bhrb_needed,
> >       .fields = (VMStateField[]) {
> > -        VMSTATE_UINTTL(env.bhrb_offset, PowerPCCPU),
> > +        VMSTATE_UINT32(env.bhrb_offset, PowerPCCPU),
>
> This requires a version bump.

In this case the first patch hasn't gone upstream yet.

> > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > index 81afc892de..05f0f1ac52 100644
> > --- a/target/ppc/translate.c
> > +++ b/target/ppc/translate.c
> > @@ -4167,21 +4167,24 @@ static void gen_rvwinkle(DisasContext *ctx)
> >   #endif /* defined(CONFIG_USER_ONLY) */
> >   }
> >   
> > -static inline TCGv gen_write_bhrb(TCGv base, TCGv offset, TCGv mask, TCGv value)
> > +static TCGv_i32 gen_write_bhrb(TCGv_i32 offset, TCGv_i32 mask, TCGv_i64 value)
> >   {
> > -    TCGv tmp = tcg_temp_new();
> > +    TCGv_ptr ptr = tcg_temp_new_ptr();
> > +    TCGv_i32 tmp = tcg_temp_new_i32();
> >   
> > -    /* add base and offset to get address of bhrb entry */
> > -    tcg_gen_add_tl(tmp, base, offset);
> > +    /* add base and offset to tcg_env to get address of bhrb entry */
> > +    tcg_gen_addi_i32(tmp, offset, offsetof(CPUPPCState, bhrb));
> > +    tcg_gen_ext_i32_ptr(ptr, tmp);
> > +    tcg_gen_add_ptr(ptr, ptr, tcg_env);
> >   
> >       /* store value into bhrb at bhrb_offset */
> > -    tcg_gen_st_i64(value, (TCGv_ptr)tmp, 0);
> > +    tcg_gen_st_i64(value, ptr, 0);
>
> Better to add the constant with the store offset.
>
>    tcg_gen_ext_i32_ptr(ptr, offset);
>    tcg_gen_add_ptr(ptr, ptr, tcg_env);
>    tcg_gen_st_i64(value, ptr, offsetof(bhrb));

Good suggestion thanks. Maybe in light of the fact I was wrong
about not using host pointer here, the original approach is
better and just needs a smaller fix for 32-bit hosts.

Thanks,
Nick

Re: [PATCH] target/ppc: BHRB avoid using host pointer in translated code

[Nicholas Piggin]

On Fri Feb 16, 2024 at 3:50 AM AEST, Peter Maydell wrote:
> On Thu, 15 Feb 2024 at 17:16, Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > Calculate the BHRB base from arithmetic on the tcg_env target ptr.
> >
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> > Hi Glenn,
> >
> > I think I have to squash this into the BHRB series. 32-bit host
> > compile shows up a size mismatch warning... I think it's not quite
> > right to be using host pointer directly in target code. The change
> > of offset and mask to 32-bit is needed due to to seemingly missing
> > tl->ptr conversion helpers, but 32-bit is okay for those anyway.
>
> There's nothing inherently wrong with it (depending on what the
> pointer is pointing to!), but you need to use the right type.

Ah okay, thanks for the correction.

> target_ulong and the _tl suffix are for the type which
> depends on the size of the target's 'long'. The TCG type which is
> "size of a host pointer" is TCG_TYPE_PTR, and you want the _ptr
> suffix functions and to pass it around with TCGv_ptr.

In that case, original approach may be better with small fixes
for 32-bit host.

Thanks,
Nick

Re: [PATCH] target/ppc: BHRB avoid using host pointer in translated code

[Miles Glenn]

On Tue, 2024-02-20 at 10:35 +1000, Nicholas Piggin wrote:
> On Fri Feb 16, 2024 at 3:50 AM AEST, Peter Maydell wrote:
> > On Thu, 15 Feb 2024 at 17:16, Nicholas Piggin <npiggin@gmail.com>
> > wrote:
> > > Calculate the BHRB base from arithmetic on the tcg_env target
> > > ptr.
> > > 
> > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > > ---
> > > Hi Glenn,
> > > 
> > > I think I have to squash this into the BHRB series. 32-bit host
> > > compile shows up a size mismatch warning... I think it's not
> > > quite
> > > right to be using host pointer directly in target code. The
> > > change
> > > of offset and mask to 32-bit is needed due to to seemingly
> > > missing
> > > tl->ptr conversion helpers, but 32-bit is okay for those anyway.
> > 
> > There's nothing inherently wrong with it (depending on what the
> > pointer is pointing to!), but you need to use the right type.
> 
> Ah okay, thanks for the correction.
> 
> > target_ulong and the _tl suffix are for the type which
> > depends on the size of the target's 'long'. The TCG type which is
> > "size of a host pointer" is TCG_TYPE_PTR, and you want the _ptr
> > suffix functions and to pass it around with TCGv_ptr.
> 
> In that case, original approach may be better with small fixes
> for 32-bit host.
> 
> Thanks,
> Nick

Peter/Nick, thanks for looking into this.  I'll work on submitting
a v4 of the original BHRB series adding Peter's suggestions (and
probably a rebase) as soon as I have a chance.  Unfortunately, I have
some higher priority items to work on at the moment, so it could take
a week or two before I can get to it.

Thanks,
Glenn