Re: [PATCH v2 1/3] x86/emulate: add support for {, v}movq xmm, xmm/m64

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 18/07/16 15:30, Mihai Donțu wrote:
> Signed-off-by: Mihai Donțu <mdontu@bitdefender.com>
> ---
> Changed since v1:
>  * added a test for vmovq
>  * made the tests depend on SSE and AVX, respectively
>  * added emulator support for vmovq (0xd6 forces the operand size to
>    64bit)
> ---
>  tools/tests/x86_emulator/test_x86_emulator.c | 44 ++++++++++++++++++++++++++++
>  xen/arch/x86/x86_emulate/x86_emulate.c       |  9 +++---
>  2 files changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/tools/tests/x86_emulator/test_x86_emulator.c b/tools/tests/x86_emulator/test_x86_emulator.c
> index c7f572a..8994149 100644
> --- a/tools/tests/x86_emulator/test_x86_emulator.c
> +++ b/tools/tests/x86_emulator/test_x86_emulator.c
> @@ -697,6 +697,50 @@ int main(int argc, char **argv)
>      else
>          printf("skipped\n");
>  
> +    printf("%-40s", "Testing movq %%xmm0,32(%%eax)...");
> +    if ( stack_exec && cpu_has_sse )
> +    {
> +        decl_insn(movq_to_mem2);
> +
> +        asm volatile ( "pcmpgtb %%xmm0, %%xmm0\n"
> +                       put_insn(movq_to_mem2, "movq %%xmm0, 32(%%eax)")
> +                       :: );
> +
> +        *((unsigned long *)res + 4) = 0xbdbdbdbdbdbdbdbd;
> +        set_insn(movq_to_mem2);
> +        regs.eax = (unsigned long)res;
> +        rc = x86_emulate(&ctxt, &emulops);
> +        if ( rc != X86EMUL_OKAY || !check_eip(movq_to_mem2) )
> +            goto fail;
> +        if ( *((unsigned long *)res + 4) )
> +            goto fail;
> +        printf("okay\n");
> +    }
> +    else
> +        printf("skipped\n");
> +
> +    printf("%-40s", "Testing vmovq %%xmm1,32(%%eax)...");
> +    if ( stack_exec && cpu_has_avx )
> +    {
> +        decl_insn(vmovq_to_mem);
> +
> +        asm volatile ( "pcmpgtb %%xmm1, %%xmm1\n"
> +                       put_insn(vmovq_to_mem, "vmovq %%xmm1, 32(%%eax)")
> +                       :: );
> +
> +        *((unsigned long *)res + 4) = 0xbdbdbdbdbdbdbdbd;
> +        set_insn(vmovq_to_mem);
> +        regs.eax = (unsigned long)res;
> +        rc = x86_emulate(&ctxt, &emulops);
> +        if ( rc != X86EMUL_OKAY || !check_eip(vmovq_to_mem) )
> +            goto fail;
> +        if ( *((unsigned long *)res + 4) )
> +            goto fail;
> +        printf("okay\n");
> +    }
> +    else
> +        printf("skipped\n");
> +
>      printf("%-40s", "Testing movdqu %xmm2,(%ecx)...");
>      if ( stack_exec && cpu_has_sse2 )
>      {
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
> index fe594ba..0301235 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -245,7 +245,7 @@ static uint8_t twobyte_table[256] = {
>      ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
>      ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
>      /* 0xD0 - 0xDF */
> -    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> +    0, 0, 0, 0, 0, 0, ImplicitOps|ModRM, 0, 0, 0, 0, 0, 0, 0, 0, 0,
>      /* 0xE0 - 0xEF */
>      0, 0, 0, 0, 0, 0, 0, ImplicitOps|ModRM, 0, 0, 0, 0, 0, 0, 0, 0,
>      /* 0xF0 - 0xFF */
> @@ -4412,6 +4412,7 @@ x86_emulate(
>      case 0x7f: /* movq mm,mm/m64 */
>                 /* {,v}movdq{a,u} xmm,xmm/m128 */
>                 /* vmovdq{a,u} ymm,ymm/m256 */
> +    case 0xd6: /* {,v}movq xmm,xmm/m64 */
>      {
>          uint8_t *buf = get_stub(stub);
>          struct fpu_insn_ctxt fic = { .insn_bytes = 5 };
> @@ -4429,9 +4430,9 @@ x86_emulate(
>              case vex_66:
>              case vex_f3:
>                  host_and_vcpu_must_have(sse2);
> -                buf[0] = 0x66; /* movdqa */
> +                buf[0] = 0x66; /* SSE */
>                  get_fpu(X86EMUL_FPU_xmm, &fic);
> -                ea.bytes = 16;
> +                ea.bytes = (b == 0xd6 ? 8 : 16);
>                  break;
>              case vex_none:
>                  if ( b != 0xe7 )
> @@ -4451,7 +4452,7 @@ x86_emulate(
>                      ((vex.pfx != vex_66) && (vex.pfx != vex_f3)));
>              host_and_vcpu_must_have(avx);
>              get_fpu(X86EMUL_FPU_ymm, &fic);
> -            ea.bytes = 16 << vex.l;
> +            ea.bytes = (b == 0xd6 ? 8 : 16 << vex.l);

It would be helpful to bracket (16 << vex.l) to make it more obvious as
to what the operator precedent is.

Otherwise,

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

I can fix this up on commit if there are no other issues in the series.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel