[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
|
