[PATCH v6 0/3] xen: add support for automatic debug key actions in case of crash

[PATCH v6 0/3] xen: add support for automatic debug key actions in case of crash

[Juergen Gross]

When the host crashes it would sometimes be nice to have additional
debug data available which could be produced via debug keys, but
halting the server for manual intervention might be impossible due to
the need to reboot/kexec rather sooner than later.

Add support for automatic debug key actions in case of crashes which
can be activated via boot- or runtime-parameter.

Changes in V4:
- addressed comments (now patch 3)
- added patches 1 and 2

Changes in V5:
- better bug frame construction on Arm (patch 1)
- addressed comments

Changes in V6:
- rework of patch 1 due to Arm 32-bit problems

Juergen Gross (3):
  xen/arm: add support for run_in_exception_handler()
  xen: enable keyhandlers to work without register set specified
  xen: add support for automatic debug key actions in case of crash

 docs/misc/xen-command-line.pandoc | 41 ++++++++++++++++++
 xen/arch/arm/traps.c              |  8 ++++
 xen/arch/arm/xen.lds.S            |  2 +
 xen/common/kexec.c                |  8 ++--
 xen/common/keyhandler.c           | 72 +++++++++++++++++++++++++++++--
 xen/common/shutdown.c             |  4 +-
 xen/common/virtual_region.c       |  2 -
 xen/drivers/char/console.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 ++++++++++---
 xen/include/xen/kexec.h           | 10 ++++-
 xen/include/xen/keyhandler.h      | 10 +++++
 13 files changed, 175 insertions(+), 19 deletions(-)

-- 
2.26.2

[PATCH v6 1/3] xen/arm: add support for run_in_exception_handler()

[Juergen Gross]

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 <jgross@suse.com>
---
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_bug    1
-#define BUGFRAME_assert 2
+#define BUGFRAME_run_fn 0
+#define BUGFRAME_warn   1
+#define BUGFRAME_bug    2
+#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"                                                     \
+         ".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__ */
 /*
-- 
2.26.2

[PATCH v6 2/3] xen: enable keyhandlers to work without register set specified

[Juergen Gross]

There are only two keyhandlers which make use of the cpu_user_regs
struct passed to them. In order to be able to call any keyhandler in
non-interrupt contexts, too, modify those two handlers to cope with a
NULL regs pointer by using run_in_exception_handler() in that case.

Suggested-by: Julien Grall <julien@xen.org>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V4:
- new patch
---
 xen/common/keyhandler.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index 68364e987d..38020a1360 100644
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -181,7 +181,10 @@ static void dump_registers(unsigned char key, struct cpu_user_regs *regs)
     cpumask_copy(&dump_execstate_mask, &cpu_online_map);
 
     /* Get local execution state out immediately, in case we get stuck. */
-    dump_execstate(regs);
+    if ( regs )
+        dump_execstate(regs);
+    else
+        run_in_exception_handler(dump_execstate);
 
     /* Alt. handling: remaining CPUs are dumped asynchronously one-by-one. */
     if ( alt_key_handling )
@@ -481,15 +484,23 @@ static void run_all_keyhandlers(unsigned char key, struct cpu_user_regs *regs)
     tasklet_schedule(&run_all_keyhandlers_tasklet);
 }
 
-static void do_debug_key(unsigned char key, struct cpu_user_regs *regs)
+static void do_debugger_trap_fatal(struct cpu_user_regs *regs)
 {
-    printk("'%c' pressed -> trapping into debugger\n", key);
     (void)debugger_trap_fatal(0xf001, regs);
 
     /* Prevent tail call optimisation, which confuses xendbg. */
     barrier();
 }
 
