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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 4/8] x86/spec-ctrl: Extend all SPEC_CTRL_{ENTER,EXIT}_* comments
Date: Thu, 14 Sep 2023 20:23:10 +0100	[thread overview]
Message-ID: <35177e10-3306-69fc-4ece-bba453cbdb0c@citrix.com> (raw)
In-Reply-To: <e12f46d9-25eb-d564-4cb7-0e476e741725@suse.com>

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