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(-)
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.0On 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 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 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
© 2016 - 2024 Red Hat, Inc.