Re: [PATCH 2/4] ARM: Fix register constraints in run_in_exception_handler()

[Julien Grall <julien@xxxxxxx>]

Hi,

On 08/02/2025 00:02, Andrew Cooper wrote:
> Right now, run_in_exception_handler() takes an input in an arbitrary register,
> and clobbers BUG_FN_REG.  This causes the compiler to calculate fn in the
> wrong regsiter.
Just to confirm, you mean, the compiler is not clever enough to notice 
that the value should be in the register BUG_FN_REG and therefore, two 
registers will be clobbered. Is that correct?
> > Instead, use `register asm()` which is the normal way of tying register
> constraints to exact registers.
>
> Bloat-o-meter reports:
>
>    ARM64:
>      Function                                     old     new   delta
>      dump_registers                               356     348      -8
>
>    ARM32:
>      ns16550_poll                                  52      48      -4
>      dump_registers                               432     428      -4
>
> The other instruction dropped in ARM64's dump_registers() is an alignment nop.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ----> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> CC: Michal Orzel <michal.orzel@xxxxxxx>
> CC: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
>   xen/arch/arm/include/asm/bug.h | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h
> index cacaf014ab09..8bf71587bea1 100644
> --- a/xen/arch/arm/include/asm/bug.h
> +++ b/xen/arch/arm/include/asm/bug.h
> @@ -59,15 +59,15 @@ struct bug_frame {
>    * be called function in a fixed register.
>    */
>   #define  run_in_exception_handler(fn) do {                                  \
> -    asm ("mov " __stringify(BUG_FN_REG) ", %0\n"                            \
> -         "1:"BUG_INSTR"\n"                                                  \
> +    register unsigned long _fn asm (STR(BUG_FN_REG)) = (unsigned long)(fn); \
> +    asm ("1:"BUG_INSTR"\n"                                                  \
>            ".pushsection .bug_frames." __stringify(BUGFRAME_run_fn) ","       \
>            "             \"a\", %%progbits\n"                                 \
>            "2:\n"                                                             \
>            ".p2align 2\n"                                                     \
>            ".long (1b - 2b)\n"                                                \
>            ".long 0, 0, 0\n"                                                  \
> -         ".popsection" :: "r" (fn) : __stringify(BUG_FN_REG) );             \
> +         ".popsection" :: "r" (_fn) );                                      \
>   } while (0)
>   
>   #define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, "")
--
Julien Grall