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

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

On 2025-07-21 08:41, Jan Beulich wrote:
> 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@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> 
>> 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)?
> 

Only for using the __label__ token in 
automation/eclair_analysis/ECLAIR/toolchain.ecl. The extension itself is 
already documented in 5590c7e6590d ("eclair: allow and document use of 
GCC extension for label addresses")

>> 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

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253