+static void do_debug_key(unsigned char key, struct cpu_user_regs *regs)
+{
+    printk("'%c' pressed -> trapping into debugger\n", key);
+    if ( regs )
+        do_debugger_trap_fatal(regs);
+    else
+        run_in_exception_handler(do_debugger_trap_fatal);
+}
+
 static void do_toggle_alt_key(unsigned char key, struct cpu_user_regs *regs)
 {
     alt_key_handling = !alt_key_handling;
-- 
2.26.2

[PATCH v6 3/3] xen: add support for automatic debug key actions in case of crash

[Juergen Gross]

When the host crashes it would sometimes be nice to have additional
debug data available which could be produced via debug keys, but
halting the server for manual intervention might be impossible due to
the need to reboot/kexec rather sooner than later.

Add support for automatic debug key actions in case of crashes which
can be activated via boot- or runtime-parameter.

Depending on the type of crash the desired data might be different, so
support different settings for the possible types of crashes.

The parameter is "crash-debug" with the following syntax:

  crash-debug-<type>=<string>

with <type> being one of:

  panic, hwdom, watchdog, kexeccmd, debugkey

and <string> a sequence of debug key characters with '+' having the
special semantics of a 10 millisecond pause.

So "crash-debug-watchdog=0+0qr" would result in special output in case
of watchdog triggered crash (dom0 state, 10 ms pause, dom0 state,
domain info, run queues).

Don't call key handlers in early boot, as some (e.g. for 'd') require
some initializations to be finished, like scheduler or idle domain.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
V2:
- switched special character '.' to '+' (Jan Beulich)
- 10 ms instead of 1 s pause (Jan Beulich)
- added more text to the boot parameter description (Jan Beulich)

V3:
- added const (Jan Beulich)
- thorough test of crash reason parameter (Jan Beulich)
- kexeccmd case should depend on CONFIG_KEXEC (Jan Beulich)
- added dummy get_irq_regs() helper on Arm

V4:
- call keyhandlers with NULL for regs
- use ARRAY_SIZE() (Jan Beulich)
- don't activate handlers in early boot (Jan Beulich)
- avoid recursion
- extend documentation a bit

V5:
- move recursion check down a little bit (Jan Beulich)
---
 docs/misc/xen-command-line.pandoc | 41 +++++++++++++++++++++++
 xen/common/kexec.c                |  8 +++--
 xen/common/keyhandler.c           | 55 +++++++++++++++++++++++++++++++
 xen/common/shutdown.c             |  4 +--
 xen/drivers/char/console.c        |  2 +-
 xen/include/xen/kexec.h           | 10 ++++--
 xen/include/xen/keyhandler.h      | 10 ++++++
 7 files changed, 122 insertions(+), 8 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 7ec1e804b7..a713a0535c 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -574,6 +574,47 @@ reduction of features at Xen's disposal to manage guests.
 ### cpuinfo (x86)
 > `= <boolean>`
 
+### crash-debug-debugkey
+### crash-debug-hwdom
+### crash-debug-kexeccmd
+### crash-debug-panic
+### crash-debug-watchdog
+> `= <string>`
+
+> Can be modified at runtime
+
+Specify debug-key actions in cases of crashes. Each of the parameters applies
+to a different crash reason. The `<string>` is a sequence of debug key
+characters, with `+` having the special meaning of a 10 millisecond pause.
+
+`crash-debug-debugkey` will be used for crashes induced by the `C` debug
+key (i.e. manually induced crash).
+
+`crash-debug-hwdom` denotes a crash of dom0.
+
+`crash-debug-kexeccmd` is an explicit request of dom0 to continue with the
+kdump kernel via kexec. Only available on hypervisors built with CONFIG_KEXEC.
+
+`crash-debug-panic` is a crash of the hypervisor.
+
+`crash-debug-watchdog` is a crash due to the watchdog timer expiring.
+
+It should be noted that dumping diagnosis data to the console can fail in
+multiple ways (missing data, hanging system, ...) depending on the reason
+of the crash, which might have left the hypervisor in a bad state. In case
+a debug-key action leads to another crash recursion will be avoided, so no
+additional debug-key actions will be performed in this case. A crash in the
+early boot phase will not result in any debug-key action, as the system
+might not yet be in a state where the handlers can work.
+
+So e.g. `crash-debug-watchdog=0+0r` would dump dom0 state twice with 10
+milliseconds between the two state dumps, followed by the run queues of the
+hypervisor, if the system crashes due to a watchdog timeout.
+
+Depending on the reason of the system crash it might happen that triggering
+some debug key action will result in a hang instead of dumping data and then
+doing a reboot or crash dump.
+
 ### crashinfo_maxaddr
 > `= <size>`
 
diff --git a/xen/common/kexec.c b/xen/common/kexec.c
index 52cdc4ebc3..ebeee6405a 100644
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -373,10 +373,12 @@ static int kexec_common_shutdown(void)
     return 0;
 }
 
