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

Andrew Cooper posted 8 patches 7 months, 1 week ago
There is a newer version of this series
[PATCH 4/8] x86/spec-ctrl: Extend all SPEC_CTRL_{ENTER,EXIT}_* comments
Posted by Andrew Cooper 7 months, 1 week 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.
---
 xen/arch/x86/include/asm/spec_ctrl_asm.h | 35 ++++++++++++++++++++----
 1 file changed, 30 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 ee75f2bced42..77f6c35bb9c5 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 a synchronous entry from PV context.  SYSCALL, SYSENTER, INT,
+ * etc.  Will always interrupt a guest speculation 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 a synchronous interrupt or exception.  May interrupt Xen or PV
+ * context, but will not interrupt Xen with a guest speculation context,
+ * outside of fatal error cases.
+ */
 .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,12 @@
 .endm
 
 /*
- * Use in IST interrupt/exception context.  May interrupt Xen or PV context.
+ * Used after an IST entry (i.e. needs special care, consider to be fully
+ * asynchronous with finding sane state).  May interrupt PV or Xen context,
+ * including other SPEC_CTRL_{ENTRY,EXIT}_* regions with unsanitised state.
+ *
+ * An IST entry which interrupts 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 +334,14 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
     UNLIKELY_END(\@_serialise)
 .endm
 
-/* Use when exiting to Xen in IST context. */
+/*
+ * Use when exiting from any entry context, back to Xen context.  This
+ * includes returning to other SPEC_CTRL_{ENTRY,EXIT}_* regions with
+ * unsanitised state.
+ *
+ * Because we might have interrupted Xen beyond SPEC_CTRL_EXIT_TO_$GUEST, we
+ * must treat this as if it were an EXIT_TO_$GUEST case too.
+ */
 .macro SPEC_CTRL_EXIT_TO_XEN
 /*
  * Requires %rbx=stack_end
@@ -344,6 +366,9 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
     wrmsr
 
 .L\@_skip_sc_msr:
+
+    /* TODO VERW */
+
 .endm
 
 #endif /* __ASSEMBLY__ */
-- 
2.30.2


Re: [PATCH 4/8] x86/spec-ctrl: Extend all SPEC_CTRL_{ENTER,EXIT}_* comments
Posted by Jan Beulich 7 months, 1 week ago
On 13.09.2023 22:27, Andrew Cooper wrote:
> --- 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 a synchronous entry from PV context.  SYSCALL, SYSENTER, INT,
> + * etc.  Will always interrupt a guest speculation context.
> + */
>  .macro SPEC_CTRL_ENTRY_FROM_PV
>  /*
>   * Requires %rsp=regs/cpuinfo, %rdx=0

For the entry point comments - not being a native speaker -, the use of
"{will,may} interrupt" reads odd. You're describing the macros here,
not the the events that led to their invocation. Therefore it would seem
to me that "{will,may} have interrupted" would be more appropriate.

> @@ -233,7 +236,11 @@
>          X86_FEATURE_SC_MSR_PV
>  .endm
>  
> -/* Use in interrupt/exception context.  May interrupt Xen or PV context. */
> +/*
> + * Used after a synchronous interrupt or exception.  May interrupt Xen or PV
> + * context, but will not interrupt Xen with a guest speculation context,
> + * outside of fatal error cases.
> + */
>  .macro SPEC_CTRL_ENTRY_FROM_INTR
>  /*
>   * Requires %rsp=regs, %r14=stack_end, %rdx=0

I find "synchronous" here odd, when this includes interrupts. Below you
at least qualify things further, by saying "fully asynchronous with
finding sane state"; I think further qualification is warranted here as
well, to avoid any ambiguity.

> @@ -319,7 +334,14 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
>      UNLIKELY_END(\@_serialise)
>  .endm
>  
> -/* Use when exiting to Xen in IST context. */
> +/*
> + * Use when exiting from any entry context, back to Xen context.  This
> + * includes returning to other SPEC_CTRL_{ENTRY,EXIT}_* regions with
> + * unsanitised state.
> + *
> + * Because we might have interrupted Xen beyond SPEC_CTRL_EXIT_TO_$GUEST, we
> + * must treat this as if it were an EXIT_TO_$GUEST case too.
> + */
>  .macro SPEC_CTRL_EXIT_TO_XEN
>  /*
>   * Requires %rbx=stack_end

Is it really "must"? At least in theory there are ways to recognize that
exit is back to Xen context outside of interrupted entry/exit regions
(simply by evaluating how far below stack top %rsp is).

> @@ -344,6 +366,9 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
>      wrmsr
>  
>  .L\@_skip_sc_msr:
> +
> +    /* TODO VERW */
> +
>  .endm

