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
