Re: [Xen-devel] [PATCH 6/6] x86emul: support MOVDIR{I,64B} insns

[Jan Beulich <jbeulich@suse.com>]

On 28.08.2019 13:51, Andrew Cooper wrote:
> On 28/08/2019 07:26, Jan Beulich wrote:
>> On 27.08.2019 18:04, Andrew Cooper wrote:
>>> On 01/07/2019 12:58, Jan Beulich wrote:
>>>> @@ -9896,6 +9902,32 @@ x86_emulate(
>>>>                                 : "0" ((uint32_t)src.val), "rm"
>>>> (_regs.edx) );
>>>>             break;
>>>>     +    case X86EMUL_OPC_66(0x0f38, 0xf8): /* movdir64b r,m512 */
>>>> +        vcpu_must_have(movdir64b);
>>>> +        generate_exception_if(ea.type != OP_MEM, EXC_UD);
>>>> +        src.val = truncate_ea(*dst.reg);
>>>> +        generate_exception_if(!is_aligned(x86_seg_es, src.val, 64,
>>>> ctxt, ops),
>>>> +                              EXC_GP, 0);
>>>> +        /* Ignore the non-temporal behavior for now. */
>>>> +        fail_if(!ops->write);
>>>> +        BUILD_BUG_ON(sizeof(*mmvalp) < 64);
>>>> +        if ( (rc = ops->read(ea.mem.seg, ea.mem.off, mmvalp, 64,
>>>> +                             ctxt)) != X86EMUL_OKAY ||
>>>> +             (rc = ops->write(x86_seg_es, src.val, mmvalp, 64,
>>>> +                              ctxt)) != X86EMUL_OKAY )
>>>> +            goto done;
>>>> +        state->simd_size = simd_none;
>>>> +        sfence = true;
>>>> +        break;
>>>> +
>>>> +    case X86EMUL_OPC(0x0f38, 0xf9): /* movdiri mem,r */
>>>> +        vcpu_must_have(movdiri);
>>>> +        generate_exception_if(dst.type != OP_MEM, EXC_UD);
>>>> +        /* Ignore the non-temporal behavior for now. */
>>>> +        dst.val = src.val;
>>>> +        sfence = true;
>>>> +        break;
>>>
>>> I'm not certain this gives the required atomicity.  AFAICT, it degrades
>>> into ops->write(), which can end up with bytewise writes.
>>>
>>> I think we need to map the destination and issue an explicit mov
>>> instruction.
>>
>> I don't think so, no - plain MOV has the same property (in particular
>> when not going through the cache), and also uses the ->write() hook.
>> It's the hook function that needs to behave properly for all of this
>> to be correct.
> 
> It only occurred to me after sending this email that plain MOV was
> broken as well.

Well, they're both in the same state, but it shouldn't be the core
emulator's job to establish a mapping. In fact hvmemul_write() already
calls hvmemul_map_linear_addr(). What's sub-optimal is its reliance on
memcpy()'s implementation (when there's a valid GFN->MFN mapping).

Jan

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