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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Jan

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