Re: [Xen-devel] [PATCH v4 12/17] x86emul: support SSE4.1 insns

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/02/17 12:56, Jan Beulich wrote:
> @@ -6951,6 +7040,97 @@ x86_emulate(
>          fic.insn_bytes = PFX_BYTES + 3;
>          break;
>  
> +    case X86EMUL_OPC_66(0x0f38, 0x20): /* pmovsxbw xmm/m64,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x21): /* pmovsxbd xmm/m32,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x22): /* pmovsxbq xmm/m16,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x23): /* pmovsxwd xmm/m64,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x24): /* pmovsxwq xmm/m32,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x25): /* pmovsxdq xmm/m64,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x30): /* pmovzxbw xmm/m64,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x31): /* pmovzxbd xmm/m32,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x32): /* pmovzxbq xmm/m16,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x33): /* pmovzxwd xmm/m64,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x34): /* pmovzxwq xmm/m32,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x35): /* pmovzxdq xmm/m64,xmm */
> +        op_bytes = 16 >> pmov_convert_delta[b & 7];
> +        /* fall through */
> +    case X86EMUL_OPC_66(0x0f38, 0x10): /* pblendvb XMM0,xmm/m128,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x14): /* blendvps XMM0,xmm/m128,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x15): /* blendvpd XMM0,xmm/m128,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x28): /* pmuldq xmm/m128,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x29): /* pcmpeqq xmm/m128,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x2b): /* packusdw xmm/m128,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x38): /* pminsb xmm/m128,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x39): /* pminsd xmm/m128,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x3a): /* pminub xmm/m128,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x3b): /* pminud xmm/m128,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x3c): /* pmaxsb xmm/m128,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x3d): /* pmaxsd xmm/m128,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x3e): /* pmaxub xmm/m128,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x3f): /* pmaxud xmm/m128,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x40): /* pmulld xmm/m128,xmm */
> +    case X86EMUL_OPC_66(0x0f38, 0x41): /* phminposuw xmm/m128,xmm */
> +        host_and_vcpu_must_have(sse4_1);
> +        goto simd_0f38_common;
> +
> +    case X86EMUL_OPC_66(0x0f38, 0x17):     /* ptest xmm/m128,xmm */
> +    case X86EMUL_OPC_VEX_66(0x0f38, 0x17): /* vptest {x,y}mm/mem,{x,y}mm */
> +        if ( vex.opcx == vex_none )
> +        {
> +            host_and_vcpu_must_have(sse4_1);
> +            get_fpu(X86EMUL_FPU_xmm, &fic);
> +        }
> +        else
> +        {
> +            generate_exception_if(vex.reg != 0xf, EXC_UD);
> +            host_and_vcpu_must_have(avx);
> +            get_fpu(X86EMUL_FPU_ymm, &fic);
> +        }
> +
> +        opc = init_prefixes(stub);
> +        if ( vex.opcx == vex_none )
> +            opc[0] = 0x38;
> +        opc[vex.opcx == vex_none] = b;
> +        opc[1 + (vex.opcx == vex_none)] = modrm;

This use of (vex.opcx == vex_none) for construction is very awkward to read.

How about:

if ( vex.opcx == vex_none )
{
    opc[0] = 0x38;
    opc++; /* Adjust for extra prefix. */
}

...

if ( vex.opcx == vex_none )
    opc--; /* Undo adjustment for extra prefix. */

which allows the rest of the opc[] setup to read like all the other
similar code.


In fact, thinking more about this, using a pointer-arithmatic-based
method of filling the stub would allow for the removal of
"fic.insn_bytes = PFX_BYTES + $X", and any chance of getting the count
wrong.

~Andrew

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