[PATCH 4/9] x86/spec-ctrl: Improve all SPEC_CTRL_{ENTER,EXIT}_* comments

Andrew Cooper posted 9 patches 11 months, 4 weeks ago
[PATCH 4/9] x86/spec-ctrl: Improve all SPEC_CTRL_{ENTER,EXIT}_* comments
Posted by Andrew Cooper 11 months, 4 weeks ago
... to better explain how they're used.

Doing so highlights that SPEC_CTRL_EXIT_TO_XEN is missing a VERW flush for the
corner case when e.g. an NMI hits late in an exit-to-guest path.

Leave a TODO, which will be addressed in subsequent patches which arrange for
DO_COND_VERW to be safe within SPEC_CTRL_EXIT_TO_XEN.

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>

This was decided not to be XSA-worthy, as guests can't usefully control when
IST events occur.

v2:
 * Rewrite.
---
 xen/arch/x86/include/asm/spec_ctrl_asm.h | 36 ++++++++++++++++++++----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/include/asm/spec_ctrl_asm.h b/xen/arch/x86/include/asm/spec_ctrl_asm.h
index f768b0f48a0b..8996fe3fc0ef 100644
--- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
+++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
@@ -218,7 +218,10 @@
     wrmsr
 .endm
 
-/* Use after an entry from PV context (syscall/sysenter/int80/int82/etc). */
+/*
+ * Used after an entry from PV context: SYSCALL, SYSENTER, INT,
+ * etc.  There is always a guest speculation state in context.
+ */
 .macro SPEC_CTRL_ENTRY_FROM_PV
 /*
  * Requires %rsp=regs/cpuinfo, %rdx=0
@@ -233,7 +236,11 @@
         X86_FEATURE_SC_MSR_PV
 .endm
 
-/* Use in interrupt/exception context.  May interrupt Xen or PV context. */
+/*
+ * Used after an exception or maskable interrupt, hitting Xen or PV context.
+ * There will either be a guest speculation context, or (baring fatal
+ * exceptions) a well-formed Xen speculation context.
+ */
 .macro SPEC_CTRL_ENTRY_FROM_INTR
 /*
  * Requires %rsp=regs, %r14=stack_end, %rdx=0
@@ -248,7 +255,10 @@
         X86_FEATURE_SC_MSR_PV
 .endm
 
-/* Use when exiting to PV guest context. */
+/*
+ * Used when exiting from any entry context, back to PV context.  This
+ * includes from an IST entry which moved onto the primary stack.
+ */
 .macro SPEC_CTRL_EXIT_TO_PV
 /*
  * Requires %rax=spec_ctrl, %rsp=regs/info
@@ -260,7 +270,13 @@
 .endm
 
 /*
- * Use in IST interrupt/exception context.  May interrupt Xen or PV context.
+ * Used after an IST entry hitting Xen or PV context.  Special care is needed,
+ * because when hitting Xen context, there may not a well-formed speculation
+ * context.  (i.e. it can hit in the middle of SPEC_CTRL_{ENTRY,EXIT}_*
+ * regions.)
+ *
+ * An IST entry which hits PV context moves onto the primary stack and leaves
+ * via SPEC_CTRL_EXIT_TO_PV, *not* SPEC_CTRL_EXIT_TO_XEN.
  */
 .macro SPEC_CTRL_ENTRY_FROM_INTR_IST
 /*
@@ -319,7 +335,14 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
     UNLIKELY_END(\@_serialise)
 .endm
 
-/* Use when exiting to Xen context. */
+/*
+ * Use when exiting from any entry context, back to Xen context.  This
+ * includes returning to other SPEC_CTRL_{ENTRY,EXIT}_* regions with an
+ * incomplete speculation context.
+ *
+ * Because we might have interrupted Xen beyond SPEC_CTRL_EXIT_TO_$GUEST, we
+ * need to treat this as if it were an EXIT_TO_$GUEST case too.
+ */
 .macro SPEC_CTRL_EXIT_TO_XEN
 /*
  * Requires %rbx=stack_end
@@ -344,6 +367,9 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
     wrmsr
 
 .L\@_skip_sc_msr:
+
+    /* TODO VERW */
+
 .endm
 
 #endif /* __ASSEMBLY__ */
-- 
2.30.2


Re: [PATCH 4/9] x86/spec-ctrl: Improve all SPEC_CTRL_{ENTER,EXIT}_* comments
Posted by Jan Beulich 11 months, 3 weeks ago
On 15.09.2023 17:00, Andrew Cooper wrote:
> ... to better explain how they're used.
> 
> Doing so highlights that SPEC_CTRL_EXIT_TO_XEN is missing a VERW flush for the
> corner case when e.g. an NMI hits late in an exit-to-guest path.
> 
> Leave a TODO, which will be addressed in subsequent patches which arrange for
> DO_COND_VERW to be safe within SPEC_CTRL_EXIT_TO_XEN.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Two nits though:

> @@ -233,7 +236,11 @@
>          X86_FEATURE_SC_MSR_PV
>  .endm
>  
> -/* Use in interrupt/exception context.  May interrupt Xen or PV context. */
> +/*
> + * Used after an exception or maskable interrupt, hitting Xen or PV context.
> + * There will either be a guest speculation context, or (baring fatal

Isn't this "barring"?

> @@ -260,7 +270,13 @@
>  .endm
>  
>  /*
> - * Use in IST interrupt/exception context.  May interrupt Xen or PV context.
> + * Used after an IST entry hitting Xen or PV context.  Special care is needed,
> + * because when hitting Xen context, there may not a well-formed speculation

Missing "be"?

Jan
Re: [PATCH 4/9] x86/spec-ctrl: Improve all SPEC_CTRL_{ENTER,EXIT}_* comments
Posted by Andrew Cooper 11 months, 3 weeks ago
On 18/09/2023 11:59 am, Jan Beulich wrote:
> On 15.09.2023 17:00, Andrew Cooper wrote:
>> ... to better explain how they're used.
>>
>> Doing so highlights that SPEC_CTRL_EXIT_TO_XEN is missing a VERW flush for the
>> corner case when e.g. an NMI hits late in an exit-to-guest path.
>>
>> Leave a TODO, which will be addressed in subsequent patches which arrange for
>> DO_COND_VERW to be safe within SPEC_CTRL_EXIT_TO_XEN.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Two nits though:
>
>> @@ -233,7 +236,11 @@
>>          X86_FEATURE_SC_MSR_PV
>>  .endm
>>  
>> -/* Use in interrupt/exception context.  May interrupt Xen or PV context. */
>> +/*
>> + * Used after an exception or maskable interrupt, hitting Xen or PV context.
>> + * There will either be a guest speculation context, or (baring fatal
> Isn't this "barring"?
>
>> @@ -260,7 +270,13 @@
>>  .endm
>>  
>>  /*
>> - * Use in IST interrupt/exception context.  May interrupt Xen or PV context.
>> + * Used after an IST entry hitting Xen or PV context.  Special care is needed,
>> + * because when hitting Xen context, there may not a well-formed speculation
> Missing "be"?

Both fixed, thanks.

~Andrew