Re: [PATCH v2 08/14] x86/cpu: Adjust reset_stack_and_jump() to be shadow stack compatible

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.05.2020 21:18, Andrew Cooper wrote:
> We need to unwind up to the supervisor token.  See the comment for details.
> 
> The use of UNLIKELY_END_SECTION in this case highlights that it isn't safe
> when it isn't the final statement of an asm().  Adjust all declarations with a
> newline.

That's only one perspective to take. I'd appreciate if you undid this.
The intention has always been ...

> --- a/xen/include/asm-x86/current.h
> +++ b/xen/include/asm-x86/current.h
> @@ -124,13 +124,55 @@ unsigned long get_stack_dump_bottom (unsigned long sp);
>  # define CHECK_FOR_LIVEPATCH_WORK ""
>  #endif
>  
> +#ifdef CONFIG_XEN_SHSTK
> +/*
> + * We need to unwind the primary shadow stack to its supervisor token, 
> located
> + * at 0x5ff8 from the base of the stack blocks.
> + *
> + * Read the shadow stack pointer, subtract it from 0x5ff8, divide by 8 to get
> + * the number of slots needing popping.
> + *
> + * INCSSPQ can't pop more than 255 entries.  We shouldn't ever need to pop
> + * that many entries, and getting this wrong will cause us to #DF later.  
> Turn
> + * it into a BUG() now for fractionally easier debugging.
> + */
> +# define SHADOW_STACK_WORK                                      \
> +    "mov $1, %[ssp];"                                           \
> +    "rdsspd %[ssp];"                                            \
> +    "cmp $1, %[ssp];"                                           \
> +    "je .L_shstk_done.%=;" /* CET not active?  Skip. */         \
> +    "mov $%c[skstk_base], %[val];"                              \
> +    "and $%c[stack_mask], %[ssp];"                              \
> +    "sub %[ssp], %[val];"                                       \
> +    "shr $3, %[val];"                                           \
> +    "cmp $255, %[val];" /* More than 255 entries?  Crash. */    \
> +    UNLIKELY_START(a, shstk_adjust)                             \

... to put suitable separators at the use sites (which, seeing all
the other semicolons here, would be a semicolon then, not a
newline/tab combination. If a tab was to be put anywhere, then like
this:

#define UNLIKELY_START_SECTION "\t.pushsection .text.unlikely,\"ax\""
#define UNLIKELY_END_SECTION   "\t.popsection"

as directives (from a cross arch perspective) are supposed to be
indented; it's just that on most arch-es it doesn't matter (and
hence is irrelevant here).

Preferably with this aspect undone
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

As to the comment, could I talk you into replacing the two 0x5ff8
instances by something like "last word of the shadow stack"?

Jan