On 12/23/23 05:15, Paolo Bonzini wrote:
> The previous check erroneously allowed CMP to be modified with LOCK.
> Instead, tag explicitly the instructions that do support LOCK.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> target/i386/tcg/decode-new.c.inc | 17 ++++++++++-------
> target/i386/tcg/decode-new.h | 3 +++
> target/i386/tcg/emit.c.inc | 5 -----
> 3 files changed, 13 insertions(+), 12 deletions(-)
It's hard to see how this fits together, because there don't seem to be any uses of
X86_SPECIAL_{Locked,HasLock} yet.
But the illegal test in disas_insn_new looks more plausibly correct than the one in
decode_modrm, so
Acked-by: Richard Henderson <richard.henderson@linaro.org>
r~
>
> diff --git a/target/i386/tcg/decode-new.c.inc b/target/i386/tcg/decode-new.c.inc
> index 232c6a45c96..5eb2e9d0224 100644
> --- a/target/i386/tcg/decode-new.c.inc
> +++ b/target/i386/tcg/decode-new.c.inc
> @@ -151,6 +151,7 @@
>
> #define cpuid(feat) .cpuid = X86_FEAT_##feat,
> #define xchg .special = X86_SPECIAL_Locked,
> +#define lock .special = X86_SPECIAL_HasLock,
> #define mmx .special = X86_SPECIAL_MMX,
> #define zext0 .special = X86_SPECIAL_ZExtOp0,
> #define zext2 .special = X86_SPECIAL_ZExtOp2,
> @@ -1103,10 +1104,6 @@ static int decode_modrm(DisasContext *s, CPUX86State *env, X86DecodedInsn *decod
> {
> int modrm = get_modrm(s, env);
> if ((modrm >> 6) == 3) {
> - if (s->prefix & PREFIX_LOCK) {
> - decode->e.gen = gen_illegal;
> - return 0xff;
> - }
> op->n = (modrm & 7);
> if (type != X86_TYPE_Q && type != X86_TYPE_N) {
> op->n |= REX_B(s);
> @@ -1881,6 +1878,9 @@ static void disas_insn_new(DisasContext *s, CPUState *cpu, int b)
> if (decode.op[0].has_ea) {
> s->prefix |= PREFIX_LOCK;
> }
> + decode.e.special = X86_SPECIAL_HasLock;
> + /* fallthrough */
> + case X86_SPECIAL_HasLock:
> break;
>
> case X86_SPECIAL_ZExtOp0:
> @@ -1909,6 +1909,12 @@ static void disas_insn_new(DisasContext *s, CPUState *cpu, int b)
> break;
> }
>
> + if (s->prefix & PREFIX_LOCK) {
> + if (decode.e.special != X86_SPECIAL_HasLock || !decode.op[0].has_ea) {
> + goto illegal_op;
> + }
> + }
> +
> if (!validate_vex(s, &decode)) {
> return;
> }
> @@ -1952,9 +1958,6 @@ static void disas_insn_new(DisasContext *s, CPUState *cpu, int b)
> gen_load_ea(s, &decode.mem, decode.e.vex_class == 12);
> }
> if (s->prefix & PREFIX_LOCK) {
> - if (decode.op[0].unit != X86_OP_INT || !decode.op[0].has_ea) {
> - goto illegal_op;
> - }
> gen_load(s, &decode, 2, s->T1);
> decode.e.gen(s, env, &decode);
> } else {
> diff --git a/target/i386/tcg/decode-new.h b/target/i386/tcg/decode-new.h
> index e6c904a3192..611bfddd957 100644
> --- a/target/i386/tcg/decode-new.h
> +++ b/target/i386/tcg/decode-new.h
> @@ -158,6 +158,9 @@ typedef enum X86InsnCheck {
> typedef enum X86InsnSpecial {
> X86_SPECIAL_None,
>
> + /* Accepts LOCK prefix; LOCKed operations do not load or writeback operand 0 */
> + X86_SPECIAL_HasLock,
> +
> /* Always locked if it has a memory operand (XCHG) */
> X86_SPECIAL_Locked,
>
> diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc
> index d444d83e534..98c4c9569ef 100644
> --- a/target/i386/tcg/emit.c.inc
> +++ b/target/i386/tcg/emit.c.inc
> @@ -55,11 +55,6 @@ static void gen_NM_exception(DisasContext *s)
> gen_exception(s, EXCP07_PREX);
> }
>
> -static void gen_illegal(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
> -{
> - gen_illegal_opcode(s);
> -}
> -
> static void gen_load_ea(DisasContext *s, AddressParts *mem, bool is_vsib)
> {
> TCGv ea = gen_lea_modrm_1(s, *mem, is_vsib);