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
