diff mbox series

[2/3] x86/svm: Enumeration for CET

Message ID 20210426175421.30497-3-andrew.cooper3@citrix.com (mailing list archive)
State New
Headers show
Series x86: Initial pieces for guest CET support | expand

Commit Message

Andrew Cooper April 26, 2021, 5:54 p.m. UTC
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(-)

Comments

Jan Beulich April 27, 2021, 3:53 p.m. UTC | #1
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
Andrew Cooper April 27, 2021, 5:47 p.m. UTC | #2
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
Jan Beulich April 28, 2021, 9:14 a.m. UTC | #3
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
Andrew Cooper April 28, 2021, 2:17 p.m. UTC | #4
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 mbox series

Patch

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;