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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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@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)?
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