On 21/10/26 12:42, Richard Henderson wrote:
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.
Bare with me, x86 is the architecture I have less interest in, I'm
looking at your patch because you are pinging for review since 1 month.
I couldn't grasp the "exactly that" so asked a LLM to trigger the path,
and it found a way updating test-branch-wrap16.S to use paging aliases
(see below).
I can not make sense of it, so feel free to disregard as slop :)
AI;DR;
1. No mask is emitted. The TB translated at linear 0x400100 (EIP 0x0100)
contains only:
0x00400100: 66 e9 fc 00 jmp 0x400200
add_i32 eip,eip,$0x100
goto_tb $0x0
There's no and and goto_tb is still used, as predicted.
2. The same TB is reused at a different EIP:
Trace 0: 0x3001dc200 [00400000/0000000000400100/...] first run
Trace 0: 0x3001dc200 [00400000/0000000000410100/...] reused at EIP 0x10100
3. The wrong target is reached. Execution continues at 0x410200 (EIP
0x10200 instead of 0x0200), takes the failure branch at 0x0041022a, and
prints FAIL: scenario 3.
4. Restoring CODE32(s) && ot == MO_16 || in front of the page check
gives PASS.
-- >8 --
diff --git a/tests/tcg/i386/system/test-branch-wrap16.S
b/tests/tcg/i386/system/test-branch-wrap16.S
index 9a64b4c6d04..7b7acd74b9a 100644
--- a/tests/tcg/i386/system/test-branch-wrap16.S
+++ b/tests/tcg/i386/system/test-branch-wrap16.S
@@ -1,20 +1,35 @@
/*
* SPDX-License-Identifier: GPL-2.0-or-later
*
- * A 16-bit near branch must truncate EIP to 16 bits.
+ * A near branch with a 16-bit operand size must truncate EIP to 16 bits.
*
- * The test jumps from IP 0xFFF0 to IP 0x0020 in 16-bit code. The jump
- * crosses 0xFFFF. For correct 16-bit behaviour the CPU should wrap the
- * target to 0x0020, rather than branching to 0x10020.
+ * Scenario 1 (16-bit code): jumps from IP 0xFFF0 to IP 0x0020 in 16-bit
+ * code. The jump crosses 0xFFFF. For correct 16-bit behaviour the CPU
+ * should wrap the target to 0x0020, rather than branching to 0x10020.
+ *
+ * Scenario 2 (32-bit code, data16 branch): jumps from EIP 0xFFF0 to
+ * EIP 0x0020 in 32-bit code with a 0x66 operand-size prefix (ot == MO_16
+ * in CODE32). EIP must again wrap to 0x0020.
*
* TCG used to skip the wrap when the untruncated target (CS base +
* 0x10020) was on the same guest page as the jump. That only happens
- * when the CS base is not page aligned, so the code segment starts at
+ * when the CS base is not page aligned, so each code segment starts at
* a page boundary plus 0x10.
+ *
+ * Scenario 3 (32-bit code, data16 branch, TB reuse): with CF_PCREL a TB
+ * is looked up by physical address, so a TB translated at EIP 0x0100 is
+ * reused at EIP 0x10100 when both linear addresses alias the same
+ * physical page. A data16 branch from 0x0100 to 0x0200 does not wrap at
+ * translation time, but on reuse it must still truncate 0x10200 to
+ * 0x0200. The target checks its own EIP with call/pop.
*/
#define SEL_CODE32 0x08
#define SEL_CODE16 0x18
+#define SEL_WR32 0x20
+#define SEL_ALIAS 0x28
+
+#define ALIAS_BASE 0x400000 /* linear base of SEL_ALIAS */
.data
@@ -30,6 +45,14 @@ gdt16_code16:
/* 16-bit code, 64 KiB limit; main fills in the base */
.word 0xFFFF, 0
.byte 0, 0x9b, 0x00, 0
+gdt16_wrap32:
+ /* 32-bit code, 64 KiB limit; main fills in the base */
+ .word 0xFFFF, 0
+ .byte 0, 0x9b, 0x40, 0
+gdt16_alias:
+ /* 32-bit code, 4 GiB limit, base ALIAS_BASE */
+ .word 0xFFFF, ALIAS_BASE & 0xFFFF
+ .byte (ALIAS_BASE >> 16) & 0xFF, 0x9b, 0xCF, ALIAS_BASE >> 24
gdt16_end:
gdtr16:
@@ -37,10 +60,28 @@ gdtr16:
.long gdt16
fail_msg:
- .asciz "FAIL: branch left EIP untruncated\n"
+ .asciz "FAIL: scenario %d: branch left EIP untruncated\n"
pass_msg:
.asciz "PASS\n"
+ .balign 4
+scenario:
+ .long 0
+alias_runs:
+ .long 0
+
+ /*
+ * Page tables: 0-4 MiB identity mapped, and the two linear pages
+ * ALIAS_BASE and ALIAS_BASE + 0x10000 both mapped to alias_page.
+ */
+ .balign 0x1000
+alias_pd:
+ .fill 1024, 4, 0
+alias_pt0:
+ .fill 1024, 4, 0
+alias_pt1:
+ .fill 1024, 4, 0
+
.section .text16, "ax"
.code16
@@ -55,7 +96,7 @@ wrap16_cs_base:
.skip 0x10
wrap16_target:
/* jmp lands here when EIP wraps to 0x0020 */
- ljmpl $SEL_CODE32, $test_passed
+ ljmpl $SEL_CODE32, $test32_setup
.org wrap16_cs_base + 0xfff0
wrap16_entry:
@@ -73,6 +114,72 @@ wrap16_fail:
/* jmp lands here when EIP does not wrap (0x10020) */
ljmpl $SEL_CODE32, $test_failed
+ /*
+ * Second segment: 32-bit code, same non-page-aligned layout.
+ * The branch here uses a 0x66 prefix, i.e. ot == MO_16 in
+ * CODE32, which previously disabled goto_tb unconditionally.
+ */
+ .balign 0x1000
+ .skip 0x10
+wrap32_cs_base:
+
+ .skip 0x10
+wrap32_target:
+ /* data16 jmp lands here when EIP wraps to 0x0020 */
+ .code32
+ ljmp $SEL_CODE32, $test_alias_setup
+
+ .org wrap32_cs_base + 0xfff0
+wrap32_entry:
+ .code32
+ /*
+ * Same wrapping branch, but in 32-bit code with an operand-size
+ * prefix: data16 'jmp rel16'.
+ */
+ .byte 0x66, 0xe9 /* o16 jmp rel16 */
+ .word wrap32_target - (. + 2) /* cw: 16-bit offset */
+
+ .org wrap32_cs_base + 0x10020
+wrap32_fail:
+ .code32
+ /* data16 jmp lands here when EIP does not wrap (0x10020) */
+ ljmp $SEL_CODE32, $test_failed
+
+ /*
+ * Third segment: one physical page seen through SEL_ALIAS at
+ * EIP 0x0000 and EIP 0x10000.
+ */
+ .balign 0x1000
+alias_page:
+
+ .org alias_page + 0x100
+alias_entry:
+ .code32
+ /*
+ * data16 'jmp alias_target': does not cross a page at EIP 0x0100,
+ * so TCG may omit the truncation, but the TB is re-run at 0x10100.
+ */
+ .byte 0x66, 0xe9 /* o16 jmp rel16 */
+ .word alias_target - (. + 2) /* cw: 16-bit offset */
+
+ .org alias_page + 0x200
+alias_target:
+ call alias_eip
+alias_eip:
+ popl %eax
+ cmpl $(alias_eip - alias_page), %eax
+ jne 1f
+ incl alias_runs
+ cmpl $2, alias_runs
+ je 2f
+ /* Re-run the translated branch TB through the aliased page */
+ ljmp $SEL_ALIAS, $(alias_entry - alias_page + 0x10000)
+2:
+ ljmp $SEL_CODE32, $test_passed
+1:
+ /* EIP was 0x10205: the branch did not truncate EIP */
+ ljmp $SEL_CODE32, $test_failed
+
.text
.code32
.globl main
@@ -82,10 +189,48 @@ main:
shrl $16, %eax
movb %al, gdt16_code16 + 4
movb %ah, gdt16_code16 + 7
+
+ movl $wrap32_cs_base, %eax
+ movw %ax, gdt16_wrap32 + 2
+ shrl $16, %eax
+ movb %al, gdt16_wrap32 + 4
+ movb %ah, gdt16_wrap32 + 7
+
lgdt gdtr16
+ movl $1, scenario
ljmpw $SEL_CODE16, $0xFFF0
+test32_setup:
+ movl $2, scenario
+ ljmp $SEL_WR32, $0xFFF0
+
+test_alias_setup:
+ movl $3, scenario
+ xorl %ecx, %ecx
+1:
+ movl %ecx, %eax
+ shll $12, %eax
+ orl $3, %eax
+ movl %eax, alias_pt0(,%ecx,4)
+ incl %ecx
+ cmpl $1024, %ecx
+ jne 1b
+
+ movl $alias_pt0 + 3, alias_pd
+ movl $alias_pt1 + 3, alias_pd + (ALIAS_BASE >> 22) * 4
+ movl $alias_page + 3, alias_pt1
+ movl $alias_page + 3, alias_pt1 + 0x10 * 4
+
+ movl $alias_pd, %eax
+ movl %eax, %cr3
+ movl %cr0, %eax
+ orl $0x80000000, %eax
+ movl %eax, %cr0
+
+ /* First run: translate the branch TB at EIP 0x0100 */
+ ljmp $SEL_ALIAS, $(alias_entry - alias_page)
+
test_passed:
pushl $pass_msg
call ml_printf
@@ -94,6 +239,7 @@ test_passed:
ret
test_failed:
+ pushl scenario
pushl $fail_msg
call ml_printf
addl $8, %esp
---
