In order to be able to defer the context switch IBPB to the last
possible point, add logic to the exit-to-guest paths to issue the
barrier there, including the "IBPB doesn't flush the RSB/RAS"
workaround. Since alternatives, for now at least, can't nest, emit JMP
to skip past both constructs where both are needed. This may be more
efficient anyway, as the sequence of NOPs is pretty long.
LFENCEs are omitted - for HVM a VM entry is immanent, which already
elsewhere we deem sufficiently serializing an event. For 32-bit PV
we're going through IRET, which ought to be good enough as well. While
64-bit PV may use SYSRET, there are several more conditional branches
there which are all unprotected.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I have to admit that I'm not really certain about the placement of the
IBPB wrt the MSR_SPEC_CTRL writes. For now I've simply used "opposite of
entry".
Since we're going to run out of SCF_* bits soon and since the new flag
is meaningful only in struct cpu_info's spec_ctrl_flags, we could choose
to widen that field to 16 bits right away and then use bit 8 (or higher)
for the purpose here.
---
v3: New.
--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/xen/arch/x86/hvm/svm/entry.S
@@ -75,6 +75,12 @@ __UNLIKELY_END(nsvm_hap)
.endm
ALTERNATIVE "", svm_vmentry_spec_ctrl, X86_FEATURE_SC_MSR_HVM
+ ALTERNATIVE "jmp 2f", __stringify(DO_SPEC_CTRL_EXIT_IBPB disp=(2f-1f)), \
+ X86_FEATURE_IBPB_EXIT_HVM
+1:
+ ALTERNATIVE "", DO_OVERWRITE_RSB, X86_BUG_IBPB_NO_RET
+2:
+
pop %r15
pop %r14
pop %r13
--- a/xen/arch/x86/hvm/vmx/entry.S
+++ b/xen/arch/x86/hvm/vmx/entry.S
@@ -86,7 +86,8 @@ UNLIKELY_END(realmode)
jz .Lvmx_vmentry_restart
/* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
- /* SPEC_CTRL_EXIT_TO_VMX Req: %rsp=regs/cpuinfo Clob: */
+ /* SPEC_CTRL_EXIT_TO_VMX Req: %rsp=regs/cpuinfo Clob: acd */
+ ALTERNATIVE "", DO_SPEC_CTRL_EXIT_IBPB, X86_FEATURE_IBPB_EXIT_HVM
DO_SPEC_CTRL_COND_VERW
mov VCPU_hvm_guest_cr2(%rbx),%rax
--- a/xen/arch/x86/include/asm/cpufeatures.h
+++ b/xen/arch/x86/include/asm/cpufeatures.h
@@ -39,8 +39,10 @@ XEN_CPUFEATURE(XEN_LBR, X86_SY
XEN_CPUFEATURE(SC_VERW_IDLE, X86_SYNTH(25)) /* VERW used by Xen for idle */
XEN_CPUFEATURE(XEN_SHSTK, X86_SYNTH(26)) /* Xen uses CET Shadow Stacks */
XEN_CPUFEATURE(XEN_IBT, X86_SYNTH(27)) /* Xen uses CET Indirect Branch Tracking */
-XEN_CPUFEATURE(IBPB_ENTRY_PV, X86_SYNTH(28)) /* MSR_PRED_CMD used by Xen for PV */
-XEN_CPUFEATURE(IBPB_ENTRY_HVM, X86_SYNTH(29)) /* MSR_PRED_CMD used by Xen for HVM */
+XEN_CPUFEATURE(IBPB_ENTRY_PV, X86_SYNTH(28)) /* MSR_PRED_CMD used by Xen when entered from PV */
+XEN_CPUFEATURE(IBPB_ENTRY_HVM, X86_SYNTH(29)) /* MSR_PRED_CMD used by Xen when entered from HVM */
+XEN_CPUFEATURE(IBPB_EXIT_PV, X86_SYNTH(30)) /* MSR_PRED_CMD used by Xen when exiting to PV */
+XEN_CPUFEATURE(IBPB_EXIT_HVM, X86_SYNTH(31)) /* MSR_PRED_CMD used by Xen when exiting to HVM */
/* Bug words follow the synthetic words. */
#define X86_NR_BUG 1
--- a/xen/arch/x86/include/asm/current.h
+++ b/xen/arch/x86/include/asm/current.h
@@ -55,9 +55,13 @@ struct cpu_info {
/* See asm/spec_ctrl_asm.h for usage. */
unsigned int shadow_spec_ctrl;
+ /*
+ * spec_ctrl_flags can be accessed as a 32-bit entity and hence needs
+ * placing suitably.
+ */
+ uint8_t spec_ctrl_flags;
uint8_t xen_spec_ctrl;
uint8_t last_spec_ctrl;
- uint8_t spec_ctrl_flags;
/*
* The following field controls copying of the L4 page table of 64-bit
--- a/xen/arch/x86/include/asm/spec_ctrl.h
+++ b/xen/arch/x86/include/asm/spec_ctrl.h
@@ -36,6 +36,8 @@
#define SCF_verw (1 << 3)
#define SCF_ist_ibpb (1 << 4)
#define SCF_entry_ibpb (1 << 5)
+#define SCF_exit_ibpb_bit 6
+#define SCF_exit_ibpb (1 << SCF_exit_ibpb_bit)
/*
* The IST paths (NMI/#MC) can interrupt any arbitrary context. Some
--- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
+++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
@@ -117,6 +117,27 @@
.L\@_done:
.endm
+.macro DO_SPEC_CTRL_EXIT_IBPB disp=0
+/*
+ * Requires %rsp=regs
+ * Clobbers %rax, %rcx, %rdx
+ *
+ * Conditionally issue IBPB if SCF_exit_ibpb is active. The macro invocation
+ * may be followed by X86_BUG_IBPB_NO_RET workaround code. The "disp" argument
+ * is to allow invocation sites to pass in the extra amount of code which needs
+ * skipping in case no action is necessary.
+ *
+ * The flag is a "one-shot" indicator, so it is being cleared at the same time.
+ */
+ btrl $SCF_exit_ibpb_bit, CPUINFO_spec_ctrl_flags(%rsp)
+ jnc .L\@_skip + (\disp)
+ mov $MSR_PRED_CMD, %ecx
+ mov $PRED_CMD_IBPB, %eax
+ xor %edx, %edx
+ wrmsr
+.L\@_skip:
+.endm
+
.macro DO_OVERWRITE_RSB tmp=rax
/*
* Requires nothing
@@ -272,6 +293,14 @@
#define SPEC_CTRL_EXIT_TO_PV \
ALTERNATIVE "", \
DO_SPEC_CTRL_EXIT_TO_GUEST, X86_FEATURE_SC_MSR_PV; \
+ ALTERNATIVE __stringify(jmp PASTE(.Lscexitpv_done, __LINE__)), \
+ __stringify(DO_SPEC_CTRL_EXIT_IBPB \
+ disp=(PASTE(.Lscexitpv_done, __LINE__) - \
+ PASTE(.Lscexitpv_rsb, __LINE__))), \
+ X86_FEATURE_IBPB_EXIT_PV; \
+PASTE(.Lscexitpv_rsb, __LINE__): \
+ ALTERNATIVE "", DO_OVERWRITE_RSB, X86_BUG_IBPB_NO_RET; \
+PASTE(.Lscexitpv_done, __LINE__): \
DO_SPEC_CTRL_COND_VERW
/*
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -8,6 +8,7 @@
#include <asm/page.h>
#include <asm/processor.h>
#include <asm/desc.h>
+#include <xen/lib.h>
#include <public/xen.h>
#include <irq_vectors.h>
@@ -156,7 +157,7 @@ ENTRY(compat_restore_all_guest)
mov VCPUMSR_spec_ctrl_raw(%rax), %eax
/* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
- SPEC_CTRL_EXIT_TO_PV /* Req: a=spec_ctrl %rsp=regs/cpuinfo, Clob: cd */
+ SPEC_CTRL_EXIT_TO_PV /* Req: a=spec_ctrl %rsp=regs/cpuinfo, Clob: acd */
RESTORE_ALL adj=8 compat=1
.Lft0: iretq
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -9,6 +9,7 @@
#include <asm/asm_defns.h>
#include <asm/page.h>
#include <asm/processor.h>
+#include <xen/lib.h>
#include <public/xen.h>
#include <irq_vectors.h>
@@ -187,7 +188,7 @@ restore_all_guest:
mov %r15d, %eax
/* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
- SPEC_CTRL_EXIT_TO_PV /* Req: a=spec_ctrl %rsp=regs/cpuinfo, Clob: cd */
+ SPEC_CTRL_EXIT_TO_PV /* Req: a=spec_ctrl %rsp=regs/cpuinfo, Clob: acd */
RESTORE_ALL
testw $TRAP_syscall,4(%rsp)On 25/01/2023 3:25 pm, Jan Beulich wrote:
> In order to be able to defer the context switch IBPB to the last
> possible point, add logic to the exit-to-guest paths to issue the
> barrier there, including the "IBPB doesn't flush the RSB/RAS"
> workaround. Since alternatives, for now at least, can't nest, emit JMP
> to skip past both constructs where both are needed. This may be more
> efficient anyway, as the sequence of NOPs is pretty long.
It is very uarch specific as to when a jump is less overhead than a line
of nops.
In all CPUs liable to be running Xen, even unconditional jumps take up
branch prediction resource, because all branch prediction is pre-decode
these days, so branch locations/types/destinations all need deriving
from %rip and "history" alone.
So whether a branch or a line of nops is better is a tradeoff between
how much competition there is for branch prediction resource, and how
efficiently the CPU can brute-force its way through a long line of nops.
But a very interesting datapoint. It turns out that AMD Zen4 CPUs
macrofuse adjacent nops, including longnops, because it reduces the
amount of execute/retire resources required. And a lot of
kernel/hypervisor fastpaths have a lot of nops these days.
For us, the "can't nest" is singularly more important than any worry
about uarch behaviour. We've frankly got much lower hanging fruit than
worring about one branch vs a few nops.
> LFENCEs are omitted - for HVM a VM entry is immanent, which already
> elsewhere we deem sufficiently serializing an event. For 32-bit PV
> we're going through IRET, which ought to be good enough as well. While
> 64-bit PV may use SYSRET, there are several more conditional branches
> there which are all unprotected.
Privilege changes are serialsing-ish, and this behaviour has been
guaranteed moving forwards, although not documented coherently.
CPL (well - privilege, which includes SMM, root/non-root, etc) is not
written speculatively. So any logic which needs to modify privilege has
to block until it is known to be an architectural execution path.
This gets us "lfence-like" or "dispatch serialising" behaviour, which is
also the reason why INT3 is our go-to speculation halting instruction.
Microcode has to be entirely certain we are going to deliver an
interrupt/exception/etc before it can start reading the IDT/etc.
Either way, we've been promised that all instructions like IRET,
SYS{CALL,RET,ENTER,EXIT}, VM{RUN,LAUNCH,RESUME} (and ERET{U,S} in the
future FRED world) do, and shall continue to not execute speculatively.
Which in practice means we don't need to worry about Spectre-v1 attack
against codepaths which hit an exit-from-xen path, in terms of skipping
protections.
We do need to be careful about memory accesses and potential double
dereferences, but all the data is on the top of the stack for XPTI
reasons. About the only concern is v->arch.msrs->* in the HVM path, and
we're fine with the current layout (AFAICT).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I have to admit that I'm not really certain about the placement of the
> IBPB wrt the MSR_SPEC_CTRL writes. For now I've simply used "opposite of
> entry".
It really doesn't matter. They're independent operations that both need
doing, and are fully serialising so can't parallelise.
But on this note, WRMSRNS and WRMSRLIST are on the horizon. The CPUs
which implement these instructions are the ones which also ought not to
need any adjustments in the exit paths. So I think it is specifically
not worth trying to make any effort to turn *these* WRMSR's into more
optimised forms.
But WRMSRLIST was designed specifically for this kind of usecase
(actually, more for the main context switch path) where you can prepare
the list of MSRs in memory, including the ability to conditionally skip
certain entries by adjusting the index field.
It occurs to me, having written this out, is that what we actually want
to do is have slightly custom not-quite-alternative blocks. We have a
sequence of independent code blocks, and a small block at the end that
happens to contain an IRET.
We could remove the nops at boot time if we treated it as one large
region, with the IRET at the end also able to have a variable position,
and assembles the "active" blocks tightly from the start. Complications
would include adjusting the IRET extable entry, but this isn't
insurmountable. Entrypoints are a bit more tricky but could be done by
packing from the back forward, and adjusting the entry position.
Either way, something to ponder. (It's also possible that it doesn't
make a measurable difference until we get to FRED, at which point we
have a set of fresh entry-points to write anyway, and a slight glimmer
of hope of not needing to pollute them with speculation workarounds...)
> Since we're going to run out of SCF_* bits soon and since the new flag
> is meaningful only in struct cpu_info's spec_ctrl_flags, we could choose
> to widen that field to 16 bits right away and then use bit 8 (or higher)
> for the purpose here.
I really don't think it matters. We've got plenty of room, and the
flexibility to shuffle, in both structures. It's absolutely not worth
trying to introduce asymmetries to save 1 bit.
> --- a/xen/arch/x86/include/asm/current.h
> +++ b/xen/arch/x86/include/asm/current.h
> @@ -55,9 +55,13 @@ struct cpu_info {
>
> /* See asm/spec_ctrl_asm.h for usage. */
> unsigned int shadow_spec_ctrl;
> + /*
> + * spec_ctrl_flags can be accessed as a 32-bit entity and hence needs
> + * placing suitably.
I'd suggest "is accessed as a 32-bit entity, and wants aligning suitably" ?
If I've followed the logic correctly. (I can't say I was specifically
aware that the bit test instructions didn't have byte forms, but I
suspect such instruction forms would be very very niche.)
> + */
> + uint8_t spec_ctrl_flags;
> uint8_t xen_spec_ctrl;
> uint8_t last_spec_ctrl;
> - uint8_t spec_ctrl_flags;
>
> /*
> * The following field controls copying of the L4 page table of 64-bit
> --- a/xen/arch/x86/include/asm/spec_ctrl.h
> +++ b/xen/arch/x86/include/asm/spec_ctrl.h
> @@ -36,6 +36,8 @@
> #define SCF_verw (1 << 3)
> #define SCF_ist_ibpb (1 << 4)
> #define SCF_entry_ibpb (1 << 5)
> +#define SCF_exit_ibpb_bit 6
> +#define SCF_exit_ibpb (1 << SCF_exit_ibpb_bit)
One option to avoid the second define is to use ILOG2() with btrl.
Of all the common forms of doing this, its the only one I'm aware of
which avoids needing the second define.
>
> /*
> * The IST paths (NMI/#MC) can interrupt any arbitrary context. Some
> --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
> +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
> @@ -117,6 +117,27 @@
> .L\@_done:
> .endm
>
> +.macro DO_SPEC_CTRL_EXIT_IBPB disp=0
> +/*
> + * Requires %rsp=regs
> + * Clobbers %rax, %rcx, %rdx
> + *
> + * Conditionally issue IBPB if SCF_exit_ibpb is active. The macro invocation
> + * may be followed by X86_BUG_IBPB_NO_RET workaround code. The "disp" argument
> + * is to allow invocation sites to pass in the extra amount of code which needs
> + * skipping in case no action is necessary.
> + *
> + * The flag is a "one-shot" indicator, so it is being cleared at the same time.
> + */
> + btrl $SCF_exit_ibpb_bit, CPUINFO_spec_ctrl_flags(%rsp)
> + jnc .L\@_skip + (\disp)
> + mov $MSR_PRED_CMD, %ecx
> + mov $PRED_CMD_IBPB, %eax
> + xor %edx, %edx
> + wrmsr
> +.L\@_skip:
> +.endm
> +
> .macro DO_OVERWRITE_RSB tmp=rax
> /*
> * Requires nothing
> @@ -272,6 +293,14 @@
> #define SPEC_CTRL_EXIT_TO_PV \
> ALTERNATIVE "", \
> DO_SPEC_CTRL_EXIT_TO_GUEST, X86_FEATURE_SC_MSR_PV; \
> + ALTERNATIVE __stringify(jmp PASTE(.Lscexitpv_done, __LINE__)), \
> + __stringify(DO_SPEC_CTRL_EXIT_IBPB \
> + disp=(PASTE(.Lscexitpv_done, __LINE__) - \
> + PASTE(.Lscexitpv_rsb, __LINE__))), \
> + X86_FEATURE_IBPB_EXIT_PV; \
> +PASTE(.Lscexitpv_rsb, __LINE__): \
> + ALTERNATIVE "", DO_OVERWRITE_RSB, X86_BUG_IBPB_NO_RET; \
> +PASTE(.Lscexitpv_done, __LINE__): \
> DO_SPEC_CTRL_COND_VERW
What's wrong with the normal %= trick? The use of __LINE__ makes this
hard to subsequently livepatch, so I'd prefer to avoid it if possible.
~Andrew
On 25.01.2023 22:10, Andrew Cooper wrote:
> On 25/01/2023 3:25 pm, Jan Beulich wrote:
>> In order to be able to defer the context switch IBPB to the last
>> possible point, add logic to the exit-to-guest paths to issue the
>> barrier there, including the "IBPB doesn't flush the RSB/RAS"
>> workaround. Since alternatives, for now at least, can't nest, emit JMP
>> to skip past both constructs where both are needed. This may be more
>> efficient anyway, as the sequence of NOPs is pretty long.
>
> It is very uarch specific as to when a jump is less overhead than a line
> of nops.
>
> In all CPUs liable to be running Xen, even unconditional jumps take up
> branch prediction resource, because all branch prediction is pre-decode
> these days, so branch locations/types/destinations all need deriving
> from %rip and "history" alone.
>
> So whether a branch or a line of nops is better is a tradeoff between
> how much competition there is for branch prediction resource, and how
> efficiently the CPU can brute-force its way through a long line of nops.
>
> But a very interesting datapoint. It turns out that AMD Zen4 CPUs
> macrofuse adjacent nops, including longnops, because it reduces the
> amount of execute/retire resources required. And a lot of
> kernel/hypervisor fastpaths have a lot of nops these days.
>
>
> For us, the "can't nest" is singularly more important than any worry
> about uarch behaviour. We've frankly got much lower hanging fruit than
> worring about one branch vs a few nops.
>
>> LFENCEs are omitted - for HVM a VM entry is immanent, which already
>> elsewhere we deem sufficiently serializing an event. For 32-bit PV
>> we're going through IRET, which ought to be good enough as well. While
>> 64-bit PV may use SYSRET, there are several more conditional branches
>> there which are all unprotected.
>
> Privilege changes are serialsing-ish, and this behaviour has been
> guaranteed moving forwards, although not documented coherently.
>
> CPL (well - privilege, which includes SMM, root/non-root, etc) is not
> written speculatively. So any logic which needs to modify privilege has
> to block until it is known to be an architectural execution path.
>
> This gets us "lfence-like" or "dispatch serialising" behaviour, which is
> also the reason why INT3 is our go-to speculation halting instruction.
> Microcode has to be entirely certain we are going to deliver an
> interrupt/exception/etc before it can start reading the IDT/etc.
>
> Either way, we've been promised that all instructions like IRET,
> SYS{CALL,RET,ENTER,EXIT}, VM{RUN,LAUNCH,RESUME} (and ERET{U,S} in the
> future FRED world) do, and shall continue to not execute speculatively.
>
> Which in practice means we don't need to worry about Spectre-v1 attack
> against codepaths which hit an exit-from-xen path, in terms of skipping
> protections.
>
> We do need to be careful about memory accesses and potential double
> dereferences, but all the data is on the top of the stack for XPTI
> reasons. About the only concern is v->arch.msrs->* in the HVM path, and
> we're fine with the current layout (AFAICT).
>
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> I have to admit that I'm not really certain about the placement of the
>> IBPB wrt the MSR_SPEC_CTRL writes. For now I've simply used "opposite of
>> entry".
>
> It really doesn't matter. They're independent operations that both need
> doing, and are fully serialising so can't parallelise.
>
> But on this note, WRMSRNS and WRMSRLIST are on the horizon. The CPUs
> which implement these instructions are the ones which also ought not to
> need any adjustments in the exit paths. So I think it is specifically
> not worth trying to make any effort to turn *these* WRMSR's into more
> optimised forms.
>
> But WRMSRLIST was designed specifically for this kind of usecase
> (actually, more for the main context switch path) where you can prepare
> the list of MSRs in memory, including the ability to conditionally skip
> certain entries by adjusting the index field.
>
>
> It occurs to me, having written this out, is that what we actually want
> to do is have slightly custom not-quite-alternative blocks. We have a
> sequence of independent code blocks, and a small block at the end that
> happens to contain an IRET.
>
> We could remove the nops at boot time if we treated it as one large
> region, with the IRET at the end also able to have a variable position,
> and assembles the "active" blocks tightly from the start. Complications
> would include adjusting the IRET extable entry, but this isn't
> insurmountable. Entrypoints are a bit more tricky but could be done by
> packing from the back forward, and adjusting the entry position.
>
> Either way, something to ponder. (It's also possible that it doesn't
> make a measurable difference until we get to FRED, at which point we
> have a set of fresh entry-points to write anyway, and a slight glimmer
> of hope of not needing to pollute them with speculation workarounds...)
>
>> Since we're going to run out of SCF_* bits soon and since the new flag
>> is meaningful only in struct cpu_info's spec_ctrl_flags, we could choose
>> to widen that field to 16 bits right away and then use bit 8 (or higher)
>> for the purpose here.
>
> I really don't think it matters. We've got plenty of room, and the
> flexibility to shuffle, in both structures. It's absolutely not worth
> trying to introduce asymmetries to save 1 bit.
Thanks for all the comments up to here. Just to clarify - I've not spotted
anything there that would result in me being expected to take any action.
>> --- a/xen/arch/x86/include/asm/current.h
>> +++ b/xen/arch/x86/include/asm/current.h
>> @@ -55,9 +55,13 @@ struct cpu_info {
>>
>> /* See asm/spec_ctrl_asm.h for usage. */
>> unsigned int shadow_spec_ctrl;
>> + /*
>> + * spec_ctrl_flags can be accessed as a 32-bit entity and hence needs
>> + * placing suitably.
>
> I'd suggest "is accessed as a 32-bit entity, and wants aligning suitably" ?
I've tried to choose the wording carefully: The 32-bit access is in an
alternative block, so doesn't always come into play. Hence the "may",
not "is". Alignment alone also isn't sufficient here (and mis-aligning
isn't merely a performance problem) - the following three bytes also
need to be valid to access in the first place. Hence "needs" and
"placing", not "wants" and "aligning".
> If I've followed the logic correctly. (I can't say I was specifically
> aware that the bit test instructions didn't have byte forms, but I
> suspect such instruction forms would be very very niche.)
Yes, there not being byte forms of BT* is the sole reason here.
>> --- a/xen/arch/x86/include/asm/spec_ctrl.h
>> +++ b/xen/arch/x86/include/asm/spec_ctrl.h
>> @@ -36,6 +36,8 @@
>> #define SCF_verw (1 << 3)
>> #define SCF_ist_ibpb (1 << 4)
>> #define SCF_entry_ibpb (1 << 5)
>> +#define SCF_exit_ibpb_bit 6
>> +#define SCF_exit_ibpb (1 << SCF_exit_ibpb_bit)
>
> One option to avoid the second define is to use ILOG2() with btrl.
Specifically not. The assembler doesn't know the conditional operator,
and the pre-processor won't collapse the expression resulting from
expanding ilog2().
>> @@ -272,6 +293,14 @@
>> #define SPEC_CTRL_EXIT_TO_PV \
>> ALTERNATIVE "", \
>> DO_SPEC_CTRL_EXIT_TO_GUEST, X86_FEATURE_SC_MSR_PV; \
>> + ALTERNATIVE __stringify(jmp PASTE(.Lscexitpv_done, __LINE__)), \
>> + __stringify(DO_SPEC_CTRL_EXIT_IBPB \
>> + disp=(PASTE(.Lscexitpv_done, __LINE__) - \
>> + PASTE(.Lscexitpv_rsb, __LINE__))), \
>> + X86_FEATURE_IBPB_EXIT_PV; \
>> +PASTE(.Lscexitpv_rsb, __LINE__): \
>> + ALTERNATIVE "", DO_OVERWRITE_RSB, X86_BUG_IBPB_NO_RET; \
>> +PASTE(.Lscexitpv_done, __LINE__): \
>> DO_SPEC_CTRL_COND_VERW
>
> What's wrong with the normal %= trick?
We're in a C macro here which is then used in assembly code. %= only
works in asm(), though. If we were in an assembler macro, I'd have
used \@. Yet wrapping the whole thing in an assembler macro would, for
my taste, hide too much information from the use sites (in particular
the X86_{FEATURE,BUG}_* which are imo relevant to be visible there).
> The use of __LINE__ makes this
> hard to subsequently livepatch, so I'd prefer to avoid it if possible.
Yes, I was certainly aware this would be a concern. I couldn't think of
a (forward looking) clean solution, though: Right now we have only one
use per source file (the native and compat PV entry.S), so we could use
a context-independent label name. But as you say above, for FRED we're
likely to get new entry points, and they're likely better placed in the
same files.
Jan
On 26/01/2023 8:02 am, Jan Beulich wrote:
> On 25.01.2023 22:10, Andrew Cooper wrote:
>> On 25/01/2023 3:25 pm, Jan Beulich wrote:
>>> In order to be able to defer the context switch IBPB to the last
>>> possible point, add logic to the exit-to-guest paths to issue the
>>> barrier there, including the "IBPB doesn't flush the RSB/RAS"
>>> workaround. Since alternatives, for now at least, can't nest, emit JMP
>>> to skip past both constructs where both are needed. This may be more
>>> efficient anyway, as the sequence of NOPs is pretty long.
>> It is very uarch specific as to when a jump is less overhead than a line
>> of nops.
>>
>> In all CPUs liable to be running Xen, even unconditional jumps take up
>> branch prediction resource, because all branch prediction is pre-decode
>> these days, so branch locations/types/destinations all need deriving
>> from %rip and "history" alone.
>>
>> So whether a branch or a line of nops is better is a tradeoff between
>> how much competition there is for branch prediction resource, and how
>> efficiently the CPU can brute-force its way through a long line of nops.
>>
>> But a very interesting datapoint. It turns out that AMD Zen4 CPUs
>> macrofuse adjacent nops, including longnops, because it reduces the
>> amount of execute/retire resources required. And a lot of
>> kernel/hypervisor fastpaths have a lot of nops these days.
>>
>>
>> For us, the "can't nest" is singularly more important than any worry
>> about uarch behaviour. We've frankly got much lower hanging fruit than
>> worring about one branch vs a few nops.
>>
>>> LFENCEs are omitted - for HVM a VM entry is immanent, which already
>>> elsewhere we deem sufficiently serializing an event. For 32-bit PV
>>> we're going through IRET, which ought to be good enough as well. While
>>> 64-bit PV may use SYSRET, there are several more conditional branches
>>> there which are all unprotected.
>> Privilege changes are serialsing-ish, and this behaviour has been
>> guaranteed moving forwards, although not documented coherently.
>>
>> CPL (well - privilege, which includes SMM, root/non-root, etc) is not
>> written speculatively. So any logic which needs to modify privilege has
>> to block until it is known to be an architectural execution path.
>>
>> This gets us "lfence-like" or "dispatch serialising" behaviour, which is
>> also the reason why INT3 is our go-to speculation halting instruction.
>> Microcode has to be entirely certain we are going to deliver an
>> interrupt/exception/etc before it can start reading the IDT/etc.
>>
>> Either way, we've been promised that all instructions like IRET,
>> SYS{CALL,RET,ENTER,EXIT}, VM{RUN,LAUNCH,RESUME} (and ERET{U,S} in the
>> future FRED world) do, and shall continue to not execute speculatively.
>>
>> Which in practice means we don't need to worry about Spectre-v1 attack
>> against codepaths which hit an exit-from-xen path, in terms of skipping
>> protections.
>>
>> We do need to be careful about memory accesses and potential double
>> dereferences, but all the data is on the top of the stack for XPTI
>> reasons. About the only concern is v->arch.msrs->* in the HVM path, and
>> we're fine with the current layout (AFAICT).
>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> I have to admit that I'm not really certain about the placement of the
>>> IBPB wrt the MSR_SPEC_CTRL writes. For now I've simply used "opposite of
>>> entry".
>> It really doesn't matter. They're independent operations that both need
>> doing, and are fully serialising so can't parallelise.
>>
>> But on this note, WRMSRNS and WRMSRLIST are on the horizon. The CPUs
>> which implement these instructions are the ones which also ought not to
>> need any adjustments in the exit paths. So I think it is specifically
>> not worth trying to make any effort to turn *these* WRMSR's into more
>> optimised forms.
>>
>> But WRMSRLIST was designed specifically for this kind of usecase
>> (actually, more for the main context switch path) where you can prepare
>> the list of MSRs in memory, including the ability to conditionally skip
>> certain entries by adjusting the index field.
>>
>>
>> It occurs to me, having written this out, is that what we actually want
>> to do is have slightly custom not-quite-alternative blocks. We have a
>> sequence of independent code blocks, and a small block at the end that
>> happens to contain an IRET.
>>
>> We could remove the nops at boot time if we treated it as one large
>> region, with the IRET at the end also able to have a variable position,
>> and assembles the "active" blocks tightly from the start. Complications
>> would include adjusting the IRET extable entry, but this isn't
>> insurmountable. Entrypoints are a bit more tricky but could be done by
>> packing from the back forward, and adjusting the entry position.
>>
>> Either way, something to ponder. (It's also possible that it doesn't
>> make a measurable difference until we get to FRED, at which point we
>> have a set of fresh entry-points to write anyway, and a slight glimmer
>> of hope of not needing to pollute them with speculation workarounds...)
>>
>>> Since we're going to run out of SCF_* bits soon and since the new flag
>>> is meaningful only in struct cpu_info's spec_ctrl_flags, we could choose
>>> to widen that field to 16 bits right away and then use bit 8 (or higher)
>>> for the purpose here.
>> I really don't think it matters. We've got plenty of room, and the
>> flexibility to shuffle, in both structures. It's absolutely not worth
>> trying to introduce asymmetries to save 1 bit.
> Thanks for all the comments up to here. Just to clarify - I've not spotted
> anything there that would result in me being expected to take any action.
I'm tempted to suggest dropping the sentence about "might be more
efficient". It's not necessary at all IMO, and it's probably not
correct if you were to compare an atom microserver vs a big Xeon.
For the lfences paragraph, I'd suggest rephrasing to something like this:
"As with all other conditional blocks on exit-to-guest paths, no
Spectre-v1 protections are necessary as execution will imminently be
hitting a serialising event."
The commentary on IRET/SYSRET and other jumps is adding confusion to
matter which is known to be safe.
>
>>> --- a/xen/arch/x86/include/asm/current.h
>>> +++ b/xen/arch/x86/include/asm/current.h
>>> @@ -55,9 +55,13 @@ struct cpu_info {
>>>
>>> /* See asm/spec_ctrl_asm.h for usage. */
>>> unsigned int shadow_spec_ctrl;
>>> + /*
>>> + * spec_ctrl_flags can be accessed as a 32-bit entity and hence needs
>>> + * placing suitably.
>> I'd suggest "is accessed as a 32-bit entity, and wants aligning suitably" ?
> I've tried to choose the wording carefully: The 32-bit access is in an
> alternative block, so doesn't always come into play. Hence the "may",
> not "is". Alignment alone also isn't sufficient here (and mis-aligning
> isn't merely a performance problem) - the following three bytes also
> need to be valid to access in the first place. Hence "needs" and
> "placing", not "wants" and "aligning".
"can" is actually a very ambiguous, and one valid interpretation of the
sentence is "spec_ctrl_fags is permitted to be accessed as a 32bit
quantity, but isn't actually accessed in that way".
In this case, we mean "it *is* accessed as a 32bit entity, in certain
cases".
So what about:
"spec_ctrl_flags is accessed as a 32-bit entity in some cases. Place it
so it can be used as if it were an int."
?
>
>> If I've followed the logic correctly. (I can't say I was specifically
>> aware that the bit test instructions didn't have byte forms, but I
>> suspect such instruction forms would be very very niche.)
> Yes, there not being byte forms of BT* is the sole reason here.
>
>>> --- a/xen/arch/x86/include/asm/spec_ctrl.h
>>> +++ b/xen/arch/x86/include/asm/spec_ctrl.h
>>> @@ -36,6 +36,8 @@
>>> #define SCF_verw (1 << 3)
>>> #define SCF_ist_ibpb (1 << 4)
>>> #define SCF_entry_ibpb (1 << 5)
>>> +#define SCF_exit_ibpb_bit 6
>>> +#define SCF_exit_ibpb (1 << SCF_exit_ibpb_bit)
>> One option to avoid the second define is to use ILOG2() with btrl.
> Specifically not. The assembler doesn't know the conditional operator,
> and the pre-processor won't collapse the expression resulting from
> expanding ilog2().
Oh that's dull.
I suspect we could construct equivalent logic with an .if/.else chain,
but I have no idea if the order of evaluation would be conducive to
doing so as part of evaluating an immediate operand. We would
specifically not want something that ended looking like `ilog2 const
"btrl $" ",%eax"`, even though I could see how to make that work.
It would be nice if we could make something suitable here, but if not we
can live with the second _bit constant.
>
>>> @@ -272,6 +293,14 @@
>>> #define SPEC_CTRL_EXIT_TO_PV \
>>> ALTERNATIVE "", \
>>> DO_SPEC_CTRL_EXIT_TO_GUEST, X86_FEATURE_SC_MSR_PV; \
>>> + ALTERNATIVE __stringify(jmp PASTE(.Lscexitpv_done, __LINE__)), \
>>> + __stringify(DO_SPEC_CTRL_EXIT_IBPB \
>>> + disp=(PASTE(.Lscexitpv_done, __LINE__) - \
>>> + PASTE(.Lscexitpv_rsb, __LINE__))), \
>>> + X86_FEATURE_IBPB_EXIT_PV; \
>>> +PASTE(.Lscexitpv_rsb, __LINE__): \
>>> + ALTERNATIVE "", DO_OVERWRITE_RSB, X86_BUG_IBPB_NO_RET; \
>>> +PASTE(.Lscexitpv_done, __LINE__): \
>>> DO_SPEC_CTRL_COND_VERW
>> What's wrong with the normal %= trick?
> We're in a C macro here which is then used in assembly code. %= only
> works in asm(), though. If we were in an assembler macro, I'd have
> used \@. Yet wrapping the whole thing in an assembler macro would, for
> my taste, hide too much information from the use sites (in particular
> the X86_{FEATURE,BUG}_* which are imo relevant to be visible there).
>
>> The use of __LINE__ makes this
>> hard to subsequently livepatch, so I'd prefer to avoid it if possible.
> Yes, I was certainly aware this would be a concern. I couldn't think of
> a (forward looking) clean solution, though: Right now we have only one
> use per source file (the native and compat PV entry.S), so we could use
> a context-independent label name. But as you say above, for FRED we're
> likely to get new entry points, and they're likely better placed in the
> same files.
I was going to suggest using __COUNTER__ but I've just realised why that
wont work.
But on further consideration, this might be ok for livepatching, so long
as __LINE__ is only ever used with a local label name. By the time it
has been compiled to a binary, the label name wont survive; only the
resulting displacement will.
I think we probably want to merge this with TEMP_NAME() (perhaps as
UNIQ_NAME(), as it will have to move elsewhere to become common with
this) to avoid proliferating our livepatching reasoning.
~Andrew
On 26.01.2023 21:27, Andrew Cooper wrote:
> On 26/01/2023 8:02 am, Jan Beulich wrote:
>> On 25.01.2023 22:10, Andrew Cooper wrote:
>>> On 25/01/2023 3:25 pm, Jan Beulich wrote:
>>>> In order to be able to defer the context switch IBPB to the last
>>>> possible point, add logic to the exit-to-guest paths to issue the
>>>> barrier there, including the "IBPB doesn't flush the RSB/RAS"
>>>> workaround. Since alternatives, for now at least, can't nest, emit JMP
>>>> to skip past both constructs where both are needed. This may be more
>>>> efficient anyway, as the sequence of NOPs is pretty long.
>>> It is very uarch specific as to when a jump is less overhead than a line
>>> of nops.
>>>
>>> In all CPUs liable to be running Xen, even unconditional jumps take up
>>> branch prediction resource, because all branch prediction is pre-decode
>>> these days, so branch locations/types/destinations all need deriving
>>> from %rip and "history" alone.
>>>
>>> So whether a branch or a line of nops is better is a tradeoff between
>>> how much competition there is for branch prediction resource, and how
>>> efficiently the CPU can brute-force its way through a long line of nops.
>>>
>>> But a very interesting datapoint. It turns out that AMD Zen4 CPUs
>>> macrofuse adjacent nops, including longnops, because it reduces the
>>> amount of execute/retire resources required. And a lot of
>>> kernel/hypervisor fastpaths have a lot of nops these days.
>>>
>>>
>>> For us, the "can't nest" is singularly more important than any worry
>>> about uarch behaviour. We've frankly got much lower hanging fruit than
>>> worring about one branch vs a few nops.
>>>
>>>> LFENCEs are omitted - for HVM a VM entry is immanent, which already
>>>> elsewhere we deem sufficiently serializing an event. For 32-bit PV
>>>> we're going through IRET, which ought to be good enough as well. While
>>>> 64-bit PV may use SYSRET, there are several more conditional branches
>>>> there which are all unprotected.
>>> Privilege changes are serialsing-ish, and this behaviour has been
>>> guaranteed moving forwards, although not documented coherently.
>>>
>>> CPL (well - privilege, which includes SMM, root/non-root, etc) is not
>>> written speculatively. So any logic which needs to modify privilege has
>>> to block until it is known to be an architectural execution path.
>>>
>>> This gets us "lfence-like" or "dispatch serialising" behaviour, which is
>>> also the reason why INT3 is our go-to speculation halting instruction.
>>> Microcode has to be entirely certain we are going to deliver an
>>> interrupt/exception/etc before it can start reading the IDT/etc.
>>>
>>> Either way, we've been promised that all instructions like IRET,
>>> SYS{CALL,RET,ENTER,EXIT}, VM{RUN,LAUNCH,RESUME} (and ERET{U,S} in the
>>> future FRED world) do, and shall continue to not execute speculatively.
>>>
>>> Which in practice means we don't need to worry about Spectre-v1 attack
>>> against codepaths which hit an exit-from-xen path, in terms of skipping
>>> protections.
>>>
>>> We do need to be careful about memory accesses and potential double
>>> dereferences, but all the data is on the top of the stack for XPTI
>>> reasons. About the only concern is v->arch.msrs->* in the HVM path, and
>>> we're fine with the current layout (AFAICT).
>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> I have to admit that I'm not really certain about the placement of the
>>>> IBPB wrt the MSR_SPEC_CTRL writes. For now I've simply used "opposite of
>>>> entry".
>>> It really doesn't matter. They're independent operations that both need
>>> doing, and are fully serialising so can't parallelise.
>>>
>>> But on this note, WRMSRNS and WRMSRLIST are on the horizon. The CPUs
>>> which implement these instructions are the ones which also ought not to
>>> need any adjustments in the exit paths. So I think it is specifically
>>> not worth trying to make any effort to turn *these* WRMSR's into more
>>> optimised forms.
>>>
>>> But WRMSRLIST was designed specifically for this kind of usecase
>>> (actually, more for the main context switch path) where you can prepare
>>> the list of MSRs in memory, including the ability to conditionally skip
>>> certain entries by adjusting the index field.
>>>
>>>
>>> It occurs to me, having written this out, is that what we actually want
>>> to do is have slightly custom not-quite-alternative blocks. We have a
>>> sequence of independent code blocks, and a small block at the end that
>>> happens to contain an IRET.
>>>
>>> We could remove the nops at boot time if we treated it as one large
>>> region, with the IRET at the end also able to have a variable position,
>>> and assembles the "active" blocks tightly from the start. Complications
>>> would include adjusting the IRET extable entry, but this isn't
>>> insurmountable. Entrypoints are a bit more tricky but could be done by
>>> packing from the back forward, and adjusting the entry position.
>>>
>>> Either way, something to ponder. (It's also possible that it doesn't
>>> make a measurable difference until we get to FRED, at which point we
>>> have a set of fresh entry-points to write anyway, and a slight glimmer
>>> of hope of not needing to pollute them with speculation workarounds...)
>>>
>>>> Since we're going to run out of SCF_* bits soon and since the new flag
>>>> is meaningful only in struct cpu_info's spec_ctrl_flags, we could choose
>>>> to widen that field to 16 bits right away and then use bit 8 (or higher)
>>>> for the purpose here.
>>> I really don't think it matters. We've got plenty of room, and the
>>> flexibility to shuffle, in both structures. It's absolutely not worth
>>> trying to introduce asymmetries to save 1 bit.
>> Thanks for all the comments up to here. Just to clarify - I've not spotted
>> anything there that would result in me being expected to take any action.
>
> I'm tempted to suggest dropping the sentence about "might be more
> efficient". It's not necessary at all IMO, and it's probably not
> correct if you were to compare an atom microserver vs a big Xeon.
Hmm - "might" still isn't "will". ISTR us actually discussing to limit
how long a sequence of NOPs we'd tolerate before switching to JMP.
>>>> --- a/xen/arch/x86/include/asm/spec_ctrl.h
>>>> +++ b/xen/arch/x86/include/asm/spec_ctrl.h
>>>> @@ -36,6 +36,8 @@
>>>> #define SCF_verw (1 << 3)
>>>> #define SCF_ist_ibpb (1 << 4)
>>>> #define SCF_entry_ibpb (1 << 5)
>>>> +#define SCF_exit_ibpb_bit 6
>>>> +#define SCF_exit_ibpb (1 << SCF_exit_ibpb_bit)
>>> One option to avoid the second define is to use ILOG2() with btrl.
>> Specifically not. The assembler doesn't know the conditional operator,
>> and the pre-processor won't collapse the expression resulting from
>> expanding ilog2().
>
> Oh that's dull.
>
> I suspect we could construct equivalent logic with an .if/.else chain,
> but I have no idea if the order of evaluation would be conducive to
> doing so as part of evaluating an immediate operand. We would
> specifically not want something that ended looking like `ilog2 const
> "btrl $" ",%eax"`, even though I could see how to make that work.
>
> It would be nice if we could make something suitable here, but if not we
> can live with the second _bit constant.
How would .if/.else be able to go inside an expression? You can't even
put this in a .macro, as that can't be invoked as part of an expression
either.
>>>> @@ -272,6 +293,14 @@
>>>> #define SPEC_CTRL_EXIT_TO_PV \
>>>> ALTERNATIVE "", \
>>>> DO_SPEC_CTRL_EXIT_TO_GUEST, X86_FEATURE_SC_MSR_PV; \
>>>> + ALTERNATIVE __stringify(jmp PASTE(.Lscexitpv_done, __LINE__)), \
>>>> + __stringify(DO_SPEC_CTRL_EXIT_IBPB \
>>>> + disp=(PASTE(.Lscexitpv_done, __LINE__) - \
>>>> + PASTE(.Lscexitpv_rsb, __LINE__))), \
>>>> + X86_FEATURE_IBPB_EXIT_PV; \
>>>> +PASTE(.Lscexitpv_rsb, __LINE__): \
>>>> + ALTERNATIVE "", DO_OVERWRITE_RSB, X86_BUG_IBPB_NO_RET; \
>>>> +PASTE(.Lscexitpv_done, __LINE__): \
>>>> DO_SPEC_CTRL_COND_VERW
>>> What's wrong with the normal %= trick?
>> We're in a C macro here which is then used in assembly code. %= only
>> works in asm(), though. If we were in an assembler macro, I'd have
>> used \@. Yet wrapping the whole thing in an assembler macro would, for
>> my taste, hide too much information from the use sites (in particular
>> the X86_{FEATURE,BUG}_* which are imo relevant to be visible there).
>>
>>> The use of __LINE__ makes this
>>> hard to subsequently livepatch, so I'd prefer to avoid it if possible.
>> Yes, I was certainly aware this would be a concern. I couldn't think of
>> a (forward looking) clean solution, though: Right now we have only one
>> use per source file (the native and compat PV entry.S), so we could use
>> a context-independent label name. But as you say above, for FRED we're
>> likely to get new entry points, and they're likely better placed in the
>> same files.
>
> I was going to suggest using __COUNTER__ but I've just realised why that
> wont work.
>
> But on further consideration, this might be ok for livepatching, so long
> as __LINE__ is only ever used with a local label name. By the time it
> has been compiled to a binary, the label name wont survive; only the
> resulting displacement will.
>
> I think we probably want to merge this with TEMP_NAME() (perhaps as
> UNIQ_NAME(), as it will have to move elsewhere to become common with
> this) to avoid proliferating our livepatching reasoning.
Even if I had recalled that we have TEMP_NAME() somewhere, I'd be very
hesitant to make anything like that into more generally accessible
infrastructure. I consider TEMP_NAME() itself already a too widely
exposed. The problem with it is that you can easily end up with two uses
as the result of expanding something that's all contained on a single
source line. Hence I very specifically want to have uses of __LINE__
only in places where either it is the top level source line, or where
- as is the case here - it is clear that no two instance of the same or
a similar macro will ever sensibly be put on one line. (Even then there's
still the risk of using the C macro inside an assembler macro, where two
instances would cause problems. But if such appeared, making the
assembler macro suitably use \@ instead shouldn't be overly difficult.)
Jan
© 2016 - 2026 Red Hat, Inc.