[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 14.12.20 12:14, Julien Grall wrote:
Hi Juergen,

On 14/12/2020 10:51, Jürgen Groß wrote:
On 14.12.20 11:17, Julien Grall wrote:
Hi Juergen,

On 14/12/2020 07:56, Juergen Gross wrote:
Add support to run a function in an exception handler for Arm. Do it
the same way as on x86 via a bug_frame.

Unfortunately inline assembly on Arm seems to be less capable than on
x86, leading to functions called via run_in_exception_handler() having
to be globally visible.

Jan already commented on this, so I am not going to comment again.

Maybe I can ask some Arm specific question related to this:

In my experiments the only working solution was using the "i" constraint
for the function pointer. Do you know whether this is supported for all
gcc versions we care about?

I don't know for sure. However, Linux has been using "i" since 2012. So I would assume it ought to be fine for all the version we care.


Or is there another way to achieve the desired functionality? I'm using
now the following macros:

#define BUG_FRAME_run_fn(fn) do {                                      \
     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 - 2b)\n"                                           \
          ".long 0\n"                                                   \
          ".hword 0, 0\n"                                               \
          ".popsection" :: "i" (fn));                                   \
} while (0)

May I ask why we need a new macro?

Using a common one might be possible, but not with the current way how
BUG_FRAME() is defined: gcc complained about the input parameter in case
of ASSERT() and WARN().

I might be missing something, but this was the fastest way to at least
confirm the scheme is working for Arm.



#define run_in_exception_handler(fn) BUG_FRAME_run_fn(fn)


Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
---
V4:
- new patch

I have verified the created bugframe is correct by inspecting the
created binary.

Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
---
  xen/arch/arm/traps.c       | 10 +++++++++-
  xen/drivers/char/ns16550.c |  3 ++-
  xen/include/asm-arm/bug.h  | 32 +++++++++++++++++++++-----------
  3 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 22bd1bd4c6..6e677affe2 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1236,8 +1236,16 @@ 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 *) = bug_ptr(bug);
+
+        fn(regs);
+        return 0;
+    }
+
      /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
-    filename = bug_file(bug);
+    filename = bug_ptr(bug);
      if ( !is_kernel(filename) )
          return -EINVAL;
      fixup = strlen(filename);
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 9235d854fe..dd6500acc8 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -192,7 +192,8 @@ static void ns16550_interrupt(
  /* Safe: ns16550_poll() runs as softirq so not reentrant on a given CPU. */
  static DEFINE_PER_CPU(struct serial_port *, poll_port);
-static void __ns16550_poll(struct cpu_user_regs *regs)
+/* run_in_exception_handler() on Arm requires globally visible symbol. */
+void __ns16550_poll(struct cpu_user_regs *regs)
  {
      struct serial_port *port = this_cpu(poll_port);
      struct ns16550 *uart = port->uart;
diff --git a/xen/include/asm-arm/bug.h b/xen/include/asm-arm/bug.h
index 36c803357c..a7da2c306f 100644
--- a/xen/include/asm-arm/bug.h
+++ b/xen/include/asm-arm/bug.h
@@ -15,34 +15,38 @@
  struct bug_frame {
      signed int loc_disp;    /* Relative address to the bug address */
-    signed int file_disp;   /* Relative address to the filename */
+    signed int ptr_disp;    /* Relative address to the filename or function */       signed int msg_disp;    /* Relative address to the predicate (for ASSERT) */
      uint16_t line;          /* Line number */
      uint32_t pad0:16;       /* Padding for 8-bytes align */
  };
  #define bug_loc(b) ((const void *)(b) + (b)->loc_disp)
-#define bug_file(b) ((const void *)(b) + (b)->file_disp);
+#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp);
  #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

Why did you renumber it? IOW, why can't BUGFRAME_run_fn be defined as 3?

This matches x86 definition. IMO there is no reason to have a different
definition and this will make it more obvious that it might be a good
idea to have a common include/xen/bug.h header.

I agree that common header would be nice. Although, I am not sure if this is achievable. However, my point here is this change would have deserved half-sentence in the commit message because to me this look like unwanted churn.

Okay.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.