| Message ID | 20201214075615.25038-2-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: add support for automatic debug key actions in case of crash | expand |
On 14.12.2020 08: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. Could you extend on this? I don't understand what the relevant difference is, from just looking at the changes. > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > V4: > - new patch > > I have verified the created bugframe is correct by inspecting the > created binary. > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > 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(-) Aiui you also need to modify xen.lds.S to cover the new (or really the last renamed) section. Jan
On 14.12.20 10:03, Jan Beulich wrote: > On 14.12.2020 08: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. > > Could you extend on this? I don't understand what the relevant > difference is, from just looking at the changes. The problem seems to be that a static symbol referenced from the inline asm seems not to silence the error that this static symbol isn't being used. On x86 the bug_frame is constructed using the %c modifier, which is not supported for Arm (at least gcc 7 used in my compile test complained), but seems to be enough for gcc on x86 to not complain. > >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> V4: >> - new patch >> >> I have verified the created bugframe is correct by inspecting the >> created binary. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> 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(-) > > Aiui you also need to modify xen.lds.S to cover the new (or really > the last renamed) section. Oh, right. I thought of that before, but forgot again. Thanks for the reminder. Juergen
On 14.12.2020 10:15, Jürgen Groß wrote: > On 14.12.20 10:03, Jan Beulich wrote: >> On 14.12.2020 08: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. >> >> Could you extend on this? I don't understand what the relevant >> difference is, from just looking at the changes. > > The problem seems to be that a static symbol referenced from the inline > asm seems not to silence the error that this static symbol isn't being > used. On x86 the bug_frame is constructed using the %c modifier, which > is not supported for Arm (at least gcc 7 used in my compile test > complained), but seems to be enough for gcc on x86 to not complain. But this isn't tied to %c not working on older gcc, is it? It looks instead to be tied to you not specifying the function pointer as an input in the asm(). The compiler would know the symbol is referenced even if the input wasn't used at all in any of the asm()'s operands (but of course it would be better to use the operand once you have it, if it can be used in some way despite %c not being possible to use). Jan
On 14.12.20 10:22, Jan Beulich wrote: > On 14.12.2020 10:15, Jürgen Groß wrote: >> On 14.12.20 10:03, Jan Beulich wrote: >>> On 14.12.2020 08: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. >>> >>> Could you extend on this? I don't understand what the relevant >>> difference is, from just looking at the changes. >> >> The problem seems to be that a static symbol referenced from the inline >> asm seems not to silence the error that this static symbol isn't being >> used. On x86 the bug_frame is constructed using the %c modifier, which >> is not supported for Arm (at least gcc 7 used in my compile test >> complained), but seems to be enough for gcc on x86 to not complain. > > But this isn't tied to %c not working on older gcc, is it? It looks > instead to be tied to you not specifying the function pointer as an > input in the asm(). The compiler would know the symbol is referenced > even if the input wasn't used at all in any of the asm()'s operands > (but of course it would be better to use the operand once you have > it, if it can be used in some way despite %c not being possible to > use). Let me check that. Juergen
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. > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > V4: > - new patch > > I have verified the created bugframe is correct by inspecting the > created binary. > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > 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? > > -#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. > */ > -#define BUG_FRAME(type, line, file, has_msg, msg) do { \ > +#define BUG_FRAME(type, line, ptr, ptr_is_file, has_msg, msg) do { \ > BUILD_BUG_ON((line) >> 16); \ > BUILD_BUG_ON((type) >= BUGFRAME_NR); \ > asm ("1:"BUG_INSTR"\n" \ > ".pushsection .rodata.str, \"aMS\", %progbits, 1\n" \ > - "2:\t.asciz " __stringify(file) "\n" \ > + "2:\n" \ > + ".if " #ptr_is_file "\n" \ > + "\t.asciz " __stringify(ptr) "\n" \ > + ".endif\n" \ > "3:\n" \ > ".if " #has_msg "\n" \ > "\t.asciz " #msg "\n" \ > @@ -52,21 +56,27 @@ struct bug_frame { > "4:\n" \ > ".p2align 2\n" \ > ".long (1b - 4b)\n" \ > + ".if " #ptr_is_file "\n" \ > ".long (2b - 4b)\n" \ > + ".else\n" \ > + ".long (" #ptr " - 4b)\n" \ > + ".endif\n" \ > ".long (3b - 4b)\n" \ > ".hword " __stringify(line) ", 0\n" \ > ".popsection"); \ > } while (0) > > -#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, "") > +#define run_in_exception_handler(fn) BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, 0, "") > + > +#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 1, 0, "") > > #define BUG() do { \ > - BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, ""); \ > + BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 1, 0, ""); \ > unreachable(); \ > } while (0) > > #define assert_failed(msg) do { \ > - BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \ > + BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, 1, msg); \ > unreachable(); \ > } while (0) > > Cheers,
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? 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) #define run_in_exception_handler(fn) BUG_FRAME_run_fn(fn) > >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> V4: >> - new patch >> >> I have verified the created bugframe is correct by inspecting the >> created binary. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> 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. Juergen
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? > > #define run_in_exception_handler(fn) BUG_FRAME_run_fn(fn) > >> >>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> --- >>> V4: >>> - new patch >>> >>> I have verified the created bugframe is correct by inspecting the >>> created binary. >>> >>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> --- >>> 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. Cheers,
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@suse.com> >>>> --- >>>> V4: >>>> - new patch >>>> >>>> I have verified the created bugframe is correct by inspecting the >>>> created binary. >>>> >>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>> --- >>>> 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
Hi Juergen, On 14/12/2020 11:21, Jürgen Groß wrote: > 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(). Could you share the code and the error message? > > I might be missing something, but this was the fastest way to at least > confirm the scheme is working for Arm. Make senses. I also don't have much time to invest in trying to have a common macro. So I am happy with a new macro. Cheers,
On 14.12.20 12:21, Jürgen Groß wrote: > 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. Okay, I think I have found a way to use a common macro, which seems to be even more simple than the original one: #define BUG_FRAME(type, line, ptr, msg) do { \ BUILD_BUG_ON((line) >> 16); \ BUILD_BUG_ON((type) >= BUGFRAME_NR); \ asm ("1:"BUG_INSTR"\n" \ ".pushsection .bug_frames." __stringify(type) \ ", \"a\", %%progbits\n" \ "2:\n" \ ".p2align 2\n" \ ".long (1b - 2b)\n" \ ".long (%0 - 2b)\n" \ ".long (%1 - 2b)\n" \ ".hword " __stringify(line) ", 0\n" \ ".popsection" :: "i" (ptr), "i" (msg)); i \ } while (0) Juergen
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 -#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. */ -#define BUG_FRAME(type, line, file, has_msg, msg) do { \ +#define BUG_FRAME(type, line, ptr, ptr_is_file, has_msg, msg) do { \ BUILD_BUG_ON((line) >> 16); \ BUILD_BUG_ON((type) >= BUGFRAME_NR); \ asm ("1:"BUG_INSTR"\n" \ ".pushsection .rodata.str, \"aMS\", %progbits, 1\n" \ - "2:\t.asciz " __stringify(file) "\n" \ + "2:\n" \ + ".if " #ptr_is_file "\n" \ + "\t.asciz " __stringify(ptr) "\n" \ + ".endif\n" \ "3:\n" \ ".if " #has_msg "\n" \ "\t.asciz " #msg "\n" \ @@ -52,21 +56,27 @@ struct bug_frame { "4:\n" \ ".p2align 2\n" \ ".long (1b - 4b)\n" \ + ".if " #ptr_is_file "\n" \ ".long (2b - 4b)\n" \ + ".else\n" \ + ".long (" #ptr " - 4b)\n" \ + ".endif\n" \ ".long (3b - 4b)\n" \ ".hword " __stringify(line) ", 0\n" \ ".popsection"); \ } while (0) -#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, "") +#define run_in_exception_handler(fn) BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, 0, "") + +#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 1, 0, "") #define BUG() do { \ - BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, ""); \ + BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 1, 0, ""); \ unreachable(); \ } while (0) #define assert_failed(msg) do { \ - BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \ + BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, 1, msg); \ unreachable(); \ } while (0)