-void kexec_crash(void)
+void kexec_crash(enum crash_reason reason)
 {
     int pos;
 
+    keyhandler_crash_action(reason);
+
     pos = (test_bit(KEXEC_FLAG_CRASH_POS, &kexec_flags) != 0);
     if ( !test_bit(KEXEC_IMAGE_CRASH_BASE + pos, &kexec_flags) )
         return;
@@ -409,7 +411,7 @@ static long kexec_reboot(void *_image)
 static void do_crashdump_trigger(unsigned char key)
 {
     printk("'%c' pressed -> triggering crashdump\n", key);
-    kexec_crash();
+    kexec_crash(CRASHREASON_DEBUGKEY);
     printk(" * no crash kernel loaded!\n");
 }
 
@@ -840,7 +842,7 @@ static int kexec_exec(XEN_GUEST_HANDLE_PARAM(void) uarg)
         ret = continue_hypercall_on_cpu(0, kexec_reboot, image);
         break;
     case KEXEC_TYPE_CRASH:
-        kexec_crash(); /* Does not return */
+        kexec_crash(CRASHREASON_KEXECCMD); /* Does not return */
         break;
     }
 
diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index 38020a1360..8b9f378371 100644
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -3,7 +3,9 @@
  */
 
 #include <asm/regs.h>
+#include <xen/delay.h>
 #include <xen/keyhandler.h>
+#include <xen/param.h>
 #include <xen/shutdown.h>
 #include <xen/event.h>
 #include <xen/console.h>
@@ -518,6 +520,59 @@ void __init initialize_keytable(void)
     }
 }
 
