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

[Jan Beulich <jbeulich@xxxxxxxx>]

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