I don't think this comment is strictly necessary to add here, when the
omission is addressed in a later patch. But I also don't mind its
addition.

Jan
Re: [PATCH 4/8] x86/spec-ctrl: Extend all SPEC_CTRL_{ENTER,EXIT}_* comments
Posted by Andrew Cooper 7 months, 1 week ago
On 14/09/2023 8:58 am, Jan Beulich wrote:
> On 13.09.2023 22:27, Andrew Cooper wrote:
>> --- 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 a synchronous entry from PV context.  SYSCALL, SYSENTER, INT,
>> + * etc.  Will always interrupt a guest speculation context.
>> + */
>>  .macro SPEC_CTRL_ENTRY_FROM_PV
>>  /*
>>   * Requires %rsp=regs/cpuinfo, %rdx=0
> For the entry point comments - not being a native speaker -, the use of
> "{will,may} interrupt" reads odd. You're describing the macros here,
> not the the events that led to their invocation. Therefore it would seem
> to me that "{will,may} have interrupted" would be more appropriate.

The salient information is what the speculation state looks like when
we're running the asm in these macros.

Sync and Async perhaps aren't the best terms.  For PV context at least,
it boils down to:

1) CPL>0 => you were in fully-good guest speculation context
2) CPL=0 => you were in fully-good Xen speculation context
3) IST && CPL=0 => Here be dragons.

HVM is more of a challenge.  VT-x behaves like an IST path, while SVM
does allow us to atomically switch to good Xen state.

Really, this needs to be a separate doc, with diagrams...

>> @@ -319,7 +334,14 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
>>      UNLIKELY_END(\@_serialise)
>>  .endm
>>  
>> -/* Use when exiting to Xen in IST context. */
>> +/*
>> + * Use when exiting from any entry context, back to Xen context.  This
>> + * includes returning to other SPEC_CTRL_{ENTRY,EXIT}_* regions with
>> + * unsanitised state.
>> + *
>> + * Because we might have interrupted Xen beyond SPEC_CTRL_EXIT_TO_$GUEST, we
>> + * must treat this as if it were an EXIT_TO_$GUEST case too.
>> + */
>>  .macro SPEC_CTRL_EXIT_TO_XEN
>>  /*
>>   * Requires %rbx=stack_end
> Is it really "must"? At least in theory there are ways to recognize that
> exit is back to Xen context outside of interrupted entry/exit regions
> (simply by evaluating how far below stack top %rsp is).

Yes, it is must - it's about how Xen behaves right now, not about some
theoretical future with different tracking mechanism.

Checking the stack is very fragile and we've had bugs doing this in the
past.  It would break completely if we were to take things such as the
recursive-NMI fix (not that we're liable to at this point with FRED on
the horizon.)

A perhaps less fragile option would be to have .text.entry.spec_suspect
section and check %rip being in that.

But neither of these are good options.  It's adding complexity (latency)
to a fastpath to avoid a small hit in a rare case, so is a concrete
anti-optimisation.

>> @@ -344,6 +366,9 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
>>      wrmsr
>>  
>>  .L\@_skip_sc_msr:
>> +
>> +    /* TODO VERW */
>> +
>>  .endm
> I don't think this comment is strictly necessary to add here, when the
> omission is addressed in a later patch. But I also don't mind its
> addition.

It doesn't especially matter if the series gets committed in one go, but
it does matter if it ends up being split.  This is the patch which
observes that VERW is missing.

~Andrew

