We claim to support the insn, but so far the emulator has been handling
it as a NOP.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
While handling x86emul_cldemote separately in hvmemul_cache_op() means
to carry some redundant code, folding it with CLFLUSH{,OPT} / CLWB
didn't seem very attractive either.
--- a/xen/arch/x86/arch.mk
+++ b/xen/arch/x86/arch.mk
@@ -23,6 +23,7 @@ $(call as-option-add,CFLAGS,CC,"xsaveopt
$(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED)
$(call as-option-add,CFLAGS,CC,"clac",-DHAVE_AS_CLAC_STAC)
$(call as-option-add,CFLAGS,CC,"clwb (%rax)",-DHAVE_AS_CLWB)
+$(call as-option-add,CFLAGS,CC,"cldemote (%rax)",-DHAVE_AS_CLDEMOTE)
$(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)
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -2351,6 +2351,28 @@ static int hvmemul_cache_op(
* to be sensibly used is in (virtualization unaware) firmware.
*/
break;
+
+ case x86emul_cldemote:
+ ASSERT(!is_x86_system_segment(seg));
+
+ if ( !boot_cpu_has(X86_FEATURE_CLDEMOTE) ||
+ hvmemul_virtual_to_linear(seg, offset, 0, NULL, hvm_access_none,
+ hvmemul_ctxt, &addr) != X86EMUL_OKAY )
+ break;
+
+ if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 )
+ pfec |= PFEC_user_mode;
+
+ mapping = hvmemul_map_linear_addr(addr, 0, pfec, hvmemul_ctxt);
+ if ( mapping == ERR_PTR(~X86EMUL_EXCEPTION) )
+ x86_emul_reset_event(&hvmemul_ctxt->ctxt);
+ if ( IS_ERR_OR_NULL(mapping) )
+ break;
+
+ cldemote(mapping);
+
+ hvmemul_unmap_linear_addr(mapping, addr, 0, hvmemul_ctxt);
+ break;
}
return X86EMUL_OKAY;
--- a/xen/arch/x86/include/asm/system.h
+++ b/xen/arch/x86/include/asm/system.h
@@ -37,6 +37,16 @@ static inline void clwb(const void *p)
#endif
}
+static inline void cldemote(const void *p)
+{
+#if defined(HAVE_AS_CLDEMOTE)
+ asm volatile ( "cldemote %0" :: "m" (*(const char *)p) );
+#else
+ asm volatile ( ".byte 0x0f, 0x1c, 0x02"
+ :: "d" (p), "m" (*(const char *)p) );
+#endif
+}
+
#define xchg(ptr,v) \
((__typeof__(*(ptr)))__xchg((unsigned long)(v),(ptr),sizeof(*(ptr))))
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -6290,7 +6290,8 @@ x86_emulate(
case X86EMUL_OPC(0x0f, 0x0d): /* GrpP (prefetch) */
case X86EMUL_OPC(0x0f, 0x18): /* Grp16 (prefetch/nop) */
- case X86EMUL_OPC(0x0f, 0x19) ... X86EMUL_OPC(0x0f, 0x1f): /* nop */
+ case X86EMUL_OPC(0x0f, 0x19) ... X86EMUL_OPC(0x0f, 0x1b): /* nop */
+ case X86EMUL_OPC(0x0f, 0x1d) ... X86EMUL_OPC(0x0f, 0x1f): /* nop */
break;
#ifndef X86EMUL_NO_MMX
@@ -6627,6 +6628,12 @@ x86_emulate(
#endif /* !X86EMUL_NO_SIMD */
+ case X86EMUL_OPC(0x0f, 0x1c): /* cldemote / nop */
+ if ( ctxt->cpuid->feat.cldemote && !vex.pfx && !modrm_reg &&
+ ops->cache_op )
+ ops->cache_op(x86emul_cldemote, ea.mem.seg, ea.mem.off, ctxt);
+ break;
+
case X86EMUL_OPC(0x0f, 0x20): /* mov cr,reg */
case X86EMUL_OPC(0x0f, 0x21): /* mov dr,reg */
case X86EMUL_OPC(0x0f, 0x22): /* mov reg,cr */
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -177,6 +177,7 @@ enum x86_emulate_fpu_type {
};
enum x86emul_cache_op {
+ x86emul_cldemote,
x86emul_clflush,
x86emul_clflushopt,
x86emul_clwb,
On Tue, Jan 25, 2022 at 03:22:25PM +0100, Jan Beulich wrote: > We claim to support the insn, but so far the emulator has been handling > it as a NOP. While not ideal, the SDM mentions that "The CLDEMOTE instruction may be ignored by hardware in certain cases and is not a guarantee.". > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > While handling x86emul_cldemote separately in hvmemul_cache_op() means > to carry some redundant code, folding it with CLFLUSH{,OPT} / CLWB > didn't seem very attractive either. > > --- a/xen/arch/x86/arch.mk > +++ b/xen/arch/x86/arch.mk > @@ -23,6 +23,7 @@ $(call as-option-add,CFLAGS,CC,"xsaveopt > $(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED) > $(call as-option-add,CFLAGS,CC,"clac",-DHAVE_AS_CLAC_STAC) > $(call as-option-add,CFLAGS,CC,"clwb (%rax)",-DHAVE_AS_CLWB) > +$(call as-option-add,CFLAGS,CC,"cldemote (%rax)",-DHAVE_AS_CLDEMOTE) > $(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) > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -2351,6 +2351,28 @@ static int hvmemul_cache_op( > * to be sensibly used is in (virtualization unaware) firmware. > */ > break; > + > + case x86emul_cldemote: > + ASSERT(!is_x86_system_segment(seg)); > + > + if ( !boot_cpu_has(X86_FEATURE_CLDEMOTE) || > + hvmemul_virtual_to_linear(seg, offset, 0, NULL, hvm_access_none, > + hvmemul_ctxt, &addr) != X86EMUL_OKAY ) > + break; > + > + if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 ) > + pfec |= PFEC_user_mode; > + > + mapping = hvmemul_map_linear_addr(addr, 0, pfec, hvmemul_ctxt); I think the emulator should map the address using the same cache attributes as the guest, or else the result might be different than intended? Thanks, Roger.
On 25.01.2022 15:35, Roger Pau Monné wrote: > On Tue, Jan 25, 2022 at 03:22:25PM +0100, Jan Beulich wrote: >> We claim to support the insn, but so far the emulator has been handling >> it as a NOP. > > While not ideal, the SDM mentions that "The CLDEMOTE instruction may > be ignored by hardware in certain cases and is not a guarantee.". Right; the same is effectively the case for CLFLUSH etc. Still, unlike prefetches, we implement them in the emulator. >> --- a/xen/arch/x86/hvm/emulate.c >> +++ b/xen/arch/x86/hvm/emulate.c >> @@ -2351,6 +2351,28 @@ static int hvmemul_cache_op( >> * to be sensibly used is in (virtualization unaware) firmware. >> */ >> break; >> + >> + case x86emul_cldemote: >> + ASSERT(!is_x86_system_segment(seg)); >> + >> + if ( !boot_cpu_has(X86_FEATURE_CLDEMOTE) || >> + hvmemul_virtual_to_linear(seg, offset, 0, NULL, hvm_access_none, >> + hvmemul_ctxt, &addr) != X86EMUL_OKAY ) >> + break; >> + >> + if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 ) >> + pfec |= PFEC_user_mode; >> + >> + mapping = hvmemul_map_linear_addr(addr, 0, pfec, hvmemul_ctxt); > > I think the emulator should map the address using the same cache > attributes as the guest, or else the result might be different than > intended? That's a pre-existing problem everywhere, not something specific to this one insn. Jan
On 25.01.2022 15:22, Jan Beulich wrote: > We claim to support the insn, but so far the emulator has been handling > it as a NOP. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> I'm sorry, I should have Cc-ed Paul here as well. Jan > --- > While handling x86emul_cldemote separately in hvmemul_cache_op() means > to carry some redundant code, folding it with CLFLUSH{,OPT} / CLWB > didn't seem very attractive either. > > --- a/xen/arch/x86/arch.mk > +++ b/xen/arch/x86/arch.mk > @@ -23,6 +23,7 @@ $(call as-option-add,CFLAGS,CC,"xsaveopt > $(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED) > $(call as-option-add,CFLAGS,CC,"clac",-DHAVE_AS_CLAC_STAC) > $(call as-option-add,CFLAGS,CC,"clwb (%rax)",-DHAVE_AS_CLWB) > +$(call as-option-add,CFLAGS,CC,"cldemote (%rax)",-DHAVE_AS_CLDEMOTE) > $(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) > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -2351,6 +2351,28 @@ static int hvmemul_cache_op( > * to be sensibly used is in (virtualization unaware) firmware. > */ > break; > + > + case x86emul_cldemote: > + ASSERT(!is_x86_system_segment(seg)); > + > + if ( !boot_cpu_has(X86_FEATURE_CLDEMOTE) || > + hvmemul_virtual_to_linear(seg, offset, 0, NULL, hvm_access_none, > + hvmemul_ctxt, &addr) != X86EMUL_OKAY ) > + break; > + > + if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 ) > + pfec |= PFEC_user_mode; > + > + mapping = hvmemul_map_linear_addr(addr, 0, pfec, hvmemul_ctxt); > + if ( mapping == ERR_PTR(~X86EMUL_EXCEPTION) ) > + x86_emul_reset_event(&hvmemul_ctxt->ctxt); > + if ( IS_ERR_OR_NULL(mapping) ) > + break; > + > + cldemote(mapping); > + > + hvmemul_unmap_linear_addr(mapping, addr, 0, hvmemul_ctxt); > + break; > } > > return X86EMUL_OKAY; > --- a/xen/arch/x86/include/asm/system.h > +++ b/xen/arch/x86/include/asm/system.h > @@ -37,6 +37,16 @@ static inline void clwb(const void *p) > #endif > } > > +static inline void cldemote(const void *p) > +{ > +#if defined(HAVE_AS_CLDEMOTE) > + asm volatile ( "cldemote %0" :: "m" (*(const char *)p) ); > +#else > + asm volatile ( ".byte 0x0f, 0x1c, 0x02" > + :: "d" (p), "m" (*(const char *)p) ); > +#endif > +} > + > #define xchg(ptr,v) \ > ((__typeof__(*(ptr)))__xchg((unsigned long)(v),(ptr),sizeof(*(ptr)))) > > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -6290,7 +6290,8 @@ x86_emulate( > > case X86EMUL_OPC(0x0f, 0x0d): /* GrpP (prefetch) */ > case X86EMUL_OPC(0x0f, 0x18): /* Grp16 (prefetch/nop) */ > - case X86EMUL_OPC(0x0f, 0x19) ... X86EMUL_OPC(0x0f, 0x1f): /* nop */ > + case X86EMUL_OPC(0x0f, 0x19) ... X86EMUL_OPC(0x0f, 0x1b): /* nop */ > + case X86EMUL_OPC(0x0f, 0x1d) ... X86EMUL_OPC(0x0f, 0x1f): /* nop */ > break; > > #ifndef X86EMUL_NO_MMX > @@ -6627,6 +6628,12 @@ x86_emulate( > > #endif /* !X86EMUL_NO_SIMD */ > > + case X86EMUL_OPC(0x0f, 0x1c): /* cldemote / nop */ > + if ( ctxt->cpuid->feat.cldemote && !vex.pfx && !modrm_reg && > + ops->cache_op ) > + ops->cache_op(x86emul_cldemote, ea.mem.seg, ea.mem.off, ctxt); > + break; > + > case X86EMUL_OPC(0x0f, 0x20): /* mov cr,reg */ > case X86EMUL_OPC(0x0f, 0x21): /* mov dr,reg */ > case X86EMUL_OPC(0x0f, 0x22): /* mov reg,cr */ > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -177,6 +177,7 @@ enum x86_emulate_fpu_type { > }; > > enum x86emul_cache_op { > + x86emul_cldemote, > x86emul_clflush, > x86emul_clflushopt, > x86emul_clwb, > >
On 25/01/2022 15:08, Jan Beulich wrote: > On 25.01.2022 15:22, Jan Beulich wrote: >> We claim to support the insn, but so far the emulator has been handling >> it as a NOP. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > I'm sorry, I should have Cc-ed Paul here as well. > Acked-by: Paul Durrant <paul@xen.org> > Jan > >> --- >> While handling x86emul_cldemote separately in hvmemul_cache_op() means >> to carry some redundant code, folding it with CLFLUSH{,OPT} / CLWB >> didn't seem very attractive either. >> >> --- a/xen/arch/x86/arch.mk >> +++ b/xen/arch/x86/arch.mk >> @@ -23,6 +23,7 @@ $(call as-option-add,CFLAGS,CC,"xsaveopt >> $(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED) >> $(call as-option-add,CFLAGS,CC,"clac",-DHAVE_AS_CLAC_STAC) >> $(call as-option-add,CFLAGS,CC,"clwb (%rax)",-DHAVE_AS_CLWB) >> +$(call as-option-add,CFLAGS,CC,"cldemote (%rax)",-DHAVE_AS_CLDEMOTE) >> $(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) >> --- a/xen/arch/x86/hvm/emulate.c >> +++ b/xen/arch/x86/hvm/emulate.c >> @@ -2351,6 +2351,28 @@ static int hvmemul_cache_op( >> * to be sensibly used is in (virtualization unaware) firmware. >> */ >> break; >> + >> + case x86emul_cldemote: >> + ASSERT(!is_x86_system_segment(seg)); >> + >> + if ( !boot_cpu_has(X86_FEATURE_CLDEMOTE) || >> + hvmemul_virtual_to_linear(seg, offset, 0, NULL, hvm_access_none, >> + hvmemul_ctxt, &addr) != X86EMUL_OKAY ) >> + break; >> + >> + if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 ) >> + pfec |= PFEC_user_mode; >> + >> + mapping = hvmemul_map_linear_addr(addr, 0, pfec, hvmemul_ctxt); >> + if ( mapping == ERR_PTR(~X86EMUL_EXCEPTION) ) >> + x86_emul_reset_event(&hvmemul_ctxt->ctxt); >> + if ( IS_ERR_OR_NULL(mapping) ) >> + break; >> + >> + cldemote(mapping); >> + >> + hvmemul_unmap_linear_addr(mapping, addr, 0, hvmemul_ctxt); >> + break; >> } >> >> return X86EMUL_OKAY; >> --- a/xen/arch/x86/include/asm/system.h >> +++ b/xen/arch/x86/include/asm/system.h >> @@ -37,6 +37,16 @@ static inline void clwb(const void *p) >> #endif >> } >> >> +static inline void cldemote(const void *p) >> +{ >> +#if defined(HAVE_AS_CLDEMOTE) >> + asm volatile ( "cldemote %0" :: "m" (*(const char *)p) ); >> +#else >> + asm volatile ( ".byte 0x0f, 0x1c, 0x02" >> + :: "d" (p), "m" (*(const char *)p) ); >> +#endif >> +} >> + >> #define xchg(ptr,v) \ >> ((__typeof__(*(ptr)))__xchg((unsigned long)(v),(ptr),sizeof(*(ptr)))) >> >> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >> @@ -6290,7 +6290,8 @@ x86_emulate( >> >> case X86EMUL_OPC(0x0f, 0x0d): /* GrpP (prefetch) */ >> case X86EMUL_OPC(0x0f, 0x18): /* Grp16 (prefetch/nop) */ >> - case X86EMUL_OPC(0x0f, 0x19) ... X86EMUL_OPC(0x0f, 0x1f): /* nop */ >> + case X86EMUL_OPC(0x0f, 0x19) ... X86EMUL_OPC(0x0f, 0x1b): /* nop */ >> + case X86EMUL_OPC(0x0f, 0x1d) ... X86EMUL_OPC(0x0f, 0x1f): /* nop */ >> break; >> >> #ifndef X86EMUL_NO_MMX >> @@ -6627,6 +6628,12 @@ x86_emulate( >> >> #endif /* !X86EMUL_NO_SIMD */ >> >> + case X86EMUL_OPC(0x0f, 0x1c): /* cldemote / nop */ >> + if ( ctxt->cpuid->feat.cldemote && !vex.pfx && !modrm_reg && >> + ops->cache_op ) >> + ops->cache_op(x86emul_cldemote, ea.mem.seg, ea.mem.off, ctxt); >> + break; >> + >> case X86EMUL_OPC(0x0f, 0x20): /* mov cr,reg */ >> case X86EMUL_OPC(0x0f, 0x21): /* mov dr,reg */ >> case X86EMUL_OPC(0x0f, 0x22): /* mov reg,cr */ >> --- a/xen/arch/x86/x86_emulate/x86_emulate.h >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h >> @@ -177,6 +177,7 @@ enum x86_emulate_fpu_type { >> }; >> >> enum x86emul_cache_op { >> + x86emul_cldemote, >> x86emul_clflush, >> x86emul_clflushopt, >> x86emul_clwb, >> >> >
On 25/01/2022 14:22, Jan Beulich wrote: > We claim to support the insn, but so far the emulator has been handling > it as a NOP. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Treating CLDEMOTE as a NOP is going to be more efficient than actually setting up the mapping to execute a real CLDEMOTE instruction on the line in question. CLDEMOTE is very specifically an optimisation for software producer/consumer pairs. If we want to take this patch, it should Fix[es]: ad3abc47dd23c which made the claim that CLDEMOTE needed no further additions. The only issue on whether we can treat it as a NOP completely is whether we believe the exception list. I'm not sure I believe the absence of AGU faults, but the instruction is taken from hint-nop space so guaranteed to behave similarly to clflush/clwb. ~Andrew
On 25.01.2022 16:09, Andrew Cooper wrote: > On 25/01/2022 14:22, Jan Beulich wrote: >> We claim to support the insn, but so far the emulator has been handling >> it as a NOP. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Treating CLDEMOTE as a NOP is going to be more efficient than actually > setting up the mapping to execute a real CLDEMOTE instruction on the > line in question. CLDEMOTE is very specifically an optimisation for > software producer/consumer pairs. Some similar argument could likely be made for treating CLFLUSH etc as just a NOP then? > If we want to take this patch, it should Fix[es]: ad3abc47dd23c which > made the claim that CLDEMOTE needed no further additions. Added. > The only issue on whether we can treat it as a NOP completely is whether > we believe the exception list. I'm not sure I believe the absence of > AGU faults, I also was puzzled by this, but I have no way to verify one way or the other. Hence the implementation follows what the SDM says. > but the instruction is taken from hint-nop space so > guaranteed to behave similarly to clflush/clwb. I'm confused: CLFLUSH / CLWB specifically do not live in NOP space: The former are under 0FAE, while NOP space is 0F18 ... 0F1F (with CLDEMOTE being 0F1C). Jan
© 2016 - 2024 Red Hat, Inc.