| Message ID | e41a2f72-ede5-adec-dc82-65b76368b7f7@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86emul: further work | expand |
On 24/03/2020 12:37, Jan Beulich wrote: > The dependency on a new EFER bit implies that we need to set that bit > ourselves in order to be able to successfully invoke the insn. > > Also once again introduce the SVM related constants at this occasion. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > RFC: The exact meaning of the PM stating "any errors encountered by > those stores have been signaled to associated error logging > resources" is unclear. Depending on what this entails, blindly > enabling EFER.MCOMMIT may not be a good idea. Hence the RFC. Not just that. Its not safe for Xen to ever execute MCOMMIT for emulation purposes. From what I can glean from the documentation, it is intended for non-volatile RAM, but I can't find anything discussing the error handling. The fact the instruction can be intercepted in the first place hopefully means that there must be something Xen can look at to get the real error indicator. However, the suggestion is that this will all be platform specific. The emulation problem comes from the fact that if Xen has any pending writes to to NVRAM as part of the emulation path (or an interrupt for that matter), an error intended for Xen would leak into guest context. ~Andrew
On 03.04.2020 01:47, Andrew Cooper wrote: > On 24/03/2020 12:37, Jan Beulich wrote: >> The dependency on a new EFER bit implies that we need to set that bit >> ourselves in order to be able to successfully invoke the insn. >> >> Also once again introduce the SVM related constants at this occasion. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> RFC: The exact meaning of the PM stating "any errors encountered by >> those stores have been signaled to associated error logging >> resources" is unclear. Depending on what this entails, blindly >> enabling EFER.MCOMMIT may not be a good idea. Hence the RFC. > > Not just that. Its not safe for Xen to ever execute MCOMMIT for > emulation purposes. I.e. you're suggesting we mustn't even try to emulate it? > From what I can glean from the documentation, it is intended for > non-volatile RAM, but I can't find anything discussing the error handling. > > The fact the instruction can be intercepted in the first place hopefully > means that there must be something Xen can look at to get the real error > indicator. However, the suggestion is that this will all be platform > specific. > > > The emulation problem comes from the fact that if Xen has any pending > writes to to NVRAM as part of the emulation path (or an interrupt for > that matter), an error intended for Xen would leak into guest context. I'm afraid all of this is guesswork until it becomes clear how exactly this error reporting is intended to work. Jan
On 03/04/2020 09:00, Jan Beulich wrote: > On 03.04.2020 01:47, Andrew Cooper wrote: >> On 24/03/2020 12:37, Jan Beulich wrote: >>> The dependency on a new EFER bit implies that we need to set that bit >>> ourselves in order to be able to successfully invoke the insn. >>> >>> Also once again introduce the SVM related constants at this occasion. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> RFC: The exact meaning of the PM stating "any errors encountered by >>> those stores have been signaled to associated error logging >>> resources" is unclear. Depending on what this entails, blindly >>> enabling EFER.MCOMMIT may not be a good idea. Hence the RFC. >> Not just that. Its not safe for Xen to ever execute MCOMMIT for >> emulation purposes. > I.e. you're suggesting we mustn't even try to emulate it? Sorry - that's not quite what I intended to mean. > >> From what I can glean from the documentation, it is intended for >> non-volatile RAM, but I can't find anything discussing the error handling. >> >> The fact the instruction can be intercepted in the first place hopefully >> means that there must be something Xen can look at to get the real error >> indicator. However, the suggestion is that this will all be platform >> specific. >> >> >> The emulation problem comes from the fact that if Xen has any pending >> writes to to NVRAM as part of the emulation path (or an interrupt for >> that matter), an error intended for Xen would leak into guest context. > I'm afraid all of this is guesswork until it becomes clear how > exactly this error reporting is intended to work. What I meant was that "emulating MCOMMIT can't involve executing an MCOMMIT instruction". In some future where we have combined intercept and emulation paths, whatever ends up existing will still have to reach out to the error banks directly to figure out what is going on. ~Andrew
On 03.04.2020 17:00, Andrew Cooper wrote: > On 03/04/2020 09:00, Jan Beulich wrote: >> On 03.04.2020 01:47, Andrew Cooper wrote: >>> On 24/03/2020 12:37, Jan Beulich wrote: >>>> The dependency on a new EFER bit implies that we need to set that bit >>>> ourselves in order to be able to successfully invoke the insn. >>>> >>>> Also once again introduce the SVM related constants at this occasion. >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> --- >>>> RFC: The exact meaning of the PM stating "any errors encountered by >>>> those stores have been signaled to associated error logging >>>> resources" is unclear. Depending on what this entails, blindly >>>> enabling EFER.MCOMMIT may not be a good idea. Hence the RFC. >>> Not just that. Its not safe for Xen to ever execute MCOMMIT for >>> emulation purposes. >> I.e. you're suggesting we mustn't even try to emulate it? > > Sorry - that's not quite what I intended to mean. > >> >>> From what I can glean from the documentation, it is intended for >>> non-volatile RAM, but I can't find anything discussing the error handling. >>> >>> The fact the instruction can be intercepted in the first place hopefully >>> means that there must be something Xen can look at to get the real error >>> indicator. However, the suggestion is that this will all be platform >>> specific. >>> >>> >>> The emulation problem comes from the fact that if Xen has any pending >>> writes to to NVRAM as part of the emulation path (or an interrupt for >>> that matter), an error intended for Xen would leak into guest context. >> I'm afraid all of this is guesswork until it becomes clear how >> exactly this error reporting is intended to work. > > What I meant was that "emulating MCOMMIT can't involve executing an > MCOMMIT instruction". I still don't see why - the error recording is (presumably) not dependent upon the context in which the insn was issued. > In some future where we have combined intercept and emulation paths, > whatever ends up existing will still have to reach out to the error > banks directly to figure out what is going on. I.e. you're assuming there's going to be an architectural way to access those, rather than perhaps many platform specific ones? Jan
On 03/04/2020 16:09, Jan Beulich wrote: > On 03.04.2020 17:00, Andrew Cooper wrote: >> On 03/04/2020 09:00, Jan Beulich wrote: >>> On 03.04.2020 01:47, Andrew Cooper wrote: >>>> On 24/03/2020 12:37, Jan Beulich wrote: >>>>> The dependency on a new EFER bit implies that we need to set that bit >>>>> ourselves in order to be able to successfully invoke the insn. >>>>> >>>>> Also once again introduce the SVM related constants at this occasion. >>>>> >>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>>> --- >>>>> RFC: The exact meaning of the PM stating "any errors encountered by >>>>> those stores have been signaled to associated error logging >>>>> resources" is unclear. Depending on what this entails, blindly >>>>> enabling EFER.MCOMMIT may not be a good idea. Hence the RFC. >>>> Not just that. Its not safe for Xen to ever execute MCOMMIT for >>>> emulation purposes. >>> I.e. you're suggesting we mustn't even try to emulate it? >> Sorry - that's not quite what I intended to mean. >> >>>> From what I can glean from the documentation, it is intended for >>>> non-volatile RAM, but I can't find anything discussing the error handling. >>>> >>>> The fact the instruction can be intercepted in the first place hopefully >>>> means that there must be something Xen can look at to get the real error >>>> indicator. However, the suggestion is that this will all be platform >>>> specific. >>>> >>>> >>>> The emulation problem comes from the fact that if Xen has any pending >>>> writes to to NVRAM as part of the emulation path (or an interrupt for >>>> that matter), an error intended for Xen would leak into guest context. >>> I'm afraid all of this is guesswork until it becomes clear how >>> exactly this error reporting is intended to work. >> What I meant was that "emulating MCOMMIT can't involve executing an >> MCOMMIT instruction". > I still don't see why - the error recording is (presumably) not > dependent upon the context in which the insn was issued. And that is the problem. This instruction has a 1-bit "everything ok" vs "something went wrong" feedback. We can't be telling a guest that something went wrong when in fact it was Xen doing something unrelated which suffered the error. >> In some future where we have combined intercept and emulation paths, >> whatever ends up existing will still have to reach out to the error >> banks directly to figure out what is going on. > I.e. you're assuming there's going to be an architectural way to > access those, rather than perhaps many platform specific ones? I think we're going to have to wait and see what materialises. ~Andrew
--- a/tools/libxl/libxl_cpuid.c +++ b/tools/libxl/libxl_cpuid.c @@ -261,6 +261,7 @@ int libxl_cpuid_parse_config(libxl_cpuid {"clzero", 0x80000008, NA, CPUID_REG_EBX, 0, 1}, {"rstr-fp-err-ptrs", 0x80000008, NA, CPUID_REG_EBX, 2, 1}, {"rdpru", 0x80000008, NA, CPUID_REG_EBX, 4, 1}, + {"mcommit", 0x80000008, NA, CPUID_REG_EBX, 8, 1}, {"wbnoinvd", 0x80000008, NA, CPUID_REG_EBX, 9, 1}, {"ibpb", 0x80000008, NA, CPUID_REG_EBX, 12, 1}, {"ppin", 0x80000008, NA, CPUID_REG_EBX, 23, 1}, --- a/tools/misc/xen-cpuid.c +++ b/tools/misc/xen-cpuid.c @@ -149,7 +149,7 @@ static const char *const str_e8b[32] = [ 4] = "rdpru", - /* [ 8] */ [ 9] = "wbnoinvd", + [ 8] = "mcommit", [ 9] = "wbnoinvd", [12] = "ibpb", --- a/xen/arch/x86/cpu/amd.c +++ b/xen/arch/x86/cpu/amd.c @@ -798,6 +798,9 @@ static void init_amd(struct cpuinfo_x86 wrmsr(MSR_K7_HWCR, l, h); } + if (cpu_has(c, X86_FEATURE_MCOMMIT)) + write_efer(read_efer() | EFER_MCOMMIT); + /* Prevent TSC drift in non single-processor, single-core platforms. */ if ((smp_processor_id() == 1) && !cpu_has(c, X86_FEATURE_ITSC)) disable_c1_ramping(); --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -1877,6 +1877,7 @@ in_protmode( #define vcpu_has_fma4() (ctxt->cpuid->extd.fma4) #define vcpu_has_tbm() (ctxt->cpuid->extd.tbm) #define vcpu_has_clzero() (ctxt->cpuid->extd.clzero) +#define vcpu_has_mcommit() (ctxt->cpuid->extd.mcommit) #define vcpu_has_rdpru() (ctxt->cpuid->extd.rdpru) #define vcpu_has_wbnoinvd() (ctxt->cpuid->extd.wbnoinvd) @@ -5676,6 +5677,28 @@ x86_emulate( _regs.r(cx) = (uint32_t)msr_val; goto rdtsc; + case 0xfa: /* monitorx / mcommit */ + if ( vex.pfx == vex_f3 ) + { + bool cf; + + host_and_vcpu_must_have(mcommit); + if ( !ops->read_msr || + ops->read_msr(MSR_EFER, &msr_val, ctxt) != X86EMUL_OKAY ) + msr_val = 0; + generate_exception_if(!(msr_val & EFER_MCOMMIT), EXC_UD); + memcpy(get_stub(stub), + ((uint8_t[]){ 0xf3, 0x0f, 0x01, 0xfa, 0xc3 }), 5); + _regs.eflags &= ~EFLAGS_MASK; + invoke_stub("", ASM_FLAG_OUT(, "setc %[cf]"), + [cf] ASM_FLAG_OUT("=@ccc", "=qm") (cf) : "i" (0)); + if ( cf ) + _regs.eflags |= X86_EFLAGS_CF; + put_stub(stub); + goto done; + } + goto unrecognized_insn; + case 0xfc: /* clzero */ { unsigned long zero = 0; --- a/xen/include/asm-x86/cpufeature.h +++ b/xen/include/asm-x86/cpufeature.h @@ -131,6 +131,9 @@ #define cpu_has_avx512_4fmaps boot_cpu_has(X86_FEATURE_AVX512_4FMAPS) #define cpu_has_tsx_force_abort boot_cpu_has(X86_FEATURE_TSX_FORCE_ABORT) +/* CPUID level 0x80000008.ebx */ +#define cpu_has_mcommit boot_cpu_has(X86_FEATURE_MCOMMIT) + /* CPUID level 0x00000007:1.eax */ #define cpu_has_avx512_bf16 boot_cpu_has(X86_FEATURE_AVX512_BF16) --- a/xen/include/asm-x86/hvm/svm/vmcb.h +++ b/xen/include/asm-x86/hvm/svm/vmcb.h @@ -78,6 +78,11 @@ enum GenericIntercept2bits GENERAL2_INTERCEPT_RDPRU = 1 << 14, }; +/* general 3 intercepts */ +enum GenericIntercept3bits +{ + GENERAL3_INTERCEPT_MCOMMIT = 1 << 3, +}; /* control register intercepts */ enum CRInterceptBits @@ -300,6 +305,7 @@ enum VMEXIT_EXITCODE VMEXIT_MWAIT_CONDITIONAL= 140, /* 0x8c */ VMEXIT_XSETBV = 141, /* 0x8d */ VMEXIT_RDPRU = 142, /* 0x8e */ + VMEXIT_MCOMMIT = 163, /* 0xa3 */ VMEXIT_NPF = 1024, /* 0x400, nested paging fault */ VMEXIT_INVALID = -1 }; @@ -406,7 +412,8 @@ struct vmcb_struct { u32 _exception_intercepts; /* offset 0x08 - cleanbit 0 */ u32 _general1_intercepts; /* offset 0x0C - cleanbit 0 */ u32 _general2_intercepts; /* offset 0x10 - cleanbit 0 */ - u32 res01[10]; + u32 _general3_intercepts; /* offset 0x14 - cleanbit 0 */ + u32 res01[9]; u16 _pause_filter_thresh; /* offset 0x3C - cleanbit 0 */ u16 _pause_filter_count; /* offset 0x3E - cleanbit 0 */ u64 _iopm_base_pa; /* offset 0x40 - cleanbit 1 */ @@ -590,6 +597,7 @@ VMCB_ACCESSORS(dr_intercepts, intercepts VMCB_ACCESSORS(exception_intercepts, intercepts) VMCB_ACCESSORS(general1_intercepts, intercepts) VMCB_ACCESSORS(general2_intercepts, intercepts) +VMCB_ACCESSORS(general3_intercepts, intercepts) VMCB_ACCESSORS(pause_filter_count, intercepts) VMCB_ACCESSORS(pause_filter_thresh, intercepts) VMCB_ACCESSORS(tsc_offset, intercepts) --- a/xen/include/asm-x86/msr-index.h +++ b/xen/include/asm-x86/msr-index.h @@ -88,6 +88,7 @@ #define _EFER_NX 11 /* No execute enable */ #define _EFER_SVME 12 /* AMD: SVM enable */ #define _EFER_FFXSE 14 /* AMD: Fast FXSAVE/FXRSTOR enable */ +#define _EFER_MCOMMIT 17 /* AMD: MCOMMIT insn enable */ #define EFER_SCE (1<<_EFER_SCE) #define EFER_LME (1<<_EFER_LME) @@ -95,9 +96,10 @@ #define EFER_NX (1<<_EFER_NX) #define EFER_SVME (1<<_EFER_SVME) #define EFER_FFXSE (1<<_EFER_FFXSE) +#define EFER_MCOMMIT (1<<_EFER_MCOMMIT) #define EFER_KNOWN_MASK (EFER_SCE | EFER_LME | EFER_LMA | EFER_NX | \ - EFER_SVME | EFER_FFXSE) + EFER_SVME | EFER_FFXSE | EFER_MCOMMIT) /* Intel MSRs. Some also available on other CPUs */ #define MSR_IA32_PERFCTR0 0x000000c1 --- a/xen/include/public/arch-x86/cpufeatureset.h +++ b/xen/include/public/arch-x86/cpufeatureset.h @@ -249,6 +249,7 @@ XEN_CPUFEATURE(EFRO, 7*32+10) / XEN_CPUFEATURE(CLZERO, 8*32+ 0) /*A CLZERO instruction */ XEN_CPUFEATURE(RSTR_FP_ERR_PTRS, 8*32+ 2) /*A (F)X{SAVE,RSTOR} always saves/restores FPU Error pointers */ XEN_CPUFEATURE(RDPRU, 8*32+ 4) /*A RDPRU instruction */ +XEN_CPUFEATURE(MCOMMIT, 8*32+ 8) /*A MCOMMIT instruction */ XEN_CPUFEATURE(WBNOINVD, 8*32+ 9) /* WBNOINVD instruction */ XEN_CPUFEATURE(IBPB, 8*32+12) /*A IBPB support only (no IBRS, used by AMD) */ XEN_CPUFEATURE(AMD_PPIN, 8*32+23) /* Protected Processor Inventory Number */
The dependency on a new EFER bit implies that we need to set that bit ourselves in order to be able to successfully invoke the insn. Also once again introduce the SVM related constants at this occasion. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- RFC: The exact meaning of the PM stating "any errors encountered by those stores have been signaled to associated error logging resources" is unclear. Depending on what this entails, blindly enabling EFER.MCOMMIT may not be a good idea. Hence the RFC. --- v5: Re-base. v4: New.