Re: [PATCH 4/8] x86/spec-ctrl: Extend all SPEC_CTRL_{ENTER,EXIT}_* comments
Posted by Jan Beulich 7 months ago
On 14.09.2023 21:23, Andrew Cooper wrote:
> On 14/09/2023 8:58 am, Jan Beulich wrote:
>> On 13.09.2023 22:27, Andrew Cooper wrote:
>>> --- 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 a synchronous entry from PV context.  SYSCALL, SYSENTER, INT,
>>> + * etc.  Will always interrupt a guest speculation context.
>>> + */
>>>  .macro SPEC_CTRL_ENTRY_FROM_PV
>>>  /*
>>>   * Requires %rsp=regs/cpuinfo, %rdx=0
>> For the entry point comments - not being a native speaker -, the use of
>> "{will,may} interrupt" reads odd. You're describing the macros here,
>> not the the events that led to their invocation. Therefore it would seem
>> to me that "{will,may} have interrupted" would be more appropriate.
> 
> The salient information is what the speculation state looks like when
> we're running the asm in these macros.
> 
> Sync and Async perhaps aren't the best terms.  For PV context at least,
> it boils down to:
> 
> 1) CPL>0 => you were in fully-good guest speculation context
> 2) CPL=0 => you were in fully-good Xen speculation context
> 3) IST && CPL=0 => Here be dragons.
> 
> HVM is more of a challenge.  VT-x behaves like an IST path, while SVM
> does allow us to atomically switch to good Xen state.
> 
> Really, this needs to be a separate doc, with diagrams...
> 
>>> @@ -319,7 +334,14 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
>>>      UNLIKELY_END(\@_serialise)
>>>  .endm
>>>  
>>> -/* Use when exiting to Xen in IST context. */
>>> +/*
>>> + * Use when exiting from any entry context, back to Xen context.  This
>>> + * includes returning to other SPEC_CTRL_{ENTRY,EXIT}_* regions with
>>> + * unsanitised state.
>>> + *
>>> + * Because we might have interrupted Xen beyond SPEC_CTRL_EXIT_TO_$GUEST, we
>>> + * must treat this as if it were an EXIT_TO_$GUEST case too.
>>> + */
>>>  .macro SPEC_CTRL_EXIT_TO_XEN
>>>  /*
>>>   * Requires %rbx=stack_end
>> Is it really "must"? At least in theory there are ways to recognize that
>> exit is back to Xen context outside of interrupted entry/exit regions
>> (simply by evaluating how far below stack top %rsp is).
> 
> Yes, it is must - it's about how Xen behaves right now, not about some
> theoretical future with different tracking mechanism.

Well, deleting "must" does exactly that: Describe what we currently do,
without imposing that it necessarily has to be that way. That's at least
to me, as an - as said - non-native speaker.

> Checking the stack is very fragile and we've had bugs doing this in the
> past.  It would break completely if we were to take things such as the
> recursive-NMI fix (not that we're liable to at this point with FRED on
> the horizon.)
> 
> A perhaps less fragile option would be to have .text.entry.spec_suspect
> section and check %rip being in that.
> 
> But neither of these are good options.  It's adding complexity (latency)
> to a fastpath to avoid a small hit in a rare case, so is a concrete
> anti-optimisation.

I fully accept all of this, and I wasn't meaning to suggest we do what
might be possible. I merely dislike stronger than necessary statements,
as such are liable to cause confusion down the road.

That said, I certainly won't refuse to eventually ack this patch just
because of this one word.

Jan

Re: [PATCH 4/8] x86/spec-ctrl: Extend all SPEC_CTRL_{ENTER,EXIT}_* comments
Posted by Andrew Cooper 7 months ago
On 15/09/2023 8:07 am, Jan Beulich wrote:
> On 14.09.2023 21:23, Andrew Cooper wrote:
>> On 14/09/2023 8:58 am, Jan Beulich wrote:
>>> On 13.09.2023 22:27, Andrew Cooper wrote:
>>>> @@ -319,7 +334,14 @@ UNLIKELY_DISPATCH_LABEL(\@_serialise):
>>>>      UNLIKELY_END(\@_serialise)
>>>>  .endm
>>>>  
>>>> -/* Use when exiting to Xen in IST context. */
>>>> +/*
>>>> + * Use when exiting from any entry context, back to Xen context.  This
>>>> + * includes returning to other SPEC_CTRL_{ENTRY,EXIT}_* regions with
>>>> + * unsanitised state.
>>>> + *
>>>> + * Because we might have interrupted Xen beyond SPEC_CTRL_EXIT_TO_$GUEST, we
>>>> + * must treat this as if it were an EXIT_TO_$GUEST case too.
>>>> + */
>>>>  .macro SPEC_CTRL_EXIT_TO_XEN
>>>>  /*
>>>>   * Requires %rbx=stack_end
>>> Is it really "must"? At least in theory there are ways to recognize that
>>> exit is back to Xen context outside of interrupted entry/exit regions
>>> (simply by evaluating how far below stack top %rsp is).
>> Yes, it is must - it's about how Xen behaves right now, not about some
>> theoretical future with different tracking mechanism.
> Well, deleting "must" does exactly that

Nonsense.

*When* someone changes the logic such that there's an alternative route,
the comment necessarily needs updating.  And until that point, the logic
*must* behave in this way to be correct.

~Andrew