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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.03.17 at 14:17, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 28/02/17 12:49, Jan Beulich wrote:
>> --- 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." ?

Sure.

>> @@ -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? 

The code portion actually invoking the stub should generally do this.
All it needs for this is fic.insn_bytes to be set correctly.

> This seems rather inconsistent at the moment.

Does it? At least in this patch I can't spot an inconsistency.

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

No, it's not stale: In the hypervisor we can't use get_stub() a
second time, or else we'll invoke map_domain_page() a second
time, discarding (and hence leaking) the result of the earlier
one. And in the harness using get_stub() is the cleanest way
to get hold of the pointer again. I've considered and tried
several variants, but I couldn't come up with an approach not
needing any #ifdef - if you see a way, let me know.

Jan


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