| Message ID | 20231212172510.103305-1-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [for-8.2?] target/i386: Fix 32-bit wrapping of pc/eip computation | expand |
I am waiting for review comments before tagging v8.2.0-rc4. There are already other patches queued for -rc4 so we definitely need another week before the final release anyway. Stefan
On Tue, Dec 12, 2023 at 6:25 PM Richard Henderson <richard.henderson@linaro.org> wrote: > > In 32-bit mode, pc = eip + cs_base is also 32-bit, and must wrap. > Failure to do so results in incorrect memory exceptions to the guest. > Before 732d548732ed, this was implicitly done via truncation to > target_ulong but only in qemu-system-i386, not qemu-system-x86_64. > > To fix this, we must add conditional zero-extensions. > Since we have to test for 32 vs 64-bit anyway, note that cs_base > is always zero in 64-bit mode. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2022 > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > > This may be too late for 8.2; if not, then 8.2.1 and 8.1.next. > I think I have found all forms of pc <-> eip, but another set > of eyes would be appreciated. Looks good, but perhaps you could also squash the following? diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c index 2c6a12c8350..83ee89579b8 100644 --- a/target/i386/tcg/tcg-cpu.c +++ b/target/i386/tcg/tcg-cpu.c @@ -52,7 +52,11 @@ static void x86_cpu_synchronize_from_tb(CPUState *cs, /* The instruction pointer is always up to date with CF_PCREL. */ if (!(tb_cflags(tb) & CF_PCREL)) { CPUX86State *env = cpu_env(cs); - env->eip = tb->pc - tb->cs_base; + if (tb->flags & HF_CS64_MASK) { + env->eip = tb->pc; + } else { + env->eip = (uint32_t) (tb->pc - tb->cs_base); + } } } It wouldn't be the same bug as 2022 (it wouldn't be new with the vaddr change) so it's okay to sort out this extra case after release. Paolo
On 12/12/23 13:08, Paolo Bonzini wrote: > On Tue, Dec 12, 2023 at 6:25 PM Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> In 32-bit mode, pc = eip + cs_base is also 32-bit, and must wrap. >> Failure to do so results in incorrect memory exceptions to the guest. >> Before 732d548732ed, this was implicitly done via truncation to >> target_ulong but only in qemu-system-i386, not qemu-system-x86_64. >> >> To fix this, we must add conditional zero-extensions. >> Since we have to test for 32 vs 64-bit anyway, note that cs_base >> is always zero in 64-bit mode. >> >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2022 >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> >> This may be too late for 8.2; if not, then 8.2.1 and 8.1.next. >> I think I have found all forms of pc <-> eip, but another set >> of eyes would be appreciated. > > Looks good, but perhaps you could also squash the following? > > diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c > index 2c6a12c8350..83ee89579b8 100644 > --- a/target/i386/tcg/tcg-cpu.c > +++ b/target/i386/tcg/tcg-cpu.c > @@ -52,7 +52,11 @@ static void x86_cpu_synchronize_from_tb(CPUState *cs, > /* The instruction pointer is always up to date with CF_PCREL. */ > if (!(tb_cflags(tb) & CF_PCREL)) { > CPUX86State *env = cpu_env(cs); > - env->eip = tb->pc - tb->cs_base; > + if (tb->flags & HF_CS64_MASK) { > + env->eip = tb->pc; > + } else { > + env->eip = (uint32_t) (tb->pc - tb->cs_base); > + } > } > } > > > It wouldn't be the same bug as 2022 (it wouldn't be new with the vaddr > change) so it's okay to sort out this extra case after release. Good catch, I'll squash it. Thanks. r~
On Tue, Dec 12, 2023 at 10:22 PM Richard Henderson <richard.henderson@linaro.org> wrote: > > Looks good, but perhaps you could also squash the following? > > > > diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c > > index 2c6a12c8350..83ee89579b8 100644 > > --- a/target/i386/tcg/tcg-cpu.c > > +++ b/target/i386/tcg/tcg-cpu.c > > @@ -52,7 +52,11 @@ static void x86_cpu_synchronize_from_tb(CPUState *cs, > > /* The instruction pointer is always up to date with CF_PCREL. */ > > if (!(tb_cflags(tb) & CF_PCREL)) { > > CPUX86State *env = cpu_env(cs); > > - env->eip = tb->pc - tb->cs_base; > > + if (tb->flags & HF_CS64_MASK) { > > + env->eip = tb->pc; > > + } else { > > + env->eip = (uint32_t) (tb->pc - tb->cs_base); > > + } > > } > > } > > > > > > It wouldn't be the same bug as 2022 (it wouldn't be new with the vaddr > > change) so it's okay to sort out this extra case after release. > > Good catch, I'll squash it. Thanks. BTW, Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Paolo
12.12.2023 20:25, Richard Henderson: > In 32-bit mode, pc = eip + cs_base is also 32-bit, and must wrap. > Failure to do so results in incorrect memory exceptions to the guest. > Before 732d548732ed, this was implicitly done via truncation to > target_ulong but only in qemu-system-i386, not qemu-system-x86_64. > > To fix this, we must add conditional zero-extensions. > Since we have to test for 32 vs 64-bit anyway, note that cs_base > is always zero in 64-bit mode. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2022 > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > > This may be too late for 8.2; if not, then 8.2.1 and 8.1.next. > I think I have found all forms of pc <-> eip, but another set > of eyes would be appreciated. This change breaks trivial 4M edk2 boot - both in 8.2.0 and in 8.1.4 (which also has this commit now). qemu-system-x86_64 -machine q35 -no-user-config -nodefaults -display none \ -serial stdio \ -drive file=/usr/share/OVMF/OVMF_CODE_4M.secboot.fd,if=pflash,format=raw,readonly=on \ -drive file=/usr/share/OVMF/OVMF_VARS_4M.ms.fd,if=pflash,format=raw,snapshot=on After this change, nothing is printed on the serial console anymore (or in vga, whatever). Before that commit, usual edk2 boot sequence is seen. Nothing has changed with the 2M variant though. /mjt
On 12/25/23 07:49, Michael Tokarev wrote: > 12.12.2023 20:25, Richard Henderson: >> In 32-bit mode, pc = eip + cs_base is also 32-bit, and must wrap. >> Failure to do so results in incorrect memory exceptions to the guest. >> Before 732d548732ed, this was implicitly done via truncation to >> target_ulong but only in qemu-system-i386, not qemu-system-x86_64. >> >> To fix this, we must add conditional zero-extensions. >> Since we have to test for 32 vs 64-bit anyway, note that cs_base >> is always zero in 64-bit mode. >> >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2022 >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> >> This may be too late for 8.2; if not, then 8.2.1 and 8.1.next. >> I think I have found all forms of pc <-> eip, but another set >> of eyes would be appreciated. > > This change breaks trivial 4M edk2 boot - both in 8.2.0 and in > 8.1.4 (which also has this commit now). > > qemu-system-x86_64 -machine q35 -no-user-config -nodefaults -display none \ > -serial stdio \ > -drive file=/usr/share/OVMF/OVMF_CODE_4M.secboot.fd,if=pflash,format=raw,readonly=on \ > -drive file=/usr/share/OVMF/OVMF_VARS_4M.ms.fd,if=pflash,format=raw,snapshot=on > > After this change, nothing is printed on the serial console anymore > (or in vga, whatever). Before that commit, usual edk2 boot sequence > is seen. > > Nothing has changed with the 2M variant though. Ack. Looking at it.... r~
diff --git a/target/i386/cpu.h b/target/i386/cpu.h index cd2e295bd6..ef987f344c 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -2324,10 +2324,15 @@ static inline int cpu_mmu_index_kernel(CPUX86State *env) static inline void cpu_get_tb_cpu_state(CPUX86State *env, vaddr *pc, uint64_t *cs_base, uint32_t *flags) { - *cs_base = env->segs[R_CS].base; - *pc = *cs_base + env->eip; *flags = env->hflags | (env->eflags & (IOPL_MASK | TF_MASK | RF_MASK | VM_MASK | AC_MASK)); + if (env->hflags & HF_CS64_MASK) { + *cs_base = 0; + *pc = env->eip; + } else { + *cs_base = env->segs[R_CS].base; + *pc = (uint32_t)(*cs_base + env->eip); + } } void do_cpu_init(X86CPU *cpu); diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c index 2c6a12c835..eb05a69f79 100644 --- a/target/i386/tcg/tcg-cpu.c +++ b/target/i386/tcg/tcg-cpu.c @@ -66,8 +66,10 @@ static void x86_restore_state_to_opc(CPUState *cs, 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]; } else { - env->eip = data[0] - tb->cs_base; + env->eip = (uint32_t)(data[0] - 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 587d88692a..037bc47e7c 100644 --- a/target/i386/tcg/translate.c +++ b/target/i386/tcg/translate.c @@ -552,8 +552,10 @@ static void gen_update_eip_cur(DisasContext *s) assert(s->pc_save != -1); if (tb_cflags(s->base.tb) & CF_PCREL) { tcg_gen_addi_tl(cpu_eip, cpu_eip, s->base.pc_next - s->pc_save); + } else if (CODE64(s)) { + tcg_gen_movi_tl(cpu_eip, s->base.pc_next); } else { - tcg_gen_movi_tl(cpu_eip, s->base.pc_next - s->cs_base); + tcg_gen_movi_tl(cpu_eip, (uint32_t)(s->base.pc_next - s->cs_base)); } s->pc_save = s->base.pc_next; } @@ -563,8 +565,10 @@ static void gen_update_eip_next(DisasContext *s) assert(s->pc_save != -1); if (tb_cflags(s->base.tb) & CF_PCREL) { tcg_gen_addi_tl(cpu_eip, cpu_eip, s->pc - s->pc_save); + } else if (CODE64(s)) { + tcg_gen_movi_tl(cpu_eip, s->base.pc_next); } else { - tcg_gen_movi_tl(cpu_eip, s->pc - s->cs_base); + tcg_gen_movi_tl(cpu_eip, (uint32_t)(s->base.pc_next - s->cs_base)); } s->pc_save = s->pc; } @@ -610,8 +614,10 @@ static TCGv eip_next_tl(DisasContext *s) TCGv ret = tcg_temp_new(); tcg_gen_addi_tl(ret, cpu_eip, s->pc - s->pc_save); return ret; + } else if (CODE64(s)) { + return tcg_constant_tl(s->pc); } else { - return tcg_constant_tl(s->pc - s->cs_base); + return tcg_constant_tl((uint32_t)(s->pc - s->cs_base)); } } @@ -622,8 +628,10 @@ static TCGv eip_cur_tl(DisasContext *s) TCGv ret = tcg_temp_new(); tcg_gen_addi_tl(ret, cpu_eip, s->base.pc_next - s->pc_save); return ret; + } else if (CODE64(s)) { + return tcg_constant_tl(s->base.pc_next); } else { - return tcg_constant_tl(s->base.pc_next - s->cs_base); + return tcg_constant_tl((uint32_t)(s->base.pc_next - s->cs_base)); } } @@ -2837,6 +2845,10 @@ static void gen_jmp_rel(DisasContext *s, MemOp ot, int diff, int tb_num) } } new_eip &= mask; + new_pc = new_eip + s->cs_base; + if (!CODE64(s)) { + new_pc = (uint32_t)new_pc; + } gen_update_cc_op(s); set_cc_op(s, CC_OP_DYNAMIC); @@ -2854,8 +2866,7 @@ static void gen_jmp_rel(DisasContext *s, MemOp ot, int diff, int tb_num) } } - if (use_goto_tb && - translator_use_goto_tb(&s->base, new_eip + s->cs_base)) { + if (use_goto_tb && translator_use_goto_tb(&s->base, new_pc)) { /* jump to same page: we can use a direct jump */ tcg_gen_goto_tb(tb_num); if (!(tb_cflags(s->base.tb) & CF_PCREL)) {
In 32-bit mode, pc = eip + cs_base is also 32-bit, and must wrap. Failure to do so results in incorrect memory exceptions to the guest. Before 732d548732ed, this was implicitly done via truncation to target_ulong but only in qemu-system-i386, not qemu-system-x86_64. To fix this, we must add conditional zero-extensions. Since we have to test for 32 vs 64-bit anyway, note that cs_base is always zero in 64-bit mode. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2022 Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- This may be too late for 8.2; if not, then 8.2.1 and 8.1.next. I think I have found all forms of pc <-> eip, but another set of eyes would be appreciated. r~ --- target/i386/cpu.h | 9 +++++++-- target/i386/tcg/tcg-cpu.c | 4 +++- target/i386/tcg/translate.c | 23 +++++++++++++++++------ 3 files changed, 27 insertions(+), 9 deletions(-)