+#define CRASHACTION_SIZE  32
+static char crash_debug_panic[CRASHACTION_SIZE];
+string_runtime_param("crash-debug-panic", crash_debug_panic);
+static char crash_debug_hwdom[CRASHACTION_SIZE];
+string_runtime_param("crash-debug-hwdom", crash_debug_hwdom);
+static char crash_debug_watchdog[CRASHACTION_SIZE];
+string_runtime_param("crash-debug-watchdog", crash_debug_watchdog);
+#ifdef CONFIG_KEXEC
+static char crash_debug_kexeccmd[CRASHACTION_SIZE];
+string_runtime_param("crash-debug-kexeccmd", crash_debug_kexeccmd);
+#else
+#define crash_debug_kexeccmd NULL
+#endif
+static char crash_debug_debugkey[CRASHACTION_SIZE];
+string_runtime_param("crash-debug-debugkey", crash_debug_debugkey);
+
+void keyhandler_crash_action(enum crash_reason reason)
+{
+    static const char *const crash_action[] = {
+        [CRASHREASON_PANIC] = crash_debug_panic,
+        [CRASHREASON_HWDOM] = crash_debug_hwdom,
+        [CRASHREASON_WATCHDOG] = crash_debug_watchdog,
+        [CRASHREASON_KEXECCMD] = crash_debug_kexeccmd,
+        [CRASHREASON_DEBUGKEY] = crash_debug_debugkey,
+    };
+    static bool ignore;
+    const char *action;
+
+    /* Some handlers are not functional too early. */
+    if ( system_state < SYS_STATE_smp_boot )
+        return;
+
+    if ( (unsigned int)reason >= ARRAY_SIZE(crash_action) )
+        return;
+    action = crash_action[reason];
+    if ( !action )
+        return;
+
+    /* Avoid recursion. */
+    if ( ignore )
+        return;
+    ignore = true;
+
+    while ( *action )
+    {
+        if ( *action == '+' )
+            mdelay(10);
+        else
+            handle_keypress(*action, NULL);
+        action++;
+    }
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/common/shutdown.c b/xen/common/shutdown.c
index 912593915b..abde48aa4c 100644
--- a/xen/common/shutdown.c
+++ b/xen/common/shutdown.c
@@ -43,7 +43,7 @@ void hwdom_shutdown(u8 reason)
     case SHUTDOWN_crash:
         debugger_trap_immediate();
         printk("Hardware Dom%u crashed: ", hardware_domain->domain_id);
-        kexec_crash();
+        kexec_crash(CRASHREASON_HWDOM);
         maybe_reboot();
         break; /* not reached */
 
@@ -56,7 +56,7 @@ void hwdom_shutdown(u8 reason)
     case SHUTDOWN_watchdog:
         printk("Hardware Dom%u shutdown: watchdog rebooting machine\n",
                hardware_domain->domain_id);
-        kexec_crash();
+        kexec_crash(CRASHREASON_WATCHDOG);
         machine_restart(0);
         break; /* not reached */
 
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index e3c483fd13..2358375170 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -1271,7 +1271,7 @@ void panic(const char *fmt, ...)
 
     debugger_trap_immediate();
 
-    kexec_crash();
+    kexec_crash(CRASHREASON_PANIC);
 
     if ( opt_noreboot )
         machine_halt();
diff --git a/xen/include/xen/kexec.h b/xen/include/xen/kexec.h
index e85ba16405..9f7a912e97 100644
--- a/xen/include/xen/kexec.h
+++ b/xen/include/xen/kexec.h
@@ -1,6 +1,8 @@
 #ifndef __XEN_KEXEC_H__
 #define __XEN_KEXEC_H__
 
+#include <xen/keyhandler.h>
+
 #ifdef CONFIG_KEXEC
 
 #include <public/kexec.h>
@@ -48,7 +50,7 @@ void machine_kexec_unload(struct kexec_image *image);
 void machine_kexec_reserved(xen_kexec_reserve_t *reservation);
 void machine_reboot_kexec(struct kexec_image *image);
 void machine_kexec(struct kexec_image *image);
-void kexec_crash(void);
+void kexec_crash(enum crash_reason reason);
 void kexec_crash_save_cpu(void);
 struct crash_xen_info *kexec_crash_save_info(void);
 void machine_crash_shutdown(void);
@@ -82,7 +84,11 @@ void vmcoreinfo_append_str(const char *fmt, ...)
 #define kexecing 0
 
 static inline void kexec_early_calculations(void) {}
-static inline void kexec_crash(void) {}
+static inline void kexec_crash(enum crash_reason reason)
+{
+    keyhandler_crash_action(reason);
+}
+
 static inline void kexec_crash_save_cpu(void) {}
 static inline void set_kexec_crash_area_size(u64 system_ram) {}
 
diff --git a/xen/include/xen/keyhandler.h b/xen/include/xen/keyhandler.h
index 5131e86cbc..9c5830a037 100644
--- a/xen/include/xen/keyhandler.h
+++ b/xen/include/xen/keyhandler.h
@@ -48,4 +48,14 @@ void register_irq_keyhandler(unsigned char key,
 /* Inject a keypress into the key-handling subsystem. */
 extern void handle_keypress(unsigned char key, struct cpu_user_regs *regs);
 
+enum crash_reason {
+    CRASHREASON_PANIC,
+    CRASHREASON_HWDOM,
+    CRASHREASON_WATCHDOG,
+    CRASHREASON_KEXECCMD,
+    CRASHREASON_DEBUGKEY,
+};
+
+void keyhandler_crash_action(enum crash_reason reason);
+
 #endif /* __XEN_KEYHANDLER_H__ */
-- 
2.26.2

Re: [PATCH v6 1/3] xen/arm: add support for run_in_exception_handler()

[Julien Grall]

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 <jgross@suse.com>
> ---
> 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:       90000001        adrp    x1, 0 <show_execution_state>
     13d0:       91000021        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?

Cheers,

-- 
Julien Grall

Re: [PATCH v6 1/3] xen/arm: add support for run_in_exception_handler()

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 4954 bytes --]

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 <jgross@suse.com>
>> ---
>> 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:       90000001        adrp    x1, 0 <show_execution_state>
>      13d0:       91000021        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


[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v6 1/3] xen/arm: add support for run_in_exception_handler()

[Julien Grall]

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 <jgross@suse.com>
>>> ---
>>> 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:       90000001        adrp    x1, 0 <show_execution_state>
>>      13d0:       91000021        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 <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v6 2/3] xen: enable keyhandlers to work without register set specified

[Julien Grall]

Hi Juergen,

On 16/01/2021 10:33, Juergen Gross wrote:
> There are only two keyhandlers which make use of the cpu_user_regs
> struct passed to them. In order to be able to call any keyhandler in
> non-interrupt contexts, too, modify those two handlers to cope with a
> NULL regs pointer by using run_in_exception_handler() in that case.
> 
> Suggested-by: Julien Grall <julien@xen.org>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v6 1/3] xen/arm: add support for run_in_exception_handler()

[Julien Grall]

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

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1317 bytes --]

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

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v6 1/3] xen/arm: add support for run_in_exception_handler()

[Jan Beulich]

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

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1431 bytes --]

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


[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v6 1/3] xen/arm: add support for run_in_exception_handler()

[Jan Beulich]

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

[Julien Grall]

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

[Jan Beulich]

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

[Julien Grall]

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