Message ID | 20210426175421.30497-3-andrew.cooper3@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | x86: Initial pieces for guest CET support | expand |
On 26.04.2021 19:54, Andrew Cooper wrote: > On CET-capable hardware, VMRUN/EXIT unconditionally swaps S_SET, SSP and Nit: S_CET? > ISST (subject to cleanbits) without further settings. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Jan Beulich <jbeulich@suse.com> with one question: > @@ -497,7 +501,9 @@ struct vmcb_struct { > u64 rip; > u64 res14[11]; > u64 rsp; > - u64 res15[3]; > + u64 _msr_s_cet; /* offset 0x400 + 0x1E0 - cleanbit 12 */ > + u64 _ssp; /* offset 0x400 + 0x1E8 | */ > + u64 _msr_isst; /* offset 0x400 + 0x1F0 v */ > u64 rax; > u64 star; > u64 lstar; Any reason for the leading underscores, when none of the neighboring fields have such? Did you perhaps mean to add VMCB_ACCESSORS() instances for them? (Ack remains in effect if you decide to do so.) Jan
On 27/04/2021 16:53, Jan Beulich wrote: > On 26.04.2021 19:54, Andrew Cooper wrote: >> On CET-capable hardware, VMRUN/EXIT unconditionally swaps S_SET, SSP and > Nit: S_CET? Ah yes. > >> ISST (subject to cleanbits) without further settings. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Acked-by: Jan Beulich <jbeulich@suse.com> Thanks. > with one question: > >> @@ -497,7 +501,9 @@ struct vmcb_struct { >> u64 rip; >> u64 res14[11]; >> u64 rsp; >> - u64 res15[3]; >> + u64 _msr_s_cet; /* offset 0x400 + 0x1E0 - cleanbit 12 */ >> + u64 _ssp; /* offset 0x400 + 0x1E8 | */ >> + u64 _msr_isst; /* offset 0x400 + 0x1F0 v */ >> u64 rax; >> u64 star; >> u64 lstar; > Any reason for the leading underscores, when none of the neighboring > fields have such? Yes - they're covered by a cleanbit, and for better or worse, this is our style. > Did you perhaps mean to add VMCB_ACCESSORS() > instances for them? TBH, I opencoded the cleanbit handling because I thoroughly hate that entire infrastructure. I ought to add lines, but I'm still very tempted to rip it all up and implement something which is less obfuscating. ~Andrew
On 27.04.2021 19:47, Andrew Cooper wrote: > On 27/04/2021 16:53, Jan Beulich wrote: >> On 26.04.2021 19:54, Andrew Cooper wrote: >>> @@ -497,7 +501,9 @@ struct vmcb_struct { >>> u64 rip; >>> u64 res14[11]; >>> u64 rsp; >>> - u64 res15[3]; >>> + u64 _msr_s_cet; /* offset 0x400 + 0x1E0 - cleanbit 12 */ >>> + u64 _ssp; /* offset 0x400 + 0x1E8 | */ >>> + u64 _msr_isst; /* offset 0x400 + 0x1F0 v */ >>> u64 rax; >>> u64 star; >>> u64 lstar; >> Any reason for the leading underscores, when none of the neighboring >> fields have such? > > Yes - they're covered by a cleanbit, and for better or worse, this is > our style. The underscore prefixes are, to my understanding, there only to emphasize that the fields shouldn't be accessed directly, but ... >> Did you perhaps mean to add VMCB_ACCESSORS() >> instances for them? > > TBH, I opencoded the cleanbit handling because I thoroughly hate that > entire infrastructure. ... via this (or something with similar abstracting effect). So for any fields you mean to access directly they imo shouldn't be there. I particularly don't view them as indicators of being covered by cleanbits (if the respective accessors aren't used). Jan
On 28/04/2021 10:14, Jan Beulich wrote: > On 27.04.2021 19:47, Andrew Cooper wrote: >> On 27/04/2021 16:53, Jan Beulich wrote: >>> On 26.04.2021 19:54, Andrew Cooper wrote: >>>> @@ -497,7 +501,9 @@ struct vmcb_struct { >>>> u64 rip; >>>> u64 res14[11]; >>>> u64 rsp; >>>> - u64 res15[3]; >>>> + u64 _msr_s_cet; /* offset 0x400 + 0x1E0 - cleanbit 12 */ >>>> + u64 _ssp; /* offset 0x400 + 0x1E8 | */ >>>> + u64 _msr_isst; /* offset 0x400 + 0x1F0 v */ >>>> u64 rax; >>>> u64 star; >>>> u64 lstar; >>> Any reason for the leading underscores, when none of the neighboring >>> fields have such? >> Yes - they're covered by a cleanbit, and for better or worse, this is >> our style. > The underscore prefixes are, to my understanding, there only to > emphasize that the fields shouldn't be accessed directly, but ... > >>> Did you perhaps mean to add VMCB_ACCESSORS() >>> instances for them? >> TBH, I opencoded the cleanbit handling because I thoroughly hate that >> entire infrastructure. > ... via this (or something with similar abstracting effect). So > for any fields you mean to access directly they imo shouldn't be > there. I particularly don't view them as indicators of being > covered by cleanbits (if the respective accessors aren't used). The leading underscores are enforced by 'vmcb->_ ## name' in VMCB_ACCESSORS(). The cleanbits are the only reason we can't treat this as a simple struct. ~Andrew
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index 4585efe1f8..642a64b747 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -1658,6 +1658,7 @@ const struct hvm_function_table * __init start_svm(void) P(cpu_has_pause_filter, "Pause-Intercept Filter"); P(cpu_has_pause_thresh, "Pause-Intercept Filter Threshold"); P(cpu_has_tsc_ratio, "TSC Rate MSR"); + P(cpu_has_svm_sss, "NPT Supervisor Shadow Stack"); #undef P if ( !printed ) diff --git a/xen/arch/x86/hvm/svm/svmdebug.c b/xen/arch/x86/hvm/svm/svmdebug.c index f450391df4..bce86f0ef7 100644 --- a/xen/arch/x86/hvm/svm/svmdebug.c +++ b/xen/arch/x86/hvm/svm/svmdebug.c @@ -82,6 +82,8 @@ void svm_vmcb_dump(const char *from, const struct vmcb_struct *vmcb) vmcb->cstar, vmcb->sfmask); printk("KernGSBase = 0x%016"PRIx64" PAT = 0x%016"PRIx64"\n", vmcb->kerngsbase, vmcb_get_g_pat(vmcb)); + printk("SSP = 0x%016"PRIx64" S_CET = 0x%016"PRIx64" ISST = 0x%016"PRIx64"\n", + vmcb->_ssp, vmcb->_msr_s_cet, vmcb->_msr_isst); printk("H_CR3 = 0x%016"PRIx64" CleanBits = %#x\n", vmcb_get_h_cr3(vmcb), vmcb->cleanbits.raw); diff --git a/xen/include/asm-x86/hvm/svm/svm.h b/xen/include/asm-x86/hvm/svm/svm.h index faeca40174..bee939156f 100644 --- a/xen/include/asm-x86/hvm/svm/svm.h +++ b/xen/include/asm-x86/hvm/svm/svm.h @@ -75,6 +75,7 @@ extern u32 svm_feature_flags; #define SVM_FEATURE_PAUSETHRESH 12 /* Pause intercept filter support */ #define SVM_FEATURE_VLOADSAVE 15 /* virtual vmload/vmsave */ #define SVM_FEATURE_VGIF 16 /* Virtual GIF */ +#define SVM_FEATURE_SSS 19 /* NPT Supervisor Shadow Stacks */ #define cpu_has_svm_feature(f) (svm_feature_flags & (1u << (f))) #define cpu_has_svm_npt cpu_has_svm_feature(SVM_FEATURE_NPT) @@ -89,6 +90,7 @@ extern u32 svm_feature_flags; #define cpu_has_pause_thresh cpu_has_svm_feature(SVM_FEATURE_PAUSETHRESH) #define cpu_has_tsc_ratio cpu_has_svm_feature(SVM_FEATURE_TSCRATEMSR) #define cpu_has_svm_vloadsave cpu_has_svm_feature(SVM_FEATURE_VLOADSAVE) +#define cpu_has_svm_sss cpu_has_svm_feature(SVM_FEATURE_SSS) #define SVM_PAUSEFILTER_INIT 4000 #define SVM_PAUSETHRESH_INIT 1000 diff --git a/xen/include/asm-x86/hvm/svm/vmcb.h b/xen/include/asm-x86/hvm/svm/vmcb.h index 0b03a8f076..fbedea209e 100644 --- a/xen/include/asm-x86/hvm/svm/vmcb.h +++ b/xen/include/asm-x86/hvm/svm/vmcb.h @@ -248,6 +248,8 @@ enum VMEXIT_EXITCODE VMEXIT_EXCEPTION_AC = 81, /* 0x51, alignment-check */ VMEXIT_EXCEPTION_MC = 82, /* 0x52, machine-check */ VMEXIT_EXCEPTION_XF = 83, /* 0x53, simd floating-point */ +/* VMEXIT_EXCEPTION_20 = 84, 0x54, #VE (Intel specific) */ + VMEXIT_EXCEPTION_CP = 85, /* 0x55, controlflow protection */ /* exceptions 20-31 (exitcodes 84-95) are reserved */ @@ -397,6 +399,8 @@ typedef union bool seg:1; /* 8: cs, ds, es, ss, cpl */ bool cr2:1; /* 9: cr2 */ bool lbr:1; /* 10: debugctlmsr, last{branch,int}{to,from}ip */ + bool :1; + bool cet:1; /* 12: msr_s_set, ssp, msr_isst */ }; uint32_t raw; } vmcbcleanbits_t; @@ -451,7 +455,7 @@ struct vmcb_struct { bool _sev_enable :1; bool _sev_es_enable :1; bool _gmet :1; - bool :1; + bool _np_sss :1; bool _vte :1; }; uint64_t _np_ctrl; @@ -497,7 +501,9 @@ struct vmcb_struct { u64 rip; u64 res14[11]; u64 rsp; - u64 res15[3]; + u64 _msr_s_cet; /* offset 0x400 + 0x1E0 - cleanbit 12 */ + u64 _ssp; /* offset 0x400 + 0x1E8 | */ + u64 _msr_isst; /* offset 0x400 + 0x1F0 v */ u64 rax; u64 star; u64 lstar;
On CET-capable hardware, VMRUN/EXIT unconditionally swaps S_SET, SSP and ISST (subject to cleanbits) without further settings. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> --- xen/arch/x86/hvm/svm/svm.c | 1 + xen/arch/x86/hvm/svm/svmdebug.c | 2 ++ xen/include/asm-x86/hvm/svm/svm.h | 2 ++ xen/include/asm-x86/hvm/svm/vmcb.h | 10 ++++++++-- 4 files changed, 13 insertions(+), 2 deletions(-)