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

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

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.

>
>>> --- a/tools/tests/x86_emulator/x86-emulate.c
>>> +++ b/tools/tests/x86_emulator/x86-emulate.c
>>> @@ -76,6 +76,8 @@ bool emul_test_init(void)
>>>        cp.feat.adx = true;
>>>        cp.feat.avx512pf = cp.feat.avx512f;
>>>        cp.feat.rdpid = true;
>>> +    cp.feat.movdiri = true;
>>> +    cp.feat.movdir64b = true;
>>>        cp.extd.clzero = true;
>>>           if ( cpu_has_xsave )
>>> @@ -137,15 +139,15 @@ int emul_test_cpuid(
>>>            res->c |= 1U << 22;
>>>           /*
>>> -     * The emulator doesn't itself use ADCX/ADOX/RDPID nor the S/G
>>> prefetch
>>> -     * insns, so we can always run the respective tests.
>>> +     * The emulator doesn't itself use ADCX/ADOX/RDPID/MOVDIR* nor
>>> the S/G
>>> +     * prefetch insns, so we can always run the respective tests.
>>>         */
>>>        if ( leaf == 7 && subleaf == 0 )
>>>        {
>>>            res->b |= (1U << 10) | (1U << 19);
>>>            if ( res->b & (1U << 16) )
>>>                res->b |= 1U << 26;
>>> -        res->c |= 1U << 22;
>>> +        res->c |= (1U << 22) | (1U << 27) | (1U << 28);
>>
>> I've just noticed, but we shouldn't be having both the named variables
>> and these unnamed ones.  Is their presence a rebasing issue around
>> patches into the test suite?
>
> emul_test_init() gained its current shape from fd35f32b4b, while
> emul_test_cpuid() had been left untouched at that point. So I guess
> it's more like a forgotten piece of conversion work. I'm unsure
> though whether such a conversion is actually a good idea, since aiui
> it would mean cloning at least guest_cpuid()'s first switch() into
> this function, which is quite a bit more code than there is right
> now. Perhaps (the common part of) that switch() could be morphed
> into a library function ...

I'll throw it onto the big pile of CPUID work.  It is not trivial to
library-fy because of the Xen/Viridian leaf handling.

~Andrew

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