Re: [PATCH 07/22] target/i386: reimplement check for validity of LOCK prefix

[Richard Henderson]

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);