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(-)
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;
--
2.11.0
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
© 2016 - 2026 Red Hat, Inc.