Re: [Qemu-devel] [PATCH] target/i386: Fix handling of VEX prefixes

[Paolo Bonzini]

On 13/12/2017 12:19, Peter Maydell wrote:
> In commit e3af7c788b73a6495eb9d94992ef11f6ad6f3c56 we
> replaced direct calls to to cpu_ld*_code() with calls
> to the x86_ld*_code() wrappers which incorporate an
> advance of s->pc. Unfortunately we didn't notice that
> in one place the old code was deliberately not incrementing
> s->pc:
> 
> @@ -4501,7 +4528,7 @@ static target_ulong disas_insn(DisasContext *s, 
> CPUState *cpu)
>              static const int pp_prefix[4] = {
>                  0, PREFIX_DATA, PREFIX_REPZ, PREFIX_REPNZ
>              };
> -            int vex3, vex2 = cpu_ldub_code(env, s->pc);
> +            int vex3, vex2 = x86_ldub_code(env, s);
> 
>              if (!CODE64(s) && (vex2 & 0xc0) != 0xc0) {
>                  /* 4.1.4.6: In 32-bit mode, bits [7:6] must be 11b,
> 
> This meant we were mishandling this set of instructions.
> Remove the manual advance of s->pc for the "is VEX" case
> (which is now done by x86_ldub_code()) and instead rewind
> PC in the case where we decide that this isn't really VEX.
> 
> Signed-off-by: Peter Maydell <address@hidden>
> Cc: address@hidden
> Reported-by: Alexandro Sanchez Bach <address@hidden>
> ---
> I checked the rest of the changes in e3af7c788b73a and
> I don't think we made this mistake anywhere else.
> ---
>  target/i386/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/i386/translate.c b/target/i386/translate.c
> index 088a9d9..ed5b69d 100644
> --- a/target/i386/translate.c
> +++ b/target/i386/translate.c
> @@ -4547,9 +4547,9 @@ static target_ulong disas_insn(DisasContext *s, 
> CPUState *cpu)
>              if (!CODE64(s) && (vex2 & 0xc0) != 0xc0) {
>                  /* 4.1.4.6: In 32-bit mode, bits [7:6] must be 11b,
>                     otherwise the instruction is LES or LDS.  */
> +                s->pc--; /* rewind the advance_pc() x86_ldub_code() did */
>                  break;
>              }
> -            s->pc++;
>  
>              /* 4.1.1-4.1.3: No preceding lock, 66, f2, f3, or rex prefixes. 
> */
>              if (prefixes & (PREFIX_REPZ | PREFIX_REPNZ
> 

Queued for 2.12, thanks.

Paolo