| Message ID | 20240215171512.800892-1-npiggin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/ppc: BHRB avoid using host pointer in translated code | expand |
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
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~
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
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
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
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
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(-)