| Message ID | 099d03d0-2846-2a3d-93ec-2d10dab12655@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86emul: further work | expand |
On 05/05/2020 09:13, Jan Beulich wrote: > Note that the ISA extensions document revision 038 doesn't specify > exception behavior for ModRM.mod == 0b11; assuming #UD here. Stale. > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -11480,11 +11513,36 @@ int x86_emul_blk( > { > switch ( state->blk ) > { > + bool zf; > + > /* > * Throughout this switch(), memory clobbers are used to compensate > * that other operands may not properly express the (full) memory > * ranges covered. > */ > + case blk_enqcmd: > + ASSERT(bytes == 64); > + if ( ((unsigned long)ptr & 0x3f) ) > + { > + ASSERT_UNREACHABLE(); > + return X86EMUL_UNHANDLEABLE; > + } > + *eflags &= ~EFLAGS_MASK; > +#ifdef HAVE_AS_ENQCMD > + asm ( "enqcmds (%[src]), %[dst]" ASM_FLAG_OUT(, "; setz %0") %[zf] > + : [zf] ASM_FLAG_OUT("=@ccz", "=qm") (zf) > + : [src] "r" (data), [dst] "r" (ptr) : "memory" ); Can't src get away with being "m" (*data)? There is no need to force it into a single register, even if it is overwhelmingly likely to end up with %rsi scheduled here. > +#else > + /* enqcmds (%rsi), %rdi */ > + asm ( ".byte 0xf3, 0x0f, 0x38, 0xf8, 0x3e" > + ASM_FLAG_OUT(, "; setz %[zf]") > + : [zf] ASM_FLAG_OUT("=@ccz", "=qm") (zf) > + : "S" (data), "D" (ptr) : "memory" ); > +#endif > + if ( zf ) > + *eflags |= X86_EFLAGS_ZF; > + break; > + > case blk_movdir: > switch ( bytes ) > { > --- a/xen/include/asm-x86/msr-index.h > +++ b/xen/include/asm-x86/msr-index.h > @@ -420,6 +420,10 @@ > #define MSR_IA32_TSC_DEADLINE 0x000006E0 > #define MSR_IA32_ENERGY_PERF_BIAS 0x000001b0 > > +#define MSR_IA32_PASID 0x00000d93 > +#define PASID_PASID_MASK 0x000fffff > +#define PASID_VALID 0x80000000 > + Above the legacy line please as this is using the newer style, and drop _IA32. Intel's ideal of architectural-ness isn't interesting or worth the added code volume. PASSID_PASSID_MASK isn't great, but I can't suggest anything better, and MSR_PASSID_MAS doesn't work either. Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
On 07.05.2020 20:59, Andrew Cooper wrote: > On 05/05/2020 09:13, Jan Beulich wrote: >> Note that the ISA extensions document revision 038 doesn't specify >> exception behavior for ModRM.mod == 0b11; assuming #UD here. > > Stale. In which way (beyond the question of whether to use goto unrecognized_insn in the code instead)? The doc doesn't mention ModRM.mod specifics in any way. >> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >> @@ -11480,11 +11513,36 @@ int x86_emul_blk( >> { >> switch ( state->blk ) >> { >> + bool zf; >> + >> /* >> * Throughout this switch(), memory clobbers are used to compensate >> * that other operands may not properly express the (full) memory >> * ranges covered. >> */ >> + case blk_enqcmd: >> + ASSERT(bytes == 64); >> + if ( ((unsigned long)ptr & 0x3f) ) >> + { >> + ASSERT_UNREACHABLE(); >> + return X86EMUL_UNHANDLEABLE; >> + } >> + *eflags &= ~EFLAGS_MASK; >> +#ifdef HAVE_AS_ENQCMD >> + asm ( "enqcmds (%[src]), %[dst]" ASM_FLAG_OUT(, "; setz %0") > > %[zf] Oops, indeed. >> + : [zf] ASM_FLAG_OUT("=@ccz", "=qm") (zf) >> + : [src] "r" (data), [dst] "r" (ptr) : "memory" ); > > Can't src get away with being "m" (*data)? There is no need to force it > into a single register, even if it is overwhelmingly likely to end up > with %rsi scheduled here. Well, *data can't be used, as data is of void* type. It would need to have a suitable cast on it, but since that's not going to avoid the memory clobber I didn't think it was worth it (also together with the comment ahead of the switch()). >> --- a/xen/include/asm-x86/msr-index.h >> +++ b/xen/include/asm-x86/msr-index.h >> @@ -420,6 +420,10 @@ >> #define MSR_IA32_TSC_DEADLINE 0x000006E0 >> #define MSR_IA32_ENERGY_PERF_BIAS 0x000001b0 >> >> +#define MSR_IA32_PASID 0x00000d93 >> +#define PASID_PASID_MASK 0x000fffff >> +#define PASID_VALID 0x80000000 >> + > > Above the legacy line please as this is using the newer style, Ah, yes, I should have remembered to re-base this over your change there. > and drop > _IA32. Intel's ideal of architectural-ness isn't interesting or worth > the added code volume. We'd been there before, and you know I disagree. I think it is wrong for me to make the change, but I will do so just to retain your ack. > PASSID_PASSID_MASK isn't great, but I can't suggest anything better, and > MSR_PASSID_MAS doesn't work either. > > Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks. Jan
--- a/xen/arch/x86/arch.mk +++ b/xen/arch/x86/arch.mk @@ -48,6 +48,7 @@ $(call as-option-add,CFLAGS,CC,"clwb (%r $(call as-option-add,CFLAGS,CC,".equ \"x\"$$(comma)1",-DHAVE_AS_QUOTED_SYM) $(call as-option-add,CFLAGS,CC,"invpcid (%rax)$$(comma)%rax",-DHAVE_AS_INVPCID) $(call as-option-add,CFLAGS,CC,"movdiri %rax$$(comma)(%rax)",-DHAVE_AS_MOVDIR) +$(call as-option-add,CFLAGS,CC,"enqcmd (%rax)$$(comma)%rax",-DHAVE_AS_ENQCMD) # GAS's idea of true is -1. Clang's idea is 1 $(call as-option-add,CFLAGS,CC,\ --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -855,6 +855,7 @@ struct x86_emulate_state { } rmw; enum { blk_NONE, + blk_enqcmd, blk_movdir, } blk; uint8_t modrm, modrm_mod, modrm_reg, modrm_rm; @@ -901,6 +902,7 @@ typedef union { uint64_t __attribute__ ((aligned(16))) xmm[2]; uint64_t __attribute__ ((aligned(32))) ymm[4]; uint64_t __attribute__ ((aligned(64))) zmm[8]; + uint32_t data32[16]; } mmval_t; /* @@ -1922,6 +1924,7 @@ amd_like(const struct x86_emulate_ctxt * #define vcpu_has_rdpid() (ctxt->cpuid->feat.rdpid) #define vcpu_has_movdiri() (ctxt->cpuid->feat.movdiri) #define vcpu_has_movdir64b() (ctxt->cpuid->feat.movdir64b) +#define vcpu_has_enqcmd() (ctxt->cpuid->feat.enqcmd) #define vcpu_has_avx512_4vnniw() (ctxt->cpuid->feat.avx512_4vnniw) #define vcpu_has_avx512_4fmaps() (ctxt->cpuid->feat.avx512_4fmaps) #define vcpu_has_avx512_bf16() (ctxt->cpuid->feat.avx512_bf16) @@ -10200,6 +10203,36 @@ x86_emulate( state->simd_size = simd_none; break; + case X86EMUL_OPC_F2(0x0f38, 0xf8): /* enqcmd r,m512 */ + case X86EMUL_OPC_F3(0x0f38, 0xf8): /* enqcmds r,m512 */ + host_and_vcpu_must_have(enqcmd); + generate_exception_if(ea.type != OP_MEM, EXC_UD); + generate_exception_if(vex.pfx != vex_f2 && !mode_ring0(), EXC_GP, 0); + src.val = truncate_ea(*dst.reg); + generate_exception_if(!is_aligned(x86_seg_es, src.val, 64, ctxt, ops), + EXC_GP, 0); + fail_if(!ops->blk); + BUILD_BUG_ON(sizeof(*mmvalp) < 64); + if ( (rc = ops->read(ea.mem.seg, ea.mem.off, mmvalp, 64, + ctxt)) != X86EMUL_OKAY ) + goto done; + if ( vex.pfx == vex_f2 ) /* enqcmd */ + { + fail_if(!ops->read_msr); + if ( (rc = ops->read_msr(MSR_IA32_PASID, + &msr_val, ctxt)) != X86EMUL_OKAY ) + goto done; + generate_exception_if(!(msr_val & PASID_VALID), EXC_GP, 0); + mmvalp->data32[0] = MASK_EXTR(msr_val, PASID_PASID_MASK); + } + mmvalp->data32[0] &= ~0x7ff00000; + state->blk = blk_enqcmd; + if ( (rc = ops->blk(x86_seg_es, src.val, mmvalp, 64, &_regs.eflags, + state, ctxt)) != X86EMUL_OKAY ) + goto done; + state->simd_size = simd_none; + break; + case X86EMUL_OPC(0x0f38, 0xf9): /* movdiri mem,r */ host_and_vcpu_must_have(movdiri); generate_exception_if(dst.type != OP_MEM, EXC_UD); @@ -11480,11 +11513,36 @@ int x86_emul_blk( { switch ( state->blk ) { + bool zf; + /* * Throughout this switch(), memory clobbers are used to compensate * that other operands may not properly express the (full) memory * ranges covered. */ + case blk_enqcmd: + ASSERT(bytes == 64); + if ( ((unsigned long)ptr & 0x3f) ) + { + ASSERT_UNREACHABLE(); + return X86EMUL_UNHANDLEABLE; + } + *eflags &= ~EFLAGS_MASK; +#ifdef HAVE_AS_ENQCMD + asm ( "enqcmds (%[src]), %[dst]" ASM_FLAG_OUT(, "; setz %0") + : [zf] ASM_FLAG_OUT("=@ccz", "=qm") (zf) + : [src] "r" (data), [dst] "r" (ptr) : "memory" ); +#else + /* enqcmds (%rsi), %rdi */ + asm ( ".byte 0xf3, 0x0f, 0x38, 0xf8, 0x3e" + ASM_FLAG_OUT(, "; setz %[zf]") + : [zf] ASM_FLAG_OUT("=@ccz", "=qm") (zf) + : "S" (data), "D" (ptr) : "memory" ); +#endif + if ( zf ) + *eflags |= X86_EFLAGS_ZF; + break; + case blk_movdir: switch ( bytes ) { --- a/xen/include/asm-x86/cpufeature.h +++ b/xen/include/asm-x86/cpufeature.h @@ -120,6 +120,7 @@ #define cpu_has_rdpid boot_cpu_has(X86_FEATURE_RDPID) #define cpu_has_movdiri boot_cpu_has(X86_FEATURE_MOVDIRI) #define cpu_has_movdir64b boot_cpu_has(X86_FEATURE_MOVDIR64B) +#define cpu_has_enqcmd boot_cpu_has(X86_FEATURE_ENQCMD) /* CPUID level 0x80000007.edx */ #define cpu_has_itsc boot_cpu_has(X86_FEATURE_ITSC) --- a/xen/include/asm-x86/msr-index.h +++ b/xen/include/asm-x86/msr-index.h @@ -420,6 +420,10 @@ #define MSR_IA32_TSC_DEADLINE 0x000006E0 #define MSR_IA32_ENERGY_PERF_BIAS 0x000001b0 +#define MSR_IA32_PASID 0x00000d93 +#define PASID_PASID_MASK 0x000fffff +#define PASID_VALID 0x80000000 + /* Platform Shared Resource MSRs */ #define MSR_IA32_CMT_EVTSEL 0x00000c8d #define MSR_IA32_CMT_EVTSEL_UE_MASK 0x0000ffff --- a/xen/include/public/arch-x86/cpufeatureset.h +++ b/xen/include/public/arch-x86/cpufeatureset.h @@ -240,6 +240,7 @@ XEN_CPUFEATURE(RDPID, 6*32+22) / XEN_CPUFEATURE(CLDEMOTE, 6*32+25) /*A CLDEMOTE instruction */ XEN_CPUFEATURE(MOVDIRI, 6*32+27) /*A MOVDIRI instruction */ XEN_CPUFEATURE(MOVDIR64B, 6*32+28) /*A MOVDIR64B instruction */ +XEN_CPUFEATURE(ENQCMD, 6*32+29) /* ENQCMD{,S} instructions */ /* AMD-defined CPU features, CPUID level 0x80000007.edx, word 7 */ XEN_CPUFEATURE(ITSC, 7*32+ 8) /* Invariant TSC */
Note that the ISA extensions document revision 038 doesn't specify exception behavior for ModRM.mod == 0b11; assuming #UD here. No tests are being added to the harness - this would be quite hard, we can't just issue the insns against RAM. Their similarity with MOVDIR64B should have the test case there be god enough to cover any fundamental flaws. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- TBD: This doesn't (can't) consult PASID translation tables yet, as we have no VMX code for this so far. I guess for this we will want to replace the direct ->read_msr(MSR_IA32_PASID, ...) with a new ->read_pasid() hook. --- v7: Re-base. v6: Re-base. v5: New.