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

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.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 09:58:25 +0200	[thread overview]
Message-ID: <e12f46d9-25eb-d564-4cb7-0e476e741725@suse.com> (raw)
In-Reply-To: <20230913202758.508225-5-andrew.cooper3@citrix.com>

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