| Message ID | 1475584975-25099-28-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Commit | 9b6a3ea7a699594162ed3d11e4e04b98568dc5c0 |
| Headers | show |
Peter Maydell <peter.maydell@linaro.org> writes: > In the ARM v6 architecture, 'sub pc, pc, 1' is not an interworking > branch, so the computed new value is written to r15 as a normal > value. The architecture says that in this case, bits [1:0] of > the value written must be ignored if we are in ARM mode (or > bit [0] ignored if in Thumb mode); this is a change from the > ARMv4/v5 specification that behaviour is UNPREDICTABLE. > Use the correct mask on the PC value when doing a non-interworking > store to PC. > > A popular library used on RaspberryPi uses this instruction > as part of a trick to determine whether it is running on > ARMv6 or ARMv7, and we were mishandling the sequence. > > Fixes bug: https://bugs.launchpad.net/bugs/1625295 > > Reported-by: <stu.axon@gmail.com> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > Message-id: 1474380941-4730-1-git-send-email-peter.maydell@linaro.org I'm not sure how but this seems to have regressed my ARMv7 test images (currently Linux 4.7.7). With this change I see the guest spinning in the vectors table. If I comment out the change it boots. I'll dig some more but as this affects store_reg are there any cases when writing to the PC with offset bits would be correct? > --- > target-arm/translate.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/target-arm/translate.c b/target-arm/translate.c > index 693d4bc..8df24bf 100644 > --- a/target-arm/translate.c > +++ b/target-arm/translate.c > @@ -180,7 +180,12 @@ static inline TCGv_i32 load_reg(DisasContext *s, int reg) > static void store_reg(DisasContext *s, int reg, TCGv_i32 var) > { > if (reg == 15) { > - tcg_gen_andi_i32(var, var, ~1); > + /* In Thumb mode, we must ignore bit 0. > + * In ARM mode, for ARMv4 and ARMv5, it is UNPREDICTABLE if bits [1:0] > + * are not 0b00, but for ARMv6 and above, we must ignore bits [1:0]. > + * We choose to ignore [1:0] in ARM mode for all architecture versions. > + */ > + tcg_gen_andi_i32(var, var, s->thumb ? ~1 : ~3); > s->is_jmp = DISAS_JUMP; > } > tcg_gen_mov_i32(cpu_R[reg], var); -- Alex Bennée
On 14 October 2016 at 07:44, Alex Bennée <alex.bennee@linaro.org> wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > >> In the ARM v6 architecture, 'sub pc, pc, 1' is not an interworking >> branch, so the computed new value is written to r15 as a normal >> value. The architecture says that in this case, bits [1:0] of >> the value written must be ignored if we are in ARM mode (or >> bit [0] ignored if in Thumb mode); this is a change from the >> ARMv4/v5 specification that behaviour is UNPREDICTABLE. >> Use the correct mask on the PC value when doing a non-interworking >> store to PC. >> >> A popular library used on RaspberryPi uses this instruction >> as part of a trick to determine whether it is running on >> ARMv6 or ARMv7, and we were mishandling the sequence. >> >> Fixes bug: https://bugs.launchpad.net/bugs/1625295 >> >> Reported-by: <stu.axon@gmail.com> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> Message-id: 1474380941-4730-1-git-send-email-peter.maydell@linaro.org > > I'm not sure how but this seems to have regressed my ARMv7 test images > (currently Linux 4.7.7). With this change I see the guest spinning in > the vectors table. If I comment out the change it boots. > > I'll dig some more but as this affects store_reg are there any cases > when writing to the PC with offset bits would be correct? Look for the patch I sent earlier that fixes a regression in returning from exceptions to thumb addresses that are only 2 aligned. That will probably fix it. thanks -- PMM
diff --git a/target-arm/translate.c b/target-arm/translate.c index 693d4bc..8df24bf 100644 --- a/target-arm/translate.c +++ b/target-arm/translate.c @@ -180,7 +180,12 @@ static inline TCGv_i32 load_reg(DisasContext *s, int reg) static void store_reg(DisasContext *s, int reg, TCGv_i32 var) { if (reg == 15) { - tcg_gen_andi_i32(var, var, ~1); + /* In Thumb mode, we must ignore bit 0. + * In ARM mode, for ARMv4 and ARMv5, it is UNPREDICTABLE if bits [1:0] + * are not 0b00, but for ARMv6 and above, we must ignore bits [1:0]. + * We choose to ignore [1:0] in ARM mode for all architecture versions. + */ + tcg_gen_andi_i32(var, var, s->thumb ? ~1 : ~3); s->is_jmp = DISAS_JUMP; } tcg_gen_mov_i32(cpu_R[reg], var);
In the ARM v6 architecture, 'sub pc, pc, 1' is not an interworking branch, so the computed new value is written to r15 as a normal value. The architecture says that in this case, bits [1:0] of the value written must be ignored if we are in ARM mode (or bit [0] ignored if in Thumb mode); this is a change from the ARMv4/v5 specification that behaviour is UNPREDICTABLE. Use the correct mask on the PC value when doing a non-interworking store to PC. A popular library used on RaspberryPi uses this instruction as part of a trick to determine whether it is running on ARMv6 or ARMv7, and we were mishandling the sequence. Fixes bug: https://bugs.launchpad.net/bugs/1625295 Reported-by: <stu.axon@gmail.com> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Message-id: 1474380941-4730-1-git-send-email-peter.maydell@linaro.org --- target-arm/translate.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) -- 2.7.4