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