On 10/21/26 10:50, Philippe Mathieu-Daudé wrote:
Hi Richard,
On 27/9/26 01:56, Richard Henderson wrote:
A 16-bit near branch that wraps through 0xFFFF must truncate EIP.
TCG skipped this when the target was on the same page, which can
happen when the CS base is not page aligned.
There was an existing test for this further up that attempted to
test this, but was restricted to a 16-bit branch in 32-bit mode,
which can be subsumed.
The previous test for pc being on the same page, rather than eip, is
only relevant to using goto_tb not to eip wrapping. There is another
test for pc on the same page further down, via translator_use_goto_tb.
Cc: [email protected]
Fixes: e3a79e0e878 ("target/i386: Enable TARGET_TB_PCREL")
Reported-by: Paul Galbraith <[email protected]>
Signed-off-by: Richard Henderson <[email protected]>
---
target/i386/tcg/translate.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index a38cc2c15d0..18706ec8a74 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -2015,9 +2015,6 @@ static void gen_jmp_rel(DisasContext *s, MemOp
ot, int diff, int tb_num)
if (!CODE64(s)) {
if (ot == MO_16) {
mask = 0xffff;
- if (tb_cflags(s->base.tb) & CF_PCREL && CODE32(s)) {
- use_goto_tb = false;
- }
} else {
mask = 0xffffffff;
}
@@ -2025,15 +2022,20 @@ static void gen_jmp_rel(DisasContext *s, MemOp
ot, int diff, int tb_num)
new_eip &= mask;
if (tb_cflags(s->base.tb) & CF_PCREL) {
+ assert(s->pc_save_valid);
tcg_gen_addi_tl(cpu_eip, cpu_eip, new_pc - s->pc_save);
- /*
- * If we can prove the branch does not leave the page and we
have
- * no extra masking to apply (data16 branch in code32, see
above),
- * then we have also proven that the addition does not wrap.
- */
- if (!use_goto_tb || !translator_is_same_page(&s->base,
new_pc)) {
- tcg_gen_andi_tl(cpu_eip, cpu_eip, mask);
- use_goto_tb = false;
+
+ if (mask != -1) {
+ /*
+ * If eip "page" doesn't change, then the addition cannot
+ * leave [0, mask] on any rerun of this TB, which only
+ * ever shifts EIP by whole pages.
+ */
+ target_ulong old_eip = s->pc_save - s->cs_base;
+ if ((old_eip ^ new_eip) & TARGET_PAGE_MASK) {
Is it OK to skip the previous ot == MO_16 and CODE32() checks?
Yes.
The next patch only tests 16-bit, not 32-bit protected. Can we
complement with a 32-bit test? Since this is a qemu-stable@ fix,
can you better explain the "subsuming" and why we don't need this
safer check:
if ((CODE32(s) && ot == MO_16)
Because this was trying to do exactly what...
|| ((old_eip ^ new_eip) & TARGET_PAGE_MASK)) {
... this does, but more accurately.
r~
