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.
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@citrix.com>
----
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Michal Orzel <michal.orzel@amd.com>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
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, "")
--
2.39.5Hi,
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@citrix.com>
> ----> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Michal Orzel <michal.orzel@amd.com>
> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> 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
On 10/02/2025 9:29 pm, Julien Grall wrote: > 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? Not quite. The clobbered register set is always disjoint from inputs and outputs, so the combination of one clobbered + one input always means two different registers. For "here's an input but it gets modified", you need to express that as an output into a variable which isn't subsequently used. For ARM, that is best spelt "+r" (foo) so it can also be used with register asm() to tie to a single register. On x86, you can use "=a" (tmp) : "a" (input). In principle you can do it with named parameters, so [fn] "=r" (tmp) : "[fn]" (input) I believe works too. Here is a contrived example https://godbolt.org/z/WjqTKjWWb showing how the output (discard only) is forced into r0, causing the compiler to copy a into r3 around the asm block. Notice that GCC and Clang pick the input operand differently, as both r0 and r3 are valid candidates in this case. However, for run_in_exception_handler(), "fn" isn't even modified (AFAICT), so it's correct to describe it as an input only. ~Andrew
On 08/02/2025 01: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. > > 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@citrix.com> Reviewed-by: Michal Orzel <michal.orzel@amd.com> ~Michal
On 2/8/25 1:02 AM, 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.
Probably, we should give a chance for the patch which suggests to use GENERIC_BUG_FRAME:
https://lore.kernel.org/xen-devel/8fdb98350ae4fc6029738d0aabe13a57e1945a50.1680086655.git.oleksii.kurochko@gmail.com/
~ Oleksii
> 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@citrix.com>
> ----
> CC: Stefano Stabellini<sstabellini@kernel.org>
> CC: Julien Grall<julien@xen.org>
> CC: Volodymyr Babchuk<Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis<bertrand.marquis@arm.com>
> CC: Michal Orzel<michal.orzel@amd.com>
> CC: Oleksii Kurochko<oleksii.kurochko@gmail.com>
> ---
> 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, "")
On 10/02/2025 09:21, Oleksii Kurochko wrote: > > On 2/8/25 1:02 AM, 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. > > Probably, we should give a chance for the patch which suggests to use GENERIC_BUG_FRAME: > https://lore.kernel.org/xen- > devel/8fdb98350ae4fc6029738d0aabe13a57e1945a50.1680086655.git.oleksii.kurochko@gmail.com/ That would be the ideal if someone has time for it. Otherwise, patch #3 needs to be modified (see my answer on patch #2). But I would also be ok with this as a stop-gap for the time being. Cheers, -- Julien Grall
On 10/02/2025 9:31 pm, Julien Grall wrote: > > > On 10/02/2025 09:21, Oleksii Kurochko wrote: >> >> On 2/8/25 1:02 AM, 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. >> >> Probably, we should give a chance for the patch which suggests to use >> GENERIC_BUG_FRAME: >> https://lore.kernel.org/xen- >> devel/8fdb98350ae4fc6029738d0aabe13a57e1945a50.1680086655.git.oleksii.kurochko@gmail.com/ > > That would be the ideal if someone has time for it. Otherwise, patch > #3 needs to be modified (see my answer on patch #2). > > But I would also be ok with this as a stop-gap for the time being. Getting ARM onto GENERIC_BUG_FRAME would definitely be best all around, but that is an almost-2-year-old patch with an open "it doesn't compile on ARM32" issue. I presume that all which is wanted is *a* solution that compiles (and works) everywhere we support? ~Andrew
Hi Andrew, On 10/02/2025 22:41, Andrew Cooper wrote: > On 10/02/2025 9:31 pm, Julien Grall wrote: >> >> >> On 10/02/2025 09:21, Oleksii Kurochko wrote: >>> >>> On 2/8/25 1:02 AM, 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. >>> >>> Probably, we should give a chance for the patch which suggests to use >>> GENERIC_BUG_FRAME: >>> https://lore.kernel.org/xen- >>> devel/8fdb98350ae4fc6029738d0aabe13a57e1945a50.1680086655.git.oleksii.kurochko@gmail.com/ >> >> That would be the ideal if someone has time for it. Otherwise, patch >> #3 needs to be modified (see my answer on patch #2). >> >> But I would also be ok with this as a stop-gap for the time being. > > Getting ARM onto GENERIC_BUG_FRAME would definitely be best all around, > but that is an almost-2-year-old patch with an open "it doesn't compile > on ARM32" issue. > > I presume that all which is wanted is *a* solution that compiles (and > works) everywhere we support? Correct. Looking at the previous e-mail, it sounds like the patch was meant to work but it wasn't tested with older compilers and the commit message needed some rewording to mention what was tested. Cheers, -- Julien Grall
© 2016 - 2026 Red Hat, Inc.