* [PATCH v4 0/1] cmpxchg and lock cmpxchg should not touch accumulator
[not found] <20220323013417.409858-1-lw945lw945.ref@yahoo.com>
@ 2022-03-23 1:34 ` Wei Li
2022-03-23 1:34 ` [PATCH v4 1/1] fix cmpxchg and lock cmpxchg instruction Wei Li
0 siblings, 1 reply; 3+ messages in thread
From: Wei Li @ 2022-03-23 1:34 UTC (permalink / raw)
To: pbonzini, richard.henderson, eduardo; +Cc: qemu-devel
This series fix a bug reported on issues 508.
The problem is cmpxchg and lock cmpxchg would touch accumulator when
the accumulator is equal to the TEMP.
Changes from v3
* Give a consolidated description of the problem fixed.
v3 link:
https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg05584.html
Changes from v2
* Give a better code struture to reduce more code duplication.
v2 link:
https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg05410.html
Changes from v1
* cmpxchg uses the lock cmpxchg path whenever mod != 3 to reduce code
duplication.
* lock cmpxchg uses movcond to replace branch.
* Combine the two patches into one patch because cmpxchg uses the lock
cmpxchg path.
v1 link:
https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg05023.html
Wei Li (1):
fix cmpxchg and lock cmpxchg instruction
target/i386/tcg/translate.c | 41 +++++++++++++++++--------------------
1 file changed, 19 insertions(+), 22 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH v4 1/1] fix cmpxchg and lock cmpxchg instruction
2022-03-23 1:34 ` [PATCH v4 0/1] cmpxchg and lock cmpxchg should not touch accumulator Wei Li
@ 2022-03-23 1:34 ` Wei Li
2022-03-29 12:56 ` 李玮
0 siblings, 1 reply; 3+ messages in thread
From: Wei Li @ 2022-03-23 1:34 UTC (permalink / raw)
To: pbonzini, richard.henderson, eduardo; +Cc: qemu-devel
This patch fixes a bug reported on issues #508.
The problem is cmpxchg and lock cmpxchg would touch accumulator when
the comparison is equal.
Signed-off-by: Wei Li <lw945lw945@yahoo.com>
---
target/i386/tcg/translate.c | 41 +++++++++++++++++--------------------
1 file changed, 19 insertions(+), 22 deletions(-)
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 2a94d33742..9677f9576b 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -5339,7 +5339,7 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
case 0x1b0:
case 0x1b1: /* cmpxchg Ev, Gv */
{
- TCGv oldv, newv, cmpv;
+ TCGv oldv, newv, cmpv, temp;
ot = mo_b_d(b, dflag);
modrm = x86_ldub_code(env, s);
@@ -5348,42 +5348,38 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
oldv = tcg_temp_new();
newv = tcg_temp_new();
cmpv = tcg_temp_new();
+ temp = tcg_temp_new();
gen_op_mov_v_reg(s, ot, newv, reg);
tcg_gen_mov_tl(cmpv, cpu_regs[R_EAX]);
+ tcg_gen_mov_tl(temp, cpu_regs[R_EAX]);
- if (s->prefix & PREFIX_LOCK) {
+ if ((s->prefix & PREFIX_LOCK) ||
+ (mod != 3)) {
+ /* Use the tcg_gen_atomic_cmpxchg_tl path whenever mod != 3.
+ While an unlocked cmpxchg need not be atomic, it is not
+ required to be non-atomic either. */
if (mod == 3) {
goto illegal_op;
}
gen_lea_modrm(env, s, modrm);
tcg_gen_atomic_cmpxchg_tl(oldv, s->A0, cmpv, newv,
s->mem_index, ot | MO_LE);
- gen_op_mov_reg_v(s, ot, R_EAX, oldv);
+ gen_extu(ot, oldv);
+ gen_extu(ot, cmpv);
} else {
- if (mod == 3) {
- rm = (modrm & 7) | REX_B(s);
- gen_op_mov_v_reg(s, ot, oldv, rm);
- } else {
- gen_lea_modrm(env, s, modrm);
- gen_op_ld_v(s, ot, oldv, s->A0);
- rm = 0; /* avoid warning */
- }
+ rm = (modrm & 7) | REX_B(s);
+ gen_op_mov_v_reg(s, ot, oldv, rm);
gen_extu(ot, oldv);
gen_extu(ot, cmpv);
/* store value = (old == cmp ? new : old); */
tcg_gen_movcond_tl(TCG_COND_EQ, newv, oldv, cmpv, newv, oldv);
- if (mod == 3) {
- gen_op_mov_reg_v(s, ot, R_EAX, oldv);
- gen_op_mov_reg_v(s, ot, rm, newv);
- } else {
- /* Perform an unconditional store cycle like physical cpu;
- must be before changing accumulator to ensure
- idempotency if the store faults and the instruction
- is restarted */
- gen_op_st_v(s, ot, newv, s->A0);
- gen_op_mov_reg_v(s, ot, R_EAX, oldv);
- }
+ gen_op_mov_reg_v(s, ot, rm, newv);
}
+ /* Perform the merge into %al or %ax as required by ot. */
+ gen_op_mov_reg_v(s, ot, R_EAX, oldv);
+ /* Undo the entire modification to %rax if comparison equal. */
+ tcg_gen_movcond_tl(TCG_COND_EQ, cpu_regs[R_EAX], oldv, cmpv,
+ temp, cpu_regs[R_EAX]);
tcg_gen_mov_tl(cpu_cc_src, oldv);
tcg_gen_mov_tl(s->cc_srcT, cmpv);
tcg_gen_sub_tl(cpu_cc_dst, cmpv, oldv);
@@ -5391,6 +5387,7 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
tcg_temp_free(oldv);
tcg_temp_free(newv);
tcg_temp_free(cmpv);
+ tcg_temp_free(temp);
}
break;
case 0x1c7: /* cmpxchg8b */
--
2.30.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v4 1/1] fix cmpxchg and lock cmpxchg instruction
2022-03-23 1:34 ` [PATCH v4 1/1] fix cmpxchg and lock cmpxchg instruction Wei Li
@ 2022-03-29 12:56 ` 李玮
0 siblings, 0 replies; 3+ messages in thread
From: 李玮 @ 2022-03-29 12:56 UTC (permalink / raw)
To: Wei Li; +Cc: eduardo, pbonzini, richard.henderson, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4460 bytes --]
On Wed, Mar 23, 2022 at 9:37 AM Wei Li <lw945lw945@yahoo.com> wrote:
> This patch fixes a bug reported on issues #508.
> The problem is cmpxchg and lock cmpxchg would touch accumulator when
> the comparison is equal.
>
> Signed-off-by: Wei Li <lw945lw945@yahoo.com>
> ---
> target/i386/tcg/translate.c | 41 +++++++++++++++++--------------------
> 1 file changed, 19 insertions(+), 22 deletions(-)
>
> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> index 2a94d33742..9677f9576b 100644
> --- a/target/i386/tcg/translate.c
> +++ b/target/i386/tcg/translate.c
> @@ -5339,7 +5339,7 @@ static target_ulong disas_insn(DisasContext *s,
> CPUState *cpu)
> case 0x1b0:
> case 0x1b1: /* cmpxchg Ev, Gv */
> {
> - TCGv oldv, newv, cmpv;
> + TCGv oldv, newv, cmpv, temp;
>
> ot = mo_b_d(b, dflag);
> modrm = x86_ldub_code(env, s);
> @@ -5348,42 +5348,38 @@ static target_ulong disas_insn(DisasContext *s,
> CPUState *cpu)
> oldv = tcg_temp_new();
> newv = tcg_temp_new();
> cmpv = tcg_temp_new();
> + temp = tcg_temp_new();
> gen_op_mov_v_reg(s, ot, newv, reg);
> tcg_gen_mov_tl(cmpv, cpu_regs[R_EAX]);
> + tcg_gen_mov_tl(temp, cpu_regs[R_EAX]);
>
> - if (s->prefix & PREFIX_LOCK) {
> + if ((s->prefix & PREFIX_LOCK) ||
> + (mod != 3)) {
> + /* Use the tcg_gen_atomic_cmpxchg_tl path whenever mod !=
> 3.
> + While an unlocked cmpxchg need not be atomic, it is not
> + required to be non-atomic either. */
> if (mod == 3) {
> goto illegal_op;
> }
> gen_lea_modrm(env, s, modrm);
> tcg_gen_atomic_cmpxchg_tl(oldv, s->A0, cmpv, newv,
> s->mem_index, ot | MO_LE);
> - gen_op_mov_reg_v(s, ot, R_EAX, oldv);
> + gen_extu(ot, oldv);
> + gen_extu(ot, cmpv);
> } else {
> - if (mod == 3) {
> - rm = (modrm & 7) | REX_B(s);
> - gen_op_mov_v_reg(s, ot, oldv, rm);
> - } else {
> - gen_lea_modrm(env, s, modrm);
> - gen_op_ld_v(s, ot, oldv, s->A0);
> - rm = 0; /* avoid warning */
> - }
> + rm = (modrm & 7) | REX_B(s);
> + gen_op_mov_v_reg(s, ot, oldv, rm);
> gen_extu(ot, oldv);
> gen_extu(ot, cmpv);
> /* store value = (old == cmp ? new : old); */
> tcg_gen_movcond_tl(TCG_COND_EQ, newv, oldv, cmpv, newv,
> oldv);
> - if (mod == 3) {
> - gen_op_mov_reg_v(s, ot, R_EAX, oldv);
> - gen_op_mov_reg_v(s, ot, rm, newv);
> - } else {
> - /* Perform an unconditional store cycle like physical
> cpu;
> - must be before changing accumulator to ensure
> - idempotency if the store faults and the instruction
> - is restarted */
> - gen_op_st_v(s, ot, newv, s->A0);
> - gen_op_mov_reg_v(s, ot, R_EAX, oldv);
> - }
> + gen_op_mov_reg_v(s, ot, rm, newv);
> }
> + /* Perform the merge into %al or %ax as required by ot. */
> + gen_op_mov_reg_v(s, ot, R_EAX, oldv);
> + /* Undo the entire modification to %rax if comparison equal.
> */
> + tcg_gen_movcond_tl(TCG_COND_EQ, cpu_regs[R_EAX], oldv, cmpv,
> + temp, cpu_regs[R_EAX]);
> tcg_gen_mov_tl(cpu_cc_src, oldv);
> tcg_gen_mov_tl(s->cc_srcT, cmpv);
> tcg_gen_sub_tl(cpu_cc_dst, cmpv, oldv);
> @@ -5391,6 +5387,7 @@ static target_ulong disas_insn(DisasContext *s,
> CPUState *cpu)
> tcg_temp_free(oldv);
> tcg_temp_free(newv);
> tcg_temp_free(cmpv);
> + tcg_temp_free(temp);
> }
> break;
> case 0x1c7: /* cmpxchg8b */
> --
> 2.30.2
>
>
>
Is this patch OK? or I forgot to add a reviewed-by tags by my hand?
This is my first time participating in this process.
Please let me know if anything is wrong.
Thanks a lot.
Wei Li
[-- Attachment #2: Type: text/html, Size: 5889 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-03-29 12:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20220323013417.409858-1-lw945lw945.ref@yahoo.com>
2022-03-23 1:34 ` [PATCH v4 0/1] cmpxchg and lock cmpxchg should not touch accumulator Wei Li
2022-03-23 1:34 ` [PATCH v4 1/1] fix cmpxchg and lock cmpxchg instruction Wei Li
2022-03-29 12:56 ` 李玮
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.