[PATCH 0/2] x86/spec-ctrl: Reduce HVM RAS overhead

[PATCH 0/2] x86/spec-ctrl: Reduce HVM RAS overhead

[Andrew Cooper]

This is a optimsiation discovered while working on Retbleed.

Andrew Cooper (2):
  x86/svm: Remove regs param from asm-called functions
  x86/svm: Keep the RAS balanced for guests

 xen/arch/x86/hvm/svm/entry.S     | 58 ++++++++++++++++++++++++++++++++++++----
 xen/arch/x86/hvm/svm/nestedsvm.c |  3 ++-
 xen/arch/x86/hvm/svm/svm.c       |  6 +++--
 3 files changed, 59 insertions(+), 8 deletions(-)

-- 
2.11.0

[PATCH 1/2] x86/svm: Remove regs param from asm-called functions

[Andrew Cooper]

A optimisation is going to want to conditionally have extra data on the stack
around VMExit.

We could alternative between `mov %rsp, %rdi` and `lea 8(%rsp), %rdi`, but it
is easier just to make the functions void and let the compiler do the (not
very) hard work.

Passing regs is a bit weird for HVM guests anyway, because the resulting
pointer is invariant (this isn't native exception handling where the regs
pointers *are* important), and all functions calculate `current` themselves
which is another invariant.

Finally, the compiler can merge the get_cpu_info() calculation which is common
to both `current` and guest_cpu_user_regs(), meaning the delta in C really is
just one `lea`, and not any more expensive than `mov`'s in ASM anyway.

No functional change.

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/entry.S     | 3 ---
 xen/arch/x86/hvm/svm/nestedsvm.c | 3 ++-
 xen/arch/x86/hvm/svm/svm.c       | 6 ++++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/entry.S b/xen/arch/x86/hvm/svm/entry.S
index a60d759f7108..be4ce52bd81d 100644
--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/xen/arch/x86/hvm/svm/entry.S
@@ -26,7 +26,6 @@ ENTRY(svm_asm_do_resume)
         GET_CURRENT(bx)
 .Lsvm_do_resume:
         call svm_intr_assist
-        mov  %rsp,%rdi
         call nsvm_vcpu_switch
         ASSERT_NOT_IN_ATOMIC
 
@@ -52,7 +51,6 @@ UNLIKELY_START(ne, nsvm_hap)
         jmp  .Lsvm_do_resume
 __UNLIKELY_END(nsvm_hap)
 
-        mov  %rsp, %rdi
         call svm_vmenter_helper
 
         clgi
@@ -132,7 +130,6 @@ __UNLIKELY_END(nsvm_hap)
          */
         stgi
 GLOBAL(svm_stgi_label)
-        mov  %rsp,%rdi
         call svm_vmexit_handler
         jmp  .Lsvm_do_resume
 
diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index 9f5f35f16aff..77f754736023 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -1460,8 +1460,9 @@ nestedsvm_vcpu_vmexit(struct vcpu *v, struct cpu_user_regs *regs,
 }
 
 /* VCPU switch */
-void nsvm_vcpu_switch(struct cpu_user_regs *regs)
+void nsvm_vcpu_switch(void)
 {
+    struct cpu_user_regs *regs = guest_cpu_user_regs();
     struct vcpu *v = current;
     struct nestedvcpu *nv;
     struct nestedsvm *svm;
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 0849a9dc5f41..81f0cf55676b 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1040,8 +1040,9 @@ static void noreturn cf_check svm_do_resume(void)
     reset_stack_and_jump(svm_asm_do_resume);
 }
 
-void svm_vmenter_helper(const struct cpu_user_regs *regs)
+void svm_vmenter_helper(void)
 {
+    const struct cpu_user_regs *regs = guest_cpu_user_regs();
     struct vcpu *curr = current;
     struct vmcb_struct *vmcb = curr->arch.hvm.svm.vmcb;
 
@@ -2570,8 +2571,9 @@ static struct hvm_function_table __initdata_cf_clobber svm_function_table = {
     },
 };
 
-void svm_vmexit_handler(struct cpu_user_regs *regs)
+void svm_vmexit_handler(void)
 {
+    struct cpu_user_regs *regs = guest_cpu_user_regs();
     uint64_t exit_reason;
     struct vcpu *v = current;
     struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
-- 
2.11.0

Re: [PATCH 1/2] x86/svm: Remove regs param from asm-called functions

[Jan Beulich]

On 11.08.2022 21:59, Andrew Cooper wrote:
> A optimisation is going to want to conditionally have extra data on the stack
> around VMExit.
> 
> We could alternative between `mov %rsp, %rdi` and `lea 8(%rsp), %rdi`, but it
> is easier just to make the functions void and let the compiler do the (not
> very) hard work.
> 
> Passing regs is a bit weird for HVM guests anyway, because the resulting
> pointer is invariant (this isn't native exception handling where the regs
> pointers *are* important), and all functions calculate `current` themselves
> which is another invariant.
> 
> Finally, the compiler can merge the get_cpu_info() calculation which is common
> to both `current` and guest_cpu_user_regs(), meaning the delta in C really is
> just one `lea`, and not any more expensive than `mov`'s in ASM anyway.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

[PATCH 2/2] x86/svm: Keep the RAS balanced for guests

[Andrew Cooper]

One source of lost performance was that fact that to protect Xen from a
malicious guests, we had to flush the RAS.

It turns out that CET Shadow Stacks give us enough architectural guarantees to
construct a lower overhead mitigation, which keeps the RAS balanced for the
guest so their return performance is still good.

To keep the RAS balanced, Xen must execute the same number of CALLs as RETs
across one VMexit->VMEntry.  Without CET-SS, we could achieve this fairly
easily with a `call; add $8, %rsp` and `push; ret` pair, but this is not legal
under CET-SS.  In fact, CALL is the only shadow stack "push" operation we
have, and we can't use it a second time if we intend to keep the RAS balanced.

Instead, we keep a real return address on the stack.  This means that for some
of entry.S, %rsp conditionally doesn't reference CPUINFO.

This necessitates swapping the current order of DO_OVERWRITE_RSB and
svm_vmexit_spec_ctrl; while they don't have any specific ordering
requirements, push_one_ras needs to come after svm_vmexit_spec_ctrl or else we
need some very invasive changes to fix up the %rsp changes.

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>

RFC for a couple of reasons.  This does function correctly, but I still want
to do more perf testing.

Secondly, X86_FEATURE_ALWAYS is clearly not ok for committing.  I'm still
debating whether to make this construct available in !CET-SS cases.
Mechanically, its fine, but the safety arguments depend on CET-SS being
active.

In principle, on CPUs which do not suffer Branch Type Confusion, you might be
able to reason a defence-in-depth argument that if an attacker can't control
indirect speculation, then they can't bypass the 1-stuff safety either, but
the only AMD CPUs not vulnerable to BTC have CET-SS anyway.

Third, I'd like some early feedback on how clear it the logic is given the
conditional nature of %rsp not referencing CPUINFO.

Fourth, the alternatives logic (I think) needs improving to not fix up a
direct CALL/JMP displacement if the destination is within the replacement
length.  I did the functional testing before wrapping things in alternatives.
---
 xen/arch/x86/hvm/svm/entry.S | 55 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/entry.S b/xen/arch/x86/hvm/svm/entry.S
index be4ce52bd81d..98934db41fec 100644
--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/xen/arch/x86/hvm/svm/entry.S
@@ -22,8 +22,41 @@
 #include <asm/asm_defns.h>
 #include <asm/page.h>
 
+.macro push_one_ras
+        /*
+         * Pushes one entry into the RAS, then updates the return address(es)
+         * to point at svm_ras_speculation_trap.
+         *
+         * Rogue RAS-speculation will hit the INT3 and stop.  Architectural
+         * execution will go to svm_ras_speculation_trap.
+         *
+         * This deliberately leaves the (modified) return address on the
+         * stack(s).
+         */
+        call 1f
+        int3
+1:
+        lea     svm_ras_speculation_trap(%rip), %rax
+
+#ifdef CONFIG_XEN_SHSTK
+        rdsspq  %rcx
+        wrssq   %rax, (%rcx)
+#endif
+        mov     %rax, (%rsp)
+.endm
+
 ENTRY(svm_asm_do_resume)
         GET_CURRENT(bx)
+
+        /*
+         * We've just been schedule()'d.  There's no speculation safety needed
+         * here, but we do need to set the stack up in the manner expected by
+         * later logic.
+         */
+        ALTERNATIVE "", push_one_ras, X86_FEATURE_ALWAYS
+
+        /* WARNING!  After this point, %rsp /may/ not reference cpu_info. */
+
 .Lsvm_do_resume:
         call svm_intr_assist
         call nsvm_vcpu_switch
@@ -56,6 +89,20 @@ __UNLIKELY_END(nsvm_hap)
         clgi
 
         /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
+        /* WARNING!  Before this point, %rsp /may/ not reference cpu_info. */
+
+        /*
+         * If we're trying to balance the RAS for guests, push_one_ras in the
+         * VMExit path was necessary for speculative safety, but the on-stack
+         * return address was deliberately updated to point here.
+         *
+         * We execute one RET to re-balance the RAS.  It will mispredict (to
+         * the INT3 in push_one_ras in the general case), but won't
+         * architecturally change the instruction flow.
+         */
+        ALTERNATIVE "", ret, X86_FEATURE_ALWAYS
+svm_ras_speculation_trap:
+
         /* SPEC_CTRL_EXIT_TO_SVM       Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
         .macro svm_vmentry_spec_ctrl
             mov    VCPU_arch_msrs(%rbx), %rax
@@ -108,8 +155,6 @@ __UNLIKELY_END(nsvm_hap)
 	.endm
         ALTERNATIVE "", svm_vmexit_cond_ibpb, X86_FEATURE_IBPB_ENTRY_HVM
 
-        ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM
-
         .macro svm_vmexit_spec_ctrl
             movzbl CPUINFO_xen_spec_ctrl(%rsp), %eax
             movzbl CPUINFO_last_spec_ctrl(%rsp), %edx
@@ -122,6 +167,12 @@ __UNLIKELY_END(nsvm_hap)
 1:
         .endm
         ALTERNATIVE "", svm_vmexit_spec_ctrl, X86_FEATURE_SC_MSR_HVM
+
+        ALTERNATIVE_2 "", \
+             DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM,  \
+             push_one_ras, X86_FEATURE_ALWAYS
+
+        /* WARNING!  After this point, %rsp /may/ not reference cpu_info. */
         /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
 
         /*
-- 
2.11.0

Re: [PATCH 2/2] x86/svm: Keep the RAS balanced for guests

[Jan Beulich]

On 11.08.2022 21:59, Andrew Cooper wrote:
> One source of lost performance was that fact that to protect Xen from a
> malicious guests, we had to flush the RAS.
> 
> It turns out that CET Shadow Stacks give us enough architectural guarantees to
> construct a lower overhead mitigation, which keeps the RAS balanced for the
> guest so their return performance is still good.
> 
> To keep the RAS balanced, Xen must execute the same number of CALLs as RETs
> across one VMexit->VMEntry.  Without CET-SS, we could achieve this fairly
> easily with a `call; add $8, %rsp` and `push; ret` pair, but this is not legal
> under CET-SS.  In fact, CALL is the only shadow stack "push" operation we
> have, and we can't use it a second time if we intend to keep the RAS balanced.
> 
> Instead, we keep a real return address on the stack.  This means that for some
> of entry.S, %rsp conditionally doesn't reference CPUINFO.
> 
> This necessitates swapping the current order of DO_OVERWRITE_RSB and
> svm_vmexit_spec_ctrl; while they don't have any specific ordering
> requirements, push_one_ras needs to come after svm_vmexit_spec_ctrl or else we
> need some very invasive changes to fix up the %rsp changes.
> 
> 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>
> 
> RFC for a couple of reasons.  This does function correctly, but I still want
> to do more perf testing.

As per further down you mean to say it functions correctly without the
use of alternatives. And even then (see below) I suppose it doesn't
function correctly with no (or unused) CET-SS but CONFIG_XEN_SHSTK=y.

> Secondly, X86_FEATURE_ALWAYS is clearly not ok for committing.  I'm still
> debating whether to make this construct available in !CET-SS cases.
> Mechanically, its fine, but the safety arguments depend on CET-SS being
> active.

I'm afraid it's not entirely clear what you mean here, nor why you've used
X86_FEATURE_ALWAYS in the first place when we have X86_FEATURE_XEN_SHSTK.
If "this construct" is push_one_ras, then the mere use of WRSSQ requires
it to not be used based on a runtime characteristic, not just a build
time one. Hence afaict you could as well put the entire macro body in the
#ifdef that currently encloses only the CET-SS insns.

> In principle, on CPUs which do not suffer Branch Type Confusion, you might be
> able to reason a defence-in-depth argument that if an attacker can't control
> indirect speculation, then they can't bypass the 1-stuff safety either, but
> the only AMD CPUs not vulnerable to BTC have CET-SS anyway.

Yet people may have reasons to turn off its use.

> Third, I'd like some early feedback on how clear it the logic is given the
> conditional nature of %rsp not referencing CPUINFO.

It's assembly code, touching of which needs extra care. Taking together
the size of the entire file (quite small) and the comments you add, I'd
say that's fine.

> Fourth, the alternatives logic (I think) needs improving to not fix up a
> direct CALL/JMP displacement if the destination is within the replacement
> length.  I did the functional testing before wrapping things in alternatives.

Yes, unless you want to prefix the CALL with a redundant insn prefix to
hide it from the displacement adjustment logic.

Jan

Re: [PATCH 2/2] x86/svm: Keep the RAS balanced for guests

[Andrew Cooper]

On 12/08/2022 08:29, Jan Beulich wrote:
> On 11.08.2022 21:59, Andrew Cooper wrote:
>> One source of lost performance was that fact that to protect Xen from a
>> malicious guests, we had to flush the RAS.
>>
>> It turns out that CET Shadow Stacks give us enough architectural guarantees to
>> construct a lower overhead mitigation, which keeps the RAS balanced for the
>> guest so their return performance is still good.
>>
>> To keep the RAS balanced, Xen must execute the same number of CALLs as RETs
>> across one VMexit->VMEntry.  Without CET-SS, we could achieve this fairly
>> easily with a `call; add $8, %rsp` and `push; ret` pair, but this is not legal
>> under CET-SS.  In fact, CALL is the only shadow stack "push" operation we
>> have, and we can't use it a second time if we intend to keep the RAS balanced.
>>
>> Instead, we keep a real return address on the stack.  This means that for some
>> of entry.S, %rsp conditionally doesn't reference CPUINFO.
>>
>> This necessitates swapping the current order of DO_OVERWRITE_RSB and
>> svm_vmexit_spec_ctrl; while they don't have any specific ordering
>> requirements, push_one_ras needs to come after svm_vmexit_spec_ctrl or else we
>> need some very invasive changes to fix up the %rsp changes.
>>
>> 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>
>>
>> RFC for a couple of reasons.  This does function correctly, but I still want
>> to do more perf testing.
> As per further down you mean to say it functions correctly without the
> use of alternatives. And even then (see below) I suppose it doesn't
> function correctly with no (or unused) CET-SS but CONFIG_XEN_SHSTK=y.

I came to realise that after sending that, until we get nested
alternatives, I can't make it function correctly for the
conditionally-not-CET-SS case without doing the full mov/rdsspq/cmp
sequence.

While that's possible, I'm not inclined to add the size overhead for a
case I'm not sure is safe.

So, the alternative to turn this on is going to strictly depend on
CONFIG_XEN_SHSTK.

At some point when nested alternatives appear, we can relax the
requirement, if there is a desperate wish for the perf improvement in
uncertain safety conditions.

>
>> Secondly, X86_FEATURE_ALWAYS is clearly not ok for committing.  I'm still
>> debating whether to make this construct available in !CET-SS cases.
>> Mechanically, its fine, but the safety arguments depend on CET-SS being
>> active.
> I'm afraid it's not entirely clear what you mean here, nor why you've used
> X86_FEATURE_ALWAYS in the first place when we have X86_FEATURE_XEN_SHSTK.

It can't depend on X86_FEATURE_XEN_SHSTK because, like the PBRSB case on
Intel, I want a way for the user to switch back to stuff32 if it
subsequently turns out that I've screwed up in the safety reasoning.

> If "this construct" is push_one_ras, then the mere use of WRSSQ requires
> it to not be used based on a runtime characteristic, not just a build
> time one. Hence afaict you could as well put the entire macro body in the
> #ifdef that currently encloses only the CET-SS insns.

What I mean is that "this" is faster than stuff32, and might be
"acceptably safe" for someone either on BTC-vunerable hardware, or newer
hardware but with CET-SS explicitly turned off.

That said...

>> In principle, on CPUs which do not suffer Branch Type Confusion, you might be
>> able to reason a defence-in-depth argument that if an attacker can't control
>> indirect speculation, then they can't bypass the 1-stuff safety either, but
>> the only AMD CPUs not vulnerable to BTC have CET-SS anyway.
> Yet people may have reasons to turn off its use.

... I view this as a perf improvement only.

As such, I'm entirely happy to say that it's only available in the case
which is easy to do, ought to be the common case, and is the case which
I'm confident is safe.

If people want it in other cases too, they can see about helping nested
alternatives along...

>> Third, I'd like some early feedback on how clear it the logic is given the
>> conditional nature of %rsp not referencing CPUINFO.
> It's assembly code, touching of which needs extra care. Taking together
> the size of the entire file (quite small) and the comments you add, I'd
> say that's fine.

That's good.  It turned out to be far less invasive than I feared.

The major observation which allowed for this was that it doesn't matter
trying to keep the RAS balanced immediately after schedule, so we don't
need to play games with reset_stack_and_jump() to try and execute one
fewer CALL.

>> Fourth, the alternatives logic (I think) needs improving to not fix up a
>> direct CALL/JMP displacement if the destination is within the replacement
>> length.  I did the functional testing before wrapping things in alternatives.
> Yes, unless you want to prefix the CALL with a redundant insn prefix to
> hide it from the displacement adjustment logic.

Hmm, that should work, but is ugly.

Honestly, I think I'd prefer to take the take the time to start doing a
boot time self-test for alternatives, as pert the SMC testing
improvements, because we desperately need this for so many other reasons
too.

~Andrew