Re: [Qemu-devel] [PATCH 03/11] target/arm: Introduce read_pc

[Peter Maydell]

On Wed, 7 Aug 2019 at 19:04, Richard Henderson
<address@hidden> wrote:
>
> On 8/7/19 10:27 AM, Peter Maydell wrote:
> >> +/* The architectural value of PC.  */
> >> +static uint32_t read_pc(DisasContext *s)
> >> +{
> >> +    return s->pc_curr + (s->thumb ? 4 : 8);
> >> +}
> >> +
> >>  /* Set a variable to the value of a CPU register.  */
> >>  static void load_reg_var(DisasContext *s, TCGv_i32 var, int reg)
> >>  {
> >>      if (reg == 15) {
> >> -        uint32_t addr;
> >> -        /* normally, since we updated PC, we need only to add one insn */
> >> -        if (s->thumb)
> >> -            addr = (long)s->pc + 2;
> >> -        else
> >> -            addr = (long)s->pc + 4;
> >> -        tcg_gen_movi_i32(var, addr);
> >> +        tcg_gen_movi_i32(var, read_pc(s));
> >
> > So previously:
> >  * for A32 we would return s->pc + 4, which is the same as s->pc_curr + 8
> >  * for T16 we would return s->pc + 2, which is the same as s->pc_curr + 4
> >  * for T32 we would return s->pc + 2 -- but that's not the same as
> >    s->pc_curr + 4, it's s->pc_curr + 6...
> >
> > Since s->pc_curr + 4 is the right architectural answer, are we
> > fixing a bug here? Or are all the places where T32 code calls
> > this function UNPREDICTABLE for the reg == 15 case ?
>
> I believe that this is UNPREDICTABLE.
>
> The T32 cases that reference the PC that are not UNPREDICTABLE, literal memory
> references and adr, are all of the form (s->pc & ~3) and do not come through
> load_reg_var().  Those will be unified by add_reg_for_lit() in the next patch.

Yeah, that was my assumption -- at some previous point rather
than making load_reg/load_reg_var do the right thing for 32-bit
Thumb insns we just fixed up all the callsites to specialcase 15...

How about we add this to the commit message?

This changes the behaviour for load_reg() and load_reg_var()
when called with reg==15 from a 32-bit Thumb instruction:
previously they would have returned the incorrect value
of pc_curr + 6, and now they will return the architecturally
correct value of PC, which is pc_curr + 4. This will not
affect well-behaved guest software, because all of the places
we call these functions from T32 code are instructions where
using r15 is UNPREDICTABLE. Using the architectural PC value
here is more consistent with the T16 and A32 behaviour.

thanks
-- PMM