[PATCH v6 1/3] xen/arm: add support for run_in_exception_handler()
Add support to run a function in an exception handler for Arm. Do it as on x86 via a bug_frame, but pass the function pointer via a register (this needs to be done that way, because inline asm support for 32-bit Arm lacks the needed functionality to reference an arbitrary function via the bugframe). Use the same BUGFRAME_* #defines as on x86 in order to make a future common header file more easily achievable. Signed-off-by: Juergen Gross --- V4: - new patch V5: - adjust BUG_FRAME() macro (Jan Beulich) - adjust arm linker script (Jan Beulich) - drop #ifdef x86 in common/virtual_region.c V6: - use register for function address (Arm32 build failure noticed by Julien Grall) --- xen/arch/arm/traps.c| 8 xen/arch/arm/xen.lds.S | 2 ++ xen/common/virtual_region.c | 2 -- xen/include/asm-arm/arm32/bug.h | 2 ++ xen/include/asm-arm/arm64/bug.h | 2 ++ xen/include/asm-arm/bug.h | 31 +-- 6 files changed, 39 insertions(+), 8 deletions(-) diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 1af1bb9f1b..d0df33b218 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -1236,6 +1236,14 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc) if ( !bug ) return -ENOENT; +if ( id == BUGFRAME_run_fn ) +{ +void (*fn)(const struct cpu_user_regs *) = (void *)regs->BUG_FN_REG; + +fn(regs); +return 0; +} + /* WARN, BUG or ASSERT: decode the filename pointer and line number. */ filename = bug_file(bug); if ( !is_kernel(filename) ) diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index 6342ac4ead..004b182acb 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -49,6 +49,8 @@ SECTIONS __stop_bug_frames_1 = .; *(.bug_frames.2) __stop_bug_frames_2 = .; + *(.bug_frames.3) + __stop_bug_frames_3 = .; *(.rodata) *(.rodata.*) *(.data.rel.ro) diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c index 4fbc02e35a..30b0b4ab9c 100644 --- a/xen/common/virtual_region.c +++ b/xen/common/virtual_region.c @@ -123,9 +123,7 @@ void __init setup_virtual_regions(const struct exception_table_entry *start, __stop_bug_frames_0, __stop_bug_frames_1, __stop_bug_frames_2, -#ifdef CONFIG_X86 __stop_bug_frames_3, -#endif NULL }; diff --git a/xen/include/asm-arm/arm32/bug.h b/xen/include/asm-arm/arm32/bug.h index 3e66f35969..25cce151dc 100644 --- a/xen/include/asm-arm/arm32/bug.h +++ b/xen/include/asm-arm/arm32/bug.h @@ -10,4 +10,6 @@ #define BUG_INSTR ".word " __stringify(BUG_OPCODE) +#define BUG_FN_REG r0 + #endif /* __ARM_ARM32_BUG_H__ */ diff --git a/xen/include/asm-arm/arm64/bug.h b/xen/include/asm-arm/arm64/bug.h index 59f664d7de..5e11c0dfd5 100644 --- a/xen/include/asm-arm/arm64/bug.h +++ b/xen/include/asm-arm/arm64/bug.h @@ -6,4 +6,6 @@ #define BUG_INSTR "brk " __stringify(BRK_BUG_FRAME_IMM) +#define BUG_FN_REG x0 + #endif /* __ARM_ARM64_BUG_H__ */ diff --git a/xen/include/asm-arm/bug.h b/xen/include/asm-arm/bug.h index 36c803357c..e9341daef1 100644 --- a/xen/include/asm-arm/bug.h +++ b/xen/include/asm-arm/bug.h @@ -26,16 +26,17 @@ struct bug_frame { #define bug_line(b) ((b)->line) #define bug_msg(b) ((const char *)(b) + (b)->msg_disp) -#define BUGFRAME_warn 0 -#define BUGFRAME_bug1 -#define BUGFRAME_assert 2 +#define BUGFRAME_run_fn 0 +#define BUGFRAME_warn 1 +#define BUGFRAME_bug2 +#define BUGFRAME_assert 3 -#define BUGFRAME_NR 3 +#define BUGFRAME_NR 4 /* Many versions of GCC doesn't support the asm %c parameter which would * be preferable to this unpleasantness. We use mergeable string * sections to avoid multiple copies of the string appearing in the - * Xen image. + * Xen image. BUGFRAME_run_fn needs to be handled separately. */ #define BUG_FRAME(type, line, file, has_msg, msg) do { \ BUILD_BUG_ON((line) >> 16); \ @@ -58,6 +59,23 @@ struct bug_frame { ".popsection");\ } while (0) +/* + * Unfortunately gcc for arm 32-bit doesn't support the "i" constraint, so + * the easiest way to implement run_in_exception_handler() is to pass the + * to 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" \ + ".pushsection .bug_frames." __stringify(BUGFRAME_run_fn) "," \ + " \"a\", %%progbits\n" \ + "2:\n" \ + ".p2align 2\n" \ + ".l
Re: [PATCH v6 1/3] xen/arm: add support for run_in_exception_handler()
Hi Juergen, On 16/01/2021 10:33, Juergen Gross wrote: Add support to run a function in an exception handler for Arm. Do it as on x86 via a bug_frame, but pass the function pointer via a register (this needs to be done that way, because inline asm support for 32-bit Arm lacks the needed functionality to reference an arbitrary function via the bugframe). Use the same BUGFRAME_* #defines as on x86 in order to make a future common header file more easily achievable. Signed-off-by: Juergen Gross --- V4: - new patch V5: - adjust BUG_FRAME() macro (Jan Beulich) - adjust arm linker script (Jan Beulich) - drop #ifdef x86 in common/virtual_region.c V6: - use register for function address (Arm32 build failure noticed by Julien Grall) Thank you for trying to sort out the issue on Arm32 :). +/* + * Unfortunately gcc for arm 32-bit doesn't support the "i" constraint, so + * the easiest way to implement run_in_exception_handler() is to pass the + * to be called function in a fixed register. There are a few uses of "i" in Linux Arm32 (such as jump label), therefore I am pretty confident "i" works at least in some situation. I did some more digging this afternoon. Our use of "i" is very similar to Linux, so I decided to look at the GCC flags used. It turns out that Linux will always build with -fno-pie on Arm (either 32 or 64) whereas Xen will let the compiler to decide (PIE is enabled by on my compiler). I wrote a small example to test the issue (based on Linux static key) static void test(void) { } int main(void) { asm volatile (".pushsection .bug_frames, \"aw\"\n" ".quad %0\n" ".popsection\n" :: "i"(test)); return 0; } If I add -fno-pie on the command, the constraint error disappears. On the previous version, you rewrote that didn't see any error. Is it possible that your compiler is disabling PIE by default? I think we need to code to be position independent for at least UEFI. I also have plan to look at selecting the Xen virtual address at boot time (this is necessary to solve some memory issue on Arm). From a quick test, if I use -fno-pie -fpic, then the snipped above will build fine. But it is not entirely clear whether the code would still be position independent. I need to have a look how Linux is dealing with KASLR given that -fno-pie is used... + */ +#define run_in_exception_handler(fn) do { \ +asm ("mov " __stringify(BUG_FN_REG) ", %0\n"\ + "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) ); \ +} while (0) My concern with this approach is we are going to clobber multiple registers. On Arm64, this code will be replaced by: 13cc: 9001adrpx1, 0 13d0: 9121add x1, x1, #0x0 13d4: aa0103e0mov x0, x1 13d8: d4200020brk #0x1 I guess we can optimize it to just clobber one register. Do we expect the function executed to rely/replace the content of the registers? Cheers, -- Julien Grall
Re: [PATCH v6 1/3] xen/arm: add support for run_in_exception_handler()
On 16.01.21 18:19, Julien Grall wrote: Hi Juergen, On 16/01/2021 10:33, Juergen Gross wrote: Add support to run a function in an exception handler for Arm. Do it as on x86 via a bug_frame, but pass the function pointer via a register (this needs to be done that way, because inline asm support for 32-bit Arm lacks the needed functionality to reference an arbitrary function via the bugframe). Use the same BUGFRAME_* #defines as on x86 in order to make a future common header file more easily achievable. Signed-off-by: Juergen Gross --- V4: - new patch V5: - adjust BUG_FRAME() macro (Jan Beulich) - adjust arm linker script (Jan Beulich) - drop #ifdef x86 in common/virtual_region.c V6: - use register for function address (Arm32 build failure noticed by Julien Grall) Thank you for trying to sort out the issue on Arm32 :). +/* + * Unfortunately gcc for arm 32-bit doesn't support the "i" constraint, so + * the easiest way to implement run_in_exception_handler() is to pass the + * to be called function in a fixed register. There are a few uses of "i" in Linux Arm32 (such as jump label), therefore I am pretty confident "i" works at least in some situation. I did some more digging this afternoon. Our use of "i" is very similar to Linux, so I decided to look at the GCC flags used. It turns out that Linux will always build with -fno-pie on Arm (either 32 or 64) whereas Xen will let the compiler to decide (PIE is enabled by on my compiler). I wrote a small example to test the issue (based on Linux static key) static void test(void) { } int main(void) { asm volatile (".pushsection .bug_frames, \"aw\"\n" ".quad %0\n" ".popsection\n" :: "i"(test)); return 0; } If I add -fno-pie on the command, the constraint error disappears. On the previous version, you rewrote that didn't see any error. Is it possible that your compiler is disabling PIE by default? I think we need to code to be position independent for at least UEFI. I also have plan to look at selecting the Xen virtual address at boot time (this is necessary to solve some memory issue on Arm). From a quick test, if I use -fno-pie -fpic, then the snipped above will build fine. But it is not entirely clear whether the code would still be position independent. I need to have a look how Linux is dealing with KASLR given that -fno-pie is used... + */ +#define run_in_exception_handler(fn) do { \ + asm ("mov " __stringify(BUG_FN_REG) ", %0\n" \ + "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) ); \ +} while (0) My concern with this approach is we are going to clobber multiple registers. On Arm64, this code will be replaced by: 13cc: 9001 adrp x1, 0 13d0: 9121 add x1, x1, #0x0 13d4: aa0103e0 mov x0, x1 13d8: d4200020 brk #0x1 I guess we can optimize it to just clobber one register. Do we expect the function executed to rely/replace the content of the registers? No, it is just to have an interrupt frame to print out. Basically it is just a "normal" function call with no parameters and return value via an interrupt. So other than the BUG_ON() the registers are quite uninteresting, it is nothing meant to be used for diagnosis AFAICS. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: application/pgp-keys OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v6 1/3] xen/arm: add support for run_in_exception_handler()
Hi Juergen, On 16/01/2021 19:05, Jürgen Groß wrote: On 16.01.21 18:19, Julien Grall wrote: Hi Juergen, On 16/01/2021 10:33, Juergen Gross wrote: Add support to run a function in an exception handler for Arm. Do it as on x86 via a bug_frame, but pass the function pointer via a register (this needs to be done that way, because inline asm support for 32-bit Arm lacks the needed functionality to reference an arbitrary function via the bugframe). Use the same BUGFRAME_* #defines as on x86 in order to make a future common header file more easily achievable. Signed-off-by: Juergen Gross --- V4: - new patch V5: - adjust BUG_FRAME() macro (Jan Beulich) - adjust arm linker script (Jan Beulich) - drop #ifdef x86 in common/virtual_region.c V6: - use register for function address (Arm32 build failure noticed by Julien Grall) Thank you for trying to sort out the issue on Arm32 :). +/* + * Unfortunately gcc for arm 32-bit doesn't support the "i" constraint, so + * the easiest way to implement run_in_exception_handler() is to pass the + * to be called function in a fixed register. There are a few uses of "i" in Linux Arm32 (such as jump label), therefore I am pretty confident "i" works at least in some situation. I did some more digging this afternoon. Our use of "i" is very similar to Linux, so I decided to look at the GCC flags used. It turns out that Linux will always build with -fno-pie on Arm (either 32 or 64) whereas Xen will let the compiler to decide (PIE is enabled by on my compiler). I wrote a small example to test the issue (based on Linux static key) static void test(void) { } int main(void) { asm volatile (".pushsection .bug_frames, \"aw\"\n" ".quad %0\n" ".popsection\n" :: "i"(test)); return 0; } If I add -fno-pie on the command, the constraint error disappears. On the previous version, you rewrote that didn't see any error. Is it possible that your compiler is disabling PIE by default? I think we need to code to be position independent for at least UEFI. I also have plan to look at selecting the Xen virtual address at boot time (this is necessary to solve some memory issue on Arm). From a quick test, if I use -fno-pie -fpic, then the snipped above will build fine. But it is not entirely clear whether the code would still be position independent. I need to have a look how Linux is dealing with KASLR given that -fno-pie is used... + */ +#define run_in_exception_handler(fn) do { \ + asm ("mov " __stringify(BUG_FN_REG) ", %0\n" \ + "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) ); \ +} while (0) My concern with this approach is we are going to clobber multiple registers. On Arm64, this code will be replaced by: 13cc: 9001 adrp x1, 0 13d0: 9121 add x1, x1, #0x0 13d4: aa0103e0 mov x0, x1 13d8: d4200020 brk #0x1 I guess we can optimize it to just clobber one register. Do we expect the function executed to rely/replace the content of the registers? No, it is just to have an interrupt frame to print out. Basically it is just a "normal" function call with no parameters and return value via an interrupt. So other than the BUG_ON() the registers are quite uninteresting, it is nothing meant to be used for diagnosis AFAICS. Ok. Let stick with your version for now then: Acked-by: Julien Grall Cheers, -- Julien Grall
Re: [PATCH v6 1/3] xen/arm: add support for run_in_exception_handler()
Hi Juergen, On 16/01/2021 10:33, Juergen Gross wrote: Add support to run a function in an exception handler for Arm. Do it as on x86 via a bug_frame, but pass the function pointer via a register (this needs to be done that way, because inline asm support for 32-bit Arm lacks the needed functionality to reference an arbitrary function via the bugframe). I was going to commit the series, but then realized the commit message and comment needs some tweaking because technically GCC is supporting 'i' (I managed to get it working with -fno-pie). So how about: "This is needed to be done that way because GCC complains about invalid constraint when using a function pointer with "i" and PIE is enabled (default on most of GCC shipped with distro). Clang happily accepts it, so it may be a bug in GCC." +/* + * Unfortunately gcc for arm 32-bit doesn't support the "i" constraint, so + * the easiest way to implement run_in_exception_handler() is to pass the + * to be called function in a fixed register. + */ This comment should also be updated. I can update both while committing if you are happy with the change. +#define run_in_exception_handler(fn) do { \ +asm ("mov " __stringify(BUG_FN_REG) ", %0\n"\ + "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) ); \ +} while (0) + #define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, "") #define BUG() do { \ @@ -73,7 +91,8 @@ struct bug_frame { extern const struct bug_frame __start_bug_frames[], __stop_bug_frames_0[], __stop_bug_frames_1[], - __stop_bug_frames_2[]; + __stop_bug_frames_2[], + __stop_bug_frames_3[]; #endif /* __ARM_BUG_H__ */ /* Cheers, -- Julien Grall
Re: [PATCH v6 1/3] xen/arm: add support for run_in_exception_handler()
On 20.01.21 19:20, Julien Grall wrote: Hi Juergen, On 16/01/2021 10:33, Juergen Gross wrote: Add support to run a function in an exception handler for Arm. Do it as on x86 via a bug_frame, but pass the function pointer via a register (this needs to be done that way, because inline asm support for 32-bit Arm lacks the needed functionality to reference an arbitrary function via the bugframe). I was going to commit the series, but then realized the commit message and comment needs some tweaking because technically GCC is supporting 'i' (I managed to get it working with -fno-pie). So how about: "This is needed to be done that way because GCC complains about invalid constraint when using a function pointer with "i" and PIE is enabled (default on most of GCC shipped with distro). Clang happily accepts it, so it may be a bug in GCC." +/* + * Unfortunately gcc for arm 32-bit doesn't support the "i" constraint, so + * the easiest way to implement run_in_exception_handler() is to pass the + * to be called function in a fixed register. + */ This comment should also be updated. I can update both while committing if you are happy with the change. Fine with me. Thanks, Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: application/pgp-keys OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v6 1/3] xen/arm: add support for run_in_exception_handler()
On 20.01.2021 19:20, Julien Grall wrote: > On 16/01/2021 10:33, Juergen Gross wrote: >> Add support to run a function in an exception handler for Arm. Do it >> as on x86 via a bug_frame, but pass the function pointer via a >> register (this needs to be done that way, because inline asm support >> for 32-bit Arm lacks the needed functionality to reference an >> arbitrary function via the bugframe). > > I was going to commit the series, but then realized the commit message > and comment needs some tweaking because technically GCC is supporting > 'i' (I managed to get it working with -fno-pie). > > So how about: > > "This is needed to be done that way because GCC complains about invalid > constraint when using a function pointer with "i" and PIE is enabled > (default on most of GCC shipped with distro). Clang happily accepts it, > so it may be a bug in GCC." May I ask why you think it's a bug in gcc? I'd rather consider it a bug in clang. Taking the address of a symbol doesn't yield a constant in PIC or PIE code, aiui. Jan
Re: [PATCH v6 1/3] xen/arm: add support for run_in_exception_handler()
On 21.01.21 08:55, Jan Beulich wrote: On 20.01.2021 19:20, Julien Grall wrote: On 16/01/2021 10:33, Juergen Gross wrote: Add support to run a function in an exception handler for Arm. Do it as on x86 via a bug_frame, but pass the function pointer via a register (this needs to be done that way, because inline asm support for 32-bit Arm lacks the needed functionality to reference an arbitrary function via the bugframe). I was going to commit the series, but then realized the commit message and comment needs some tweaking because technically GCC is supporting 'i' (I managed to get it working with -fno-pie). So how about: "This is needed to be done that way because GCC complains about invalid constraint when using a function pointer with "i" and PIE is enabled (default on most of GCC shipped with distro). Clang happily accepts it, so it may be a bug in GCC." May I ask why you think it's a bug in gcc? I'd rather consider it a bug in clang. Taking the address of a symbol doesn't yield a constant in PIC or PIE code, aiui. Maybe we should just not add the suspect of a bug in the compiler to either commit message or a comment. It could be a bug in gcc, or just a shortcoming (feature combination not supported). It could be a bug in clang, or clang is clever enough to produce correct code for "i" + PIE. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: application/pgp-keys OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v6 1/3] xen/arm: add support for run_in_exception_handler()
On 21.01.2021 09:00, Jürgen Groß wrote: > On 21.01.21 08:55, Jan Beulich wrote: >> On 20.01.2021 19:20, Julien Grall wrote: >>> On 16/01/2021 10:33, Juergen Gross wrote: Add support to run a function in an exception handler for Arm. Do it as on x86 via a bug_frame, but pass the function pointer via a register (this needs to be done that way, because inline asm support for 32-bit Arm lacks the needed functionality to reference an arbitrary function via the bugframe). >>> >>> I was going to commit the series, but then realized the commit message >>> and comment needs some tweaking because technically GCC is supporting >>> 'i' (I managed to get it working with -fno-pie). >>> >>> So how about: >>> >>> "This is needed to be done that way because GCC complains about invalid >>> constraint when using a function pointer with "i" and PIE is enabled >>> (default on most of GCC shipped with distro). Clang happily accepts it, >>> so it may be a bug in GCC." >> >> May I ask why you think it's a bug in gcc? I'd rather consider it >> a bug in clang. Taking the address of a symbol doesn't yield a >> constant in PIC or PIE code, aiui. > > Maybe we should just not add the suspect of a bug in the compiler to > either commit message or a comment. > > It could be a bug in gcc, or just a shortcoming (feature combination > not supported). > > It could be a bug in clang, or clang is clever enough to produce > correct code for "i" + PIE. I think the last option can be excluded. There simply is no room for cleverness. If an insn requires an immediate operand, the compiler has to find a compile time constant (possibly needing a relocation, but only PC-relative ones are permitted then). The compiler may in particular not inspect the insn(s) specified and try to substitute them. "i" (with a symbol ref) and PIE (or PIC) simply are incompatible with one another. One could imagine trickery requiring agreement between programmer and compiler, where the programmer would be to specify the relocation to use (and then do the necessary calculations to yield the actual symbol address), but that's not applicable here. I've experimented a little with gcc10 on x86-64. It behaves quite strangely, accepting some symbol references but not others (see example below). None of them get accepted with -fpic, and the ones that do get accepted with -fpie don't result in position independent code (or my understanding thereof is wrong), or to be precise in relocations that will have to be carried into the final executable because of being dependent upon the resulting program's load address. I'm pondering whether to open a bug ... Jan void efn(void); void(*efp)(void); void*test(int i) { void*res; efn(); efp(); switch(i) { case 0: asm("mov %1,%0" : "=r" (res) : "i" (test)); break; #ifndef __PIE__ case 1: asm("mov %1,%0" : "=r" (res) : "i" (efn)); break; #endif case 2: asm("mov %1,%0" : "=r" (res) : "i" (&efp)); break; default: res = (void*)0; break; } return res; }
Re: [PATCH v6 1/3] xen/arm: add support for run_in_exception_handler()
Hi Jan, On 21/01/2021 07:55, Jan Beulich wrote: On 20.01.2021 19:20, Julien Grall wrote: On 16/01/2021 10:33, Juergen Gross wrote: Add support to run a function in an exception handler for Arm. Do it as on x86 via a bug_frame, but pass the function pointer via a register (this needs to be done that way, because inline asm support for 32-bit Arm lacks the needed functionality to reference an arbitrary function via the bugframe). I was going to commit the series, but then realized the commit message and comment needs some tweaking because technically GCC is supporting 'i' (I managed to get it working with -fno-pie). So how about: "This is needed to be done that way because GCC complains about invalid constraint when using a function pointer with "i" and PIE is enabled (default on most of GCC shipped with distro). Clang happily accepts it, so it may be a bug in GCC." May I ask why you think it's a bug in gcc? I'd rather consider it a bug in clang. Taking the address of a symbol doesn't yield a constant in PIC or PIE code, aiui. I consider it a bug because we were using it as: .pushsection .bug.frame 2b: .long (%0 - 2b) So I expect the compiler to be able to find the displacement in both cases as we don't need to know the exact address. I think Clang is just clever enough to figure out we want a displacement. Do you have a suggestion of constraint that could resolve the issue? Cheers, -- Julien Grall
Re: [PATCH v6 1/3] xen/arm: add support for run_in_exception_handler()
On 21.01.2021 10:50, Julien Grall wrote: > Hi Jan, > > On 21/01/2021 07:55, Jan Beulich wrote: >> On 20.01.2021 19:20, Julien Grall wrote: >>> On 16/01/2021 10:33, Juergen Gross wrote: Add support to run a function in an exception handler for Arm. Do it as on x86 via a bug_frame, but pass the function pointer via a register (this needs to be done that way, because inline asm support for 32-bit Arm lacks the needed functionality to reference an arbitrary function via the bugframe). >>> >>> I was going to commit the series, but then realized the commit message >>> and comment needs some tweaking because technically GCC is supporting >>> 'i' (I managed to get it working with -fno-pie). >>> >>> So how about: >>> >>> "This is needed to be done that way because GCC complains about invalid >>> constraint when using a function pointer with "i" and PIE is enabled >>> (default on most of GCC shipped with distro). Clang happily accepts it, >>> so it may be a bug in GCC." >> >> May I ask why you think it's a bug in gcc? I'd rather consider it >> a bug in clang. Taking the address of a symbol doesn't yield a >> constant in PIC or PIE code, aiui. > > I consider it a bug because we were using it as: > > .pushsection .bug.frame > 2b: > .long (%0 - 2b) > > So I expect the compiler to be able to find the displacement in both > cases as we don't need to know the exact address. > > I think Clang is just clever enough to figure out we want a displacement. If they take apart the contents of the asm(), this may be possible, yes. (Did you try with -no-integrated-as, btw?) But taking apart the asm() is a very risky game a compiler would play, as it may easily break the programmer's intentions (or still fail to recognize whether the use is okay, for the containing construct being too complex). > Do you have a suggestion of constraint that could resolve the issue? No. Don't use -fpie (but I guess that's not an option for other reasons). Jan
Re: [PATCH v6 1/3] xen/arm: add support for run_in_exception_handler()
Hi Jan, On 21/01/2021 10:06, Jan Beulich wrote: On 21.01.2021 10:50, Julien Grall wrote: Hi Jan, On 21/01/2021 07:55, Jan Beulich wrote: On 20.01.2021 19:20, Julien Grall wrote: On 16/01/2021 10:33, Juergen Gross wrote: Add support to run a function in an exception handler for Arm. Do it as on x86 via a bug_frame, but pass the function pointer via a register (this needs to be done that way, because inline asm support for 32-bit Arm lacks the needed functionality to reference an arbitrary function via the bugframe). I was going to commit the series, but then realized the commit message and comment needs some tweaking because technically GCC is supporting 'i' (I managed to get it working with -fno-pie). So how about: "This is needed to be done that way because GCC complains about invalid constraint when using a function pointer with "i" and PIE is enabled (default on most of GCC shipped with distro). Clang happily accepts it, so it may be a bug in GCC." May I ask why you think it's a bug in gcc? I'd rather consider it a bug in clang. Taking the address of a symbol doesn't yield a constant in PIC or PIE code, aiui. I consider it a bug because we were using it as: .pushsection .bug.frame 2b: .long (%0 - 2b) So I expect the compiler to be able to find the displacement in both cases as we don't need to know the exact address. I think Clang is just clever enough to figure out we want a displacement. If they take apart the contents of the asm(), this may be possible, yes. (Did you try with -no-integrated-as, btw?). Clang will be able to build the code with and without -no-integrated-as. The dump of the structure will show that the address of the function will be stored (I used this small snippet [1]). [...] Do you have a suggestion of constraint that could resolve the issue? No. Don't use -fpie (but I guess that's not an option for other reasons). Yes, we need to have Xen code relocatable for at least UEFI. In the future we will need to be able to change at runtime the Xen virtual address in order to address memory issues (we can't switch page-tables with MMU enabled). Linux Arm64 supports KASLR, yet, they are building with -fno-pie. I need to figure out how they deal with relocating the kernel and see if we can re-use it in Xen. I have revised the comment to not suggest this is a bug and commit the series. Cheers, [1] int test(void) { return 0; } int main(void) { asm volatile (".pushsection .bug_frames, \"aw\"\n" ".quad %0\n" ".popsection\n" :: "i"(test)); return 0; } -- Julien Grall