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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel