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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.03.17 at 17:58, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

I can do that, as you think it's better to read (which I'm not sure of).

> 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.

Well, that would leave the need to store the base address of the
stub somewhere (requiring to keep the very #ifdef-ary you disliked
in an earlier patch). I'd rather keep it the way it is.

Jan


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