Re: [PATCH] x86/pv: Rework TRY_LOAD_SEG() to use asm goto()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.07.2025 22:25, Andrew Cooper wrote:
> This moves the exception path to being out-of-line within the function, rather
> than in the .fixup section, which improves backtraces.
> 
> Because the macro is used multiple times, the fault label needs declaring as
> local.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Slightly RFC.  I haven't checked if Eclair will be happy with __label__ yet.

Even if it is, I guess you'd need to update the list of extensions we
use (docs/misra/C-language-toolchain.rst)?

> It is disappointing that, unless we retain the xor/mov for the exception path,
> GCC decides to emit worse code, notably duplicating the mov %ds success path
> in mov %es's error path.

Is it the pair of XOR/MOV, or merely the MOV (in which case it might be
nice to try omitting at least the XOR)? Yet then the dual purpose of the
zero is likely getting in the way anyway.

> The "+r" constraint was actually wrong before; the asm only produces
> all_segs_okay and does not consume it.

Yet it only conditionally set it in the old construct. That still needs
expressing with "+r", or else the variable's earlier setting could all
be eliminated. In the new construct using "=r" is okay.

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1738,17 +1738,22 @@ static void load_segments(struct vcpu *n)
>       * @all_segs_okay in function scope, and load NUL into @sel.
>       */
>  #define TRY_LOAD_SEG(seg, val)                          \
> -    asm_inline volatile (                               \
> -        "1: mov %k[_val], %%" #seg "\n\t"               \
> -        "2:\n\t"                                        \
> -        ".section .fixup, \"ax\"\n\t"                   \
> -        "3: xor %k[ok], %k[ok]\n\t"                     \
> -        "   mov %k[ok], %%" #seg "\n\t"                 \
> -        "   jmp 2b\n\t"                                 \
> -        ".previous\n\t"                                 \
> -        _ASM_EXTABLE(1b, 3b)                            \
> -        : [ok] "+r" (all_segs_okay)                     \
> -        : [_val] "rm" (val) )
> +    ({                                                  \
> +        __label__ fault;                                \
> +        asm_inline volatile goto (                      \
> +            "1: mov %k[_val], %%" #seg "\n\t"           \
> +            _ASM_EXTABLE(1b, %l[fault])                 \
> +            :: [_val] "rm" (val)                        \

Thoughts on replacing "_val" by "sel" on this occasion?

> +            :: fault );                                 \
> +        if ( 0 )                                        \
> +        {                                               \
> +        fault: __attribute__((cold));                   \
> +            asm_inline volatile (                       \
> +                "xor %k[ok], %k[ok]\n\t"                \
> +                "mov %k[ok], %%" #seg                   \
> +                : [ok] "=r" (all_segs_okay) );          \

Purely formally I think you need "=&r" here now.

Jan