Re: [PATCH v4 01/17] x86emul: support most memory accessing MMX/SSE{, 2, 3} insns

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

On 28/02/17 12:49, Jan Beulich wrote:
> e 0x0f-escape
> space with memory operands. Not covered here are irregular moves,
> converts, and {,U}COMIS{S,D} (modifying EFLAGS).

Your email has text corruption in this paragraph, but the patch itself
looks ok.

> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -2602,13 +2698,53 @@ x86_decode(
>          ea.mem.off = truncate_ea(ea.mem.off);
>      }
>  
> -    /*
> -     * When prefix 66 has a meaning different from operand-size override,
> -     * operand size defaults to 4 and can't be overridden to 2.
> -     */
> -    if ( op_bytes == 2 &&
> -         (ctxt->opcode & X86EMUL_OPC_PFX_MASK) == X86EMUL_OPC_66(0, 0) )
> -        op_bytes = 4;

Can we have a comment here along the lines of:

"Simple op_bytes calculations.  More complicated cases use 0 and are
further decoded during execute." ?

> +    switch ( state->simd_size )
> +    {
> +    case simd_none:
> +        /*
> +         * When prefix 66 has a meaning different from operand-size override,
> +         * operand size defaults to 4 and can't be overridden to 2.
> +         */
> +        if ( op_bytes == 2 &&
> +             (ctxt->opcode & X86EMUL_OPC_PFX_MASK) == X86EMUL_OPC_66(0, 0) )
> +            op_bytes = 4;
> +        break;
> +
> +    case simd_packed_int:
> +        switch ( vex.pfx )
> +        {
> +        case vex_none: op_bytes = 8;           break;
> +        case vex_66:   op_bytes = 16 << vex.l; break;
> +        default:       op_bytes = 0;           break;
> +        }
> +        break;
> +
> +    case simd_single_fp:
> +        if ( vex.pfx & VEX_PREFIX_DOUBLE_MASK )
> +        {
> +            op_bytes = 0;
> +            break;
> +    case simd_packed_fp:
> +            if ( vex.pfx & VEX_PREFIX_SCALAR_MASK )
> +            {
> +                op_bytes = 0;
> +                break;
> +            }
> +        }
> +        /* fall through */
> +    case simd_any_fp:
> +        switch ( vex.pfx )
> +        {
> +        default:     op_bytes = 16 << vex.l; break;
> +        case vex_f3: op_bytes = 4;           break;
> +        case vex_f2: op_bytes = 8;           break;
> +        }
> +        break;
> +
> +    default:
> +        op_bytes = 0;
> +        break;
> +    }
>  
>   done:
>      return rc;
> @@ -5413,6 +5671,81 @@ x86_emulate(
>          break;
>      }
>  
> +    CASE_SIMD_PACKED_INT(0x0f, 0x70):    /* pshuf{w,d} $imm8,{,x}mm/mem,{,x}mm */
> +    case X86EMUL_OPC_VEX_66(0x0f, 0x70): /* vpshufd $imm8,{x,y}mm/mem,{x,y}mm */
> +    case X86EMUL_OPC_F3(0x0f, 0x70):     /* pshufhw $imm8,xmm/m128,xmm */
> +    case X86EMUL_OPC_VEX_F3(0x0f, 0x70): /* vpshufhw $imm8,{x,y}mm/mem,{x,y}mm */
> +    case X86EMUL_OPC_F2(0x0f, 0x70):     /* pshuflw $imm8,xmm/m128,xmm */
> +    case X86EMUL_OPC_VEX_F2(0x0f, 0x70): /* vpshuflw $imm8,{x,y}mm/mem,{x,y}mm */
> +        d = (d & ~SrcMask) | SrcMem | TwoOp;
> +        op_bytes = vex.pfx ? 16 << vex.l : 8;
> +    simd_0f_int_imm8:
> +        if ( vex.opcx != vex_none )
> +        {
> +            if ( vex.l )
> +                host_and_vcpu_must_have(avx2);
> +            else
> +            {
> +    simd_0f_imm8_avx:
> +                host_and_vcpu_must_have(avx);
> +            }
> +            get_fpu(X86EMUL_FPU_ymm, &fic);
> +        }
> +        else if ( vex.pfx )
> +        {
> +    simd_0f_imm8_sse2:
> +            vcpu_must_have(sse2);
> +            get_fpu(X86EMUL_FPU_xmm, &fic);
> +        }
> +        else
> +        {
> +            host_and_vcpu_must_have(mmx);
> +            vcpu_must_have(sse);
> +            get_fpu(X86EMUL_FPU_mmx, &fic);
> +        }
> +    simd_0f_imm8:
> +    {
> +        uint8_t *buf = get_stub(stub);
> +
> +        buf[0] = 0x3e;
> +        buf[1] = 0x3e;
> +        buf[2] = 0x0f;
> +        buf[3] = b;
> +        buf[4] = modrm;
> +        if ( ea.type == OP_MEM )
> +        {
> +            /* Convert memory operand to (%rAX). */
> +            rex_prefix &= ~REX_B;
> +            vex.b = 1;
> +            buf[4] &= 0x38;
> +        }
> +        buf[5] = imm1;
> +        fic.insn_bytes = 6;

What is the expectation with setting up the ret in the stub or not? 
This seems rather inconsistent at the moment.

> @@ -6159,6 +6551,76 @@ x86_emulate(
>          goto cannot_emulate;
>      }
>  
> +    if ( state->simd_size )
> +    {
> +#ifdef __XEN__
> +        uint8_t *buf = stub.ptr;
> +#else
> +        uint8_t *buf = get_stub(stub);
> +#endif

Is this stale?  Everywhere else is just get_stub() without any ifdefary.

~Andrew

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