* [PATCH] target/i386: pcrel: store low bits of physical address in data[0]
@ 2024-01-17 15:51 Paolo Bonzini
2024-01-17 20:32 ` Richard Henderson
2024-01-17 21:13 ` Mark Cave-Ayland
0 siblings, 2 replies; 4+ messages in thread
From: Paolo Bonzini @ 2024-01-17 15:51 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-stable, Mark Cave-Ayland, Richard Henderson
For PC-relative translation blocks, env->eip changes during the
execution of a translation block, Therefore, QEMU must be able to
recover an instruction's PC just from the TranslationBlock struct and
the instruction data with. Because a TB will not span two pages, QEMU
stores all the low bits of EIP in the instruction data and replaces them
in x86_restore_state_to_opc. Bits 12 and higher (which may vary between
executions of a PCREL TB, since these only use the physical address in
the hash key) are kept unmodified from env->eip. The assumption is that
these bits of EIP, unlike bits 0-11, will not change as the translation
block executes.
Unfortunately, this is incorrect when the CS base is not aligned to a page.
Then the linear address of the instructions (i.e. the one with the
CS base addred) indeed will never span two pages, but bits 12+ of EIP
can actually change. For example, if CS base is 0x80262200 and EIP =
0x6FF4, the first instruction in the translation block will be at linear
address 0x802691F4. Even a very small TB will cross to EIP = 0x7xxx,
while the linear addresses will remain comfortably within a single page.
The fix is simply to use the low bits of the linear address for data[0],
since those don't change. Then x86_restore_state_to_opc uses tb->cs_base
to compute a temporary linear address (referring to some unknown
instruction in the TB, but with the correct values of bits 12 and higher);
the low bits are replaced with data[0], and EIP is obtained by subtracting
again the CS base.
Huge thanks to Mark Cave-Ayland for the image and initial debugging,
and to Gitlab user @kjliew for help with bisecting another occurrence
of (hopefully!) the same bug.
It should be relatively easy to write a testcase that performs MMIO on
an EIP with different bits 12+ than the first instruction of the translation
block; any help is welcome.
Fixes: e3a79e0e878 ("target/i386: Enable TARGET_TB_PCREL", 2022-10-11)
Cc: qemu-stable@nongnu.org
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: Richard Henderson <richard.henderson@linaro.org>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1964
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2012
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/tcg/tcg-cpu.c | 20 ++++++++++++++++----
target/i386/tcg/translate.c | 1 -
2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
index 6e881e9e276..fa956d35ecd 100644
--- a/target/i386/tcg/tcg-cpu.c
+++ b/target/i386/tcg/tcg-cpu.c
@@ -68,14 +68,26 @@ static void x86_restore_state_to_opc(CPUState *cs,
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = &cpu->env;
int cc_op = data[1];
+ uint64_t new_pc;
if (tb_cflags(tb) & CF_PCREL) {
- env->eip = (env->eip & TARGET_PAGE_MASK) | data[0];
- } else if (tb->flags & HF_CS64_MASK) {
- env->eip = data[0];
+ /*
+ * To ensure that bits 0..11 do not change across the translation block,
+ * PC-relative TBs use linear addresses, i.e. addresses that have the CS
+ * base added, for data[0]. Add the CS base back before replacing the
+ * low bits, and subtract it below just like for non-PC-relative TBs.
+ */
+ uint64_t pc = env->eip + tb->cs_base;
+ new_pc = (pc & TARGET_PAGE_MASK) | data[0];
} else {
- env->eip = (uint32_t)(data[0] - tb->cs_base);
+ new_pc = data[0];
}
+ if (tb->flags & HF_CS64_MASK) {
+ env->eip = new_pc;
+ } else {
+ env->eip = (uint32_t)(new_pc - tb->cs_base);
+ }
+
if (cc_op != CC_OP_DYNAMIC) {
env->cc_op = cc_op;
}
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index cadf13bce43..e193c74472b 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -6996,7 +6996,6 @@ static void i386_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
dc->prev_insn_end = tcg_last_op();
if (tb_cflags(dcbase->tb) & CF_PCREL) {
- pc_arg -= dc->cs_base;
pc_arg &= ~TARGET_PAGE_MASK;
}
tcg_gen_insn_start(pc_arg, dc->cc_op);
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] target/i386: pcrel: store low bits of physical address in data[0]
@ 2024-01-17 15:53 Paolo Bonzini
0 siblings, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2024-01-17 15:53 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-stable, Mark Cave-Ayland, Richard Henderson
For PC-relative translation blocks, env->eip changes during the
execution of a translation block, Therefore, QEMU must be able to
recover an instruction's PC just from the TranslationBlock struct and
the instruction data with. Because a TB will not span two pages, QEMU
stores all the low bits of EIP in the instruction data and replaces them
in x86_restore_state_to_opc. Bits 12 and higher (which may vary between
executions of a PCREL TB, since these only use the physical address in
the hash key) are kept unmodified from env->eip. The assumption is that
these bits of EIP, unlike bits 0-11, will not change as the translation
block executes.
Unfortunately, this is incorrect when the CS base is not aligned to a page.
Then the linear address of the instructions (i.e. the one with the
CS base addred) indeed will never span two pages, but bits 12+ of EIP
can actually change. For example, if CS base is 0x80262200 and EIP =
0x6FF4, the first instruction in the translation block will be at linear
address 0x802691F4. Even a very small TB will cross to EIP = 0x7xxx,
while the linear addresses will remain comfortably within a single page.
The fix is simply to use the low bits of the linear address for data[0],
since those don't change. Then x86_restore_state_to_opc uses tb->cs_base
to compute a temporary linear address (referring to some unknown
instruction in the TB, but with the correct values of bits 12 and higher);
the low bits are replaced with data[0], and EIP is obtained by subtracting
again the CS base.
Huge thanks to Mark Cave-Ayland for the image and initial debugging,
and to Gitlab user @kjliew for help with bisecting another occurrence
of (hopefully!) the same bug.
It should be relatively easy to write a testcase that performs MMIO on
an EIP with different bits 12+ than the first instruction of the translation
block; any help is welcome.
Fixes: e3a79e0e878 ("target/i386: Enable TARGET_TB_PCREL", 2022-10-11)
Cc: qemu-stable@nongnu.org
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: Richard Henderson <richard.henderson@linaro.org>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1964
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2012
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
target/i386/tcg/tcg-cpu.c | 20 ++++++++++++++++----
target/i386/tcg/translate.c | 1 -
2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
index 6e881e9e276..fa956d35ecd 100644
--- a/target/i386/tcg/tcg-cpu.c
+++ b/target/i386/tcg/tcg-cpu.c
@@ -68,14 +68,26 @@ static void x86_restore_state_to_opc(CPUState *cs,
X86CPU *cpu = X86_CPU(cs);
CPUX86State *env = &cpu->env;
int cc_op = data[1];
+ uint64_t new_pc;
if (tb_cflags(tb) & CF_PCREL) {
- env->eip = (env->eip & TARGET_PAGE_MASK) | data[0];
- } else if (tb->flags & HF_CS64_MASK) {
- env->eip = data[0];
+ /*
+ * To ensure that bits 0..11 do not change across the translation block,
+ * PC-relative TBs use linear addresses, i.e. addresses that have the CS
+ * base added, for data[0]. Add the CS base back before replacing the
+ * low bits, and subtract it below just like for non-PC-relative TBs.
+ */
+ uint64_t pc = env->eip + tb->cs_base;
+ new_pc = (pc & TARGET_PAGE_MASK) | data[0];
} else {
- env->eip = (uint32_t)(data[0] - tb->cs_base);
+ new_pc = data[0];
}
+ if (tb->flags & HF_CS64_MASK) {
+ env->eip = new_pc;
+ } else {
+ env->eip = (uint32_t)(new_pc - tb->cs_base);
+ }
+
if (cc_op != CC_OP_DYNAMIC) {
env->cc_op = cc_op;
}
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index cadf13bce43..e193c74472b 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -6996,7 +6996,6 @@ static void i386_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
dc->prev_insn_end = tcg_last_op();
if (tb_cflags(dcbase->tb) & CF_PCREL) {
- pc_arg -= dc->cs_base;
pc_arg &= ~TARGET_PAGE_MASK;
}
tcg_gen_insn_start(pc_arg, dc->cc_op);
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] target/i386: pcrel: store low bits of physical address in data[0]
2024-01-17 15:51 Paolo Bonzini
@ 2024-01-17 20:32 ` Richard Henderson
2024-01-17 21:13 ` Mark Cave-Ayland
1 sibling, 0 replies; 4+ messages in thread
From: Richard Henderson @ 2024-01-17 20:32 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: qemu-stable, Mark Cave-Ayland
On 1/18/24 02:51, Paolo Bonzini wrote:
> For PC-relative translation blocks, env->eip changes during the
> execution of a translation block, Therefore, QEMU must be able to
> recover an instruction's PC just from the TranslationBlock struct and
> the instruction data with. Because a TB will not span two pages, QEMU
> stores all the low bits of EIP in the instruction data and replaces them
> in x86_restore_state_to_opc. Bits 12 and higher (which may vary between
> executions of a PCREL TB, since these only use the physical address in
> the hash key) are kept unmodified from env->eip. The assumption is that
> these bits of EIP, unlike bits 0-11, will not change as the translation
> block executes.
>
> Unfortunately, this is incorrect when the CS base is not aligned to a page.
> Then the linear address of the instructions (i.e. the one with the
> CS base addred) indeed will never span two pages, but bits 12+ of EIP
added
> can actually change. For example, if CS base is 0x80262200 and EIP =
> 0x6FF4, the first instruction in the translation block will be at linear
> address 0x802691F4. Even a very small TB will cross to EIP = 0x7xxx,
> while the linear addresses will remain comfortably within a single page.
>
> The fix is simply to use the low bits of the linear address for data[0],
> since those don't change. Then x86_restore_state_to_opc uses tb->cs_base
> to compute a temporary linear address (referring to some unknown
> instruction in the TB, but with the correct values of bits 12 and higher);
> the low bits are replaced with data[0], and EIP is obtained by subtracting
> again the CS base.
>
> Huge thanks to Mark Cave-Ayland for the image and initial debugging,
> and to Gitlab user @kjliew for help with bisecting another occurrence
> of (hopefully!) the same bug.
>
> It should be relatively easy to write a testcase that performs MMIO on
> an EIP with different bits 12+ than the first instruction of the translation
> block; any help is welcome.
>
> Fixes: e3a79e0e878 ("target/i386: Enable TARGET_TB_PCREL", 2022-10-11)
> Cc: qemu-stable@nongnu.org
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1964
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2012
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> target/i386/tcg/tcg-cpu.c | 20 ++++++++++++++++----
> target/i386/tcg/translate.c | 1 -
> 2 files changed, 16 insertions(+), 5 deletions(-)
Wow, that is subtle.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] target/i386: pcrel: store low bits of physical address in data[0]
2024-01-17 15:51 Paolo Bonzini
2024-01-17 20:32 ` Richard Henderson
@ 2024-01-17 21:13 ` Mark Cave-Ayland
1 sibling, 0 replies; 4+ messages in thread
From: Mark Cave-Ayland @ 2024-01-17 21:13 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: qemu-stable, Richard Henderson
On 17/01/2024 15:51, Paolo Bonzini wrote:
> For PC-relative translation blocks, env->eip changes during the
> execution of a translation block, Therefore, QEMU must be able to
> recover an instruction's PC just from the TranslationBlock struct and
> the instruction data with. Because a TB will not span two pages, QEMU
> stores all the low bits of EIP in the instruction data and replaces them
> in x86_restore_state_to_opc. Bits 12 and higher (which may vary between
> executions of a PCREL TB, since these only use the physical address in
> the hash key) are kept unmodified from env->eip. The assumption is that
> these bits of EIP, unlike bits 0-11, will not change as the translation
> block executes.
>
> Unfortunately, this is incorrect when the CS base is not aligned to a page.
> Then the linear address of the instructions (i.e. the one with the
> CS base addred) indeed will never span two pages, but bits 12+ of EIP
added
> can actually change. For example, if CS base is 0x80262200 and EIP =
> 0x6FF4, the first instruction in the translation block will be at linear
> address 0x802691F4. Even a very small TB will cross to EIP = 0x7xxx,
> while the linear addresses will remain comfortably within a single page.
>
> The fix is simply to use the low bits of the linear address for data[0],
> since those don't change. Then x86_restore_state_to_opc uses tb->cs_base
> to compute a temporary linear address (referring to some unknown
> instruction in the TB, but with the correct values of bits 12 and higher);
> the low bits are replaced with data[0], and EIP is obtained by subtracting
> again the CS base.
>
> Huge thanks to Mark Cave-Ayland for the image and initial debugging,
> and to Gitlab user @kjliew for help with bisecting another occurrence
> of (hopefully!) the same bug.
>
> It should be relatively easy to write a testcase that performs MMIO on
> an EIP with different bits 12+ than the first instruction of the translation
> block; any help is welcome.
>
> Fixes: e3a79e0e878 ("target/i386: Enable TARGET_TB_PCREL", 2022-10-11)
> Cc: qemu-stable@nongnu.org
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1964
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2012
And also:
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1759
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> target/i386/tcg/tcg-cpu.c | 20 ++++++++++++++++----
> target/i386/tcg/translate.c | 1 -
> 2 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
> index 6e881e9e276..fa956d35ecd 100644
> --- a/target/i386/tcg/tcg-cpu.c
> +++ b/target/i386/tcg/tcg-cpu.c
> @@ -68,14 +68,26 @@ static void x86_restore_state_to_opc(CPUState *cs,
> X86CPU *cpu = X86_CPU(cs);
> CPUX86State *env = &cpu->env;
> int cc_op = data[1];
> + uint64_t new_pc;
>
> if (tb_cflags(tb) & CF_PCREL) {
> - env->eip = (env->eip & TARGET_PAGE_MASK) | data[0];
> - } else if (tb->flags & HF_CS64_MASK) {
> - env->eip = data[0];
> + /*
> + * To ensure that bits 0..11 do not change across the translation block,
> + * PC-relative TBs use linear addresses, i.e. addresses that have the CS
> + * base added, for data[0]. Add the CS base back before replacing the
> + * low bits, and subtract it below just like for non-PC-relative TBs.
> + */
> + uint64_t pc = env->eip + tb->cs_base;
> + new_pc = (pc & TARGET_PAGE_MASK) | data[0];
> } else {
> - env->eip = (uint32_t)(data[0] - tb->cs_base);
> + new_pc = data[0];
> }
> + if (tb->flags & HF_CS64_MASK) {
> + env->eip = new_pc;
> + } else {
> + env->eip = (uint32_t)(new_pc - tb->cs_base);
> + }
> +
> if (cc_op != CC_OP_DYNAMIC) {
> env->cc_op = cc_op;
> }
> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> index cadf13bce43..e193c74472b 100644
> --- a/target/i386/tcg/translate.c
> +++ b/target/i386/tcg/translate.c
> @@ -6996,7 +6996,6 @@ static void i386_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
>
> dc->prev_insn_end = tcg_last_op();
> if (tb_cflags(dcbase->tb) & CF_PCREL) {
> - pc_arg -= dc->cs_base;
> pc_arg &= ~TARGET_PAGE_MASK;
> }
> tcg_gen_insn_start(pc_arg, dc->cc_op);
Many thanks for coming up with the fix for this :) My test case now works fine, so:
Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
ATB,
Mark.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-01-17 21:14 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-17 15:53 [PATCH] target/i386: pcrel: store low bits of physical address in data[0] Paolo Bonzini
-- strict thread matches above, loose matches on Subject: below --
2024-01-17 15:51 Paolo Bonzini
2024-01-17 20:32 ` Richard Henderson
2024-01-17 21:13 ` Mark Cave-Ayland
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).