| Message ID | 20201215063319.23290-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 15.12.2020 07:33, Juergen Gross wrote: > --- a/xen/include/asm-arm/bug.h > +++ b/xen/include/asm-arm/bug.h > @@ -15,65 +15,62 @@ > > 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, 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" \ > - "3:\n" \ > - ".if " #has_msg "\n" \ > - "\t.asciz " #msg "\n" \ > - ".endif\n" \ > - ".popsection\n" \ > - ".pushsection .bug_frames." __stringify(type) ", \"a\", %progbits\n"\ > - "4:\n" \ > + ".pushsection .bug_frames." __stringify(type) ", \"a\", %%progbits\n"\ > + "2:\n" \ > ".p2align 2\n" \ > - ".long (1b - 4b)\n" \ > - ".long (2b - 4b)\n" \ > - ".long (3b - 4b)\n" \ > + ".long (1b - 2b)\n" \ > + ".long (%0 - 2b)\n" \ > + ".long (%1 - 2b)\n" \ > ".hword " __stringify(line) ", 0\n" \ > - ".popsection"); \ > + ".popsection" :: "i" (ptr), "i" (msg)); \ > } while (0) The comment ahead of the construct now looks to be at best stale, if not entirely pointless. The reference to %c looks quite strange here to me anyway - I can only guess it appeared here because on x86 one has to use %c to output constants as operands for .long and alike, and this was then tried to use on Arm as well without there really being a need. Jan
On 15.12.20 10:02, Jan Beulich wrote: > On 15.12.2020 07:33, Juergen Gross wrote: >> --- a/xen/include/asm-arm/bug.h >> +++ b/xen/include/asm-arm/bug.h >> @@ -15,65 +15,62 @@ >> >> 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, 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" \ >> - "3:\n" \ >> - ".if " #has_msg "\n" \ >> - "\t.asciz " #msg "\n" \ >> - ".endif\n" \ >> - ".popsection\n" \ >> - ".pushsection .bug_frames." __stringify(type) ", \"a\", %progbits\n"\ >> - "4:\n" \ >> + ".pushsection .bug_frames." __stringify(type) ", \"a\", %%progbits\n"\ >> + "2:\n" \ >> ".p2align 2\n" \ >> - ".long (1b - 4b)\n" \ >> - ".long (2b - 4b)\n" \ >> - ".long (3b - 4b)\n" \ >> + ".long (1b - 2b)\n" \ >> + ".long (%0 - 2b)\n" \ >> + ".long (%1 - 2b)\n" \ >> ".hword " __stringify(line) ", 0\n" \ >> - ".popsection"); \ >> + ".popsection" :: "i" (ptr), "i" (msg)); \ >> } while (0) > > The comment ahead of the construct now looks to be at best stale, if > not entirely pointless. The reference to %c looks quite strange here > to me anyway - I can only guess it appeared here because on x86 one > has to use %c to output constants as operands for .long and alike, > and this was then tried to use on Arm as well without there really > being a need. Probably so. I can remove the comment, but would like an Arm maintainer to confirm. Juergen
Hi Jan, On 15/12/2020 09:02, Jan Beulich wrote: > On 15.12.2020 07:33, Juergen Gross wrote: >> --- a/xen/include/asm-arm/bug.h >> +++ b/xen/include/asm-arm/bug.h >> @@ -15,65 +15,62 @@ >> >> 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, 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" \ >> - "3:\n" \ >> - ".if " #has_msg "\n" \ >> - "\t.asciz " #msg "\n" \ >> - ".endif\n" \ >> - ".popsection\n" \ >> - ".pushsection .bug_frames." __stringify(type) ", \"a\", %progbits\n"\ >> - "4:\n" \ >> + ".pushsection .bug_frames." __stringify(type) ", \"a\", %%progbits\n"\ >> + "2:\n" \ >> ".p2align 2\n" \ >> - ".long (1b - 4b)\n" \ >> - ".long (2b - 4b)\n" \ >> - ".long (3b - 4b)\n" \ >> + ".long (1b - 2b)\n" \ >> + ".long (%0 - 2b)\n" \ >> + ".long (%1 - 2b)\n" \ >> ".hword " __stringify(line) ", 0\n" \ >> - ".popsection"); \ >> + ".popsection" :: "i" (ptr), "i" (msg)); \ >> } while (0) > > The comment ahead of the construct now looks to be at best stale, if > not entirely pointless. The reference to %c looks quite strange here > to me anyway - I can only guess it appeared here because on x86 one > has to use %c to output constants as operands for .long and alike, > and this was then tried to use on Arm as well without there really > being a need. Well, %c is one the reason why we can't have a common BUG_FRAME implementation. So I would like to retain this information before someone wants to consolidate the code and missed this issue. Regarding the mergeable string section, I agree that the comment in now stale. However, could someone confirm that "i" will still retain the same behavior as using mergeable string sections? Cheers,
On 15.12.2020 14:39, Julien Grall wrote: > On 15/12/2020 09:02, Jan Beulich wrote: >> On 15.12.2020 07:33, Juergen Gross wrote: >>> --- a/xen/include/asm-arm/bug.h >>> +++ b/xen/include/asm-arm/bug.h >>> @@ -15,65 +15,62 @@ >>> >>> 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, 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" \ >>> - "3:\n" \ >>> - ".if " #has_msg "\n" \ >>> - "\t.asciz " #msg "\n" \ >>> - ".endif\n" \ >>> - ".popsection\n" \ >>> - ".pushsection .bug_frames." __stringify(type) ", \"a\", %progbits\n"\ >>> - "4:\n" \ >>> + ".pushsection .bug_frames." __stringify(type) ", \"a\", %%progbits\n"\ >>> + "2:\n" \ >>> ".p2align 2\n" \ >>> - ".long (1b - 4b)\n" \ >>> - ".long (2b - 4b)\n" \ >>> - ".long (3b - 4b)\n" \ >>> + ".long (1b - 2b)\n" \ >>> + ".long (%0 - 2b)\n" \ >>> + ".long (%1 - 2b)\n" \ >>> ".hword " __stringify(line) ", 0\n" \ >>> - ".popsection"); \ >>> + ".popsection" :: "i" (ptr), "i" (msg)); \ >>> } while (0) >> >> The comment ahead of the construct now looks to be at best stale, if >> not entirely pointless. The reference to %c looks quite strange here >> to me anyway - I can only guess it appeared here because on x86 one >> has to use %c to output constants as operands for .long and alike, >> and this was then tried to use on Arm as well without there really >> being a need. > > Well, %c is one the reason why we can't have a common BUG_FRAME > implementation. So I would like to retain this information before > someone wants to consolidate the code and missed this issue. Fair enough, albeit I guess this then could do with re-wording. > Regarding the mergeable string section, I agree that the comment in now > stale. However, could someone confirm that "i" will still retain the > same behavior as using mergeable string sections? That's depend on compiler settings / behavior. Jan
Hi Jan, On 15/12/2020 13:59, Jan Beulich wrote: > On 15.12.2020 14:39, Julien Grall wrote: >> On 15/12/2020 09:02, Jan Beulich wrote: >>> On 15.12.2020 07:33, Juergen Gross wrote: >>>> --- a/xen/include/asm-arm/bug.h >>>> +++ b/xen/include/asm-arm/bug.h >>>> @@ -15,65 +15,62 @@ >>>> >>>> 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, 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" \ >>>> - "3:\n" \ >>>> - ".if " #has_msg "\n" \ >>>> - "\t.asciz " #msg "\n" \ >>>> - ".endif\n" \ >>>> - ".popsection\n" \ >>>> - ".pushsection .bug_frames." __stringify(type) ", \"a\", %progbits\n"\ >>>> - "4:\n" \ >>>> + ".pushsection .bug_frames." __stringify(type) ", \"a\", %%progbits\n"\ >>>> + "2:\n" \ >>>> ".p2align 2\n" \ >>>> - ".long (1b - 4b)\n" \ >>>> - ".long (2b - 4b)\n" \ >>>> - ".long (3b - 4b)\n" \ >>>> + ".long (1b - 2b)\n" \ >>>> + ".long (%0 - 2b)\n" \ >>>> + ".long (%1 - 2b)\n" \ >>>> ".hword " __stringify(line) ", 0\n" \ >>>> - ".popsection"); \ >>>> + ".popsection" :: "i" (ptr), "i" (msg)); \ >>>> } while (0) >>> >>> The comment ahead of the construct now looks to be at best stale, if >>> not entirely pointless. The reference to %c looks quite strange here >>> to me anyway - I can only guess it appeared here because on x86 one >>> has to use %c to output constants as operands for .long and alike, >>> and this was then tried to use on Arm as well without there really >>> being a need. >> >> Well, %c is one the reason why we can't have a common BUG_FRAME >> implementation. So I would like to retain this information before >> someone wants to consolidate the code and missed this issue. > > Fair enough, albeit I guess this then could do with re-wording. I agree. > >> Regarding the mergeable string section, I agree that the comment in now >> stale. However, could someone confirm that "i" will still retain the >> same behavior as using mergeable string sections? > > That's depend on compiler settings / behavior. Ok. I wanted to see the difference between before and after but it looks like I can't compile Xen after applying the patch: In file included from /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/xen/lib.h:23:0, from /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/xen/bitmap.h:6, from bitmap.c:10: bitmap.c: In function ‘bitmap_allocate_region’: /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:44:5: error: asm operand 0 probably doesn’t match constraints [-Werror] asm ("1:"BUG_INSTR"\n" \ ^ /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:60:5: note: in expansion of macro ‘BUG_FRAME’ BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, ""); \ ^~~~~~~~~ /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/xen/lib.h:25:42: note: in expansion of macro ‘BUG’ #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0) ^~~ bitmap.c:330:2: note: in expansion of macro ‘BUG_ON’ BUG_ON(pages > BITS_PER_LONG); ^~~~~~ /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:44:5: error: asm operand 1 probably doesn’t match constraints [-Werror] asm ("1:"BUG_INSTR"\n" \ ^ /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:60:5: note: in expansion of macro ‘BUG_FRAME’ BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, ""); \ ^~~~~~~~~ /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/xen/lib.h:25:42: note: in expansion of macro ‘BUG’ #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0) ^~~ bitmap.c:330:2: note: in expansion of macro ‘BUG_ON’ BUG_ON(pages > BITS_PER_LONG); ^~~~~~ /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:44:5: error: impossible constraint in ‘asm’ asm ("1:"BUG_INSTR"\n" \ ^ /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:60:5: note: in expansion of macro ‘BUG_FRAME’ BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, ""); \ ^~~~~~~~~ /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/xen/lib.h:25:42: note: in expansion of macro ‘BUG’ #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0) ^~~ bitmap.c:330:2: note: in expansion of macro ‘BUG_ON’ BUG_ON(pages > BITS_PER_LONG); ^~~~~~ cc1: all warnings being treated as errors I am using GCC 7.5.0 built by Linaro (cross-compiler). Native compilation with GCC 10.2.1 leads to the same error. @Juergen, which compiler did you use?
On 21.12.20 17:50, Julien Grall wrote: > Hi Jan, > > On 15/12/2020 13:59, Jan Beulich wrote: >> On 15.12.2020 14:39, Julien Grall wrote: >>> On 15/12/2020 09:02, Jan Beulich wrote: >>>> On 15.12.2020 07:33, Juergen Gross wrote: >>>>> --- a/xen/include/asm-arm/bug.h >>>>> +++ b/xen/include/asm-arm/bug.h >>>>> @@ -15,65 +15,62 @@ >>>>> 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, 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" \ >>>>> - >>>>> "3:\n" \ >>>>> - ".if " #has_msg >>>>> "\n" \ >>>>> - "\t.asciz " #msg >>>>> "\n" \ >>>>> - >>>>> ".endif\n" \ >>>>> - >>>>> ".popsection\n" \ >>>>> - ".pushsection .bug_frames." __stringify(type) ", \"a\", >>>>> %progbits\n"\ >>>>> - >>>>> "4:\n" \ >>>>> + ".pushsection .bug_frames." __stringify(type) ", \"a\", >>>>> %%progbits\n"\ >>>>> + >>>>> "2:\n" \ >>>>> ".p2align >>>>> 2\n" \ >>>>> - ".long (1b - >>>>> 4b)\n" \ >>>>> - ".long (2b - >>>>> 4b)\n" \ >>>>> - ".long (3b - >>>>> 4b)\n" \ >>>>> + ".long (1b - >>>>> 2b)\n" \ >>>>> + ".long (%0 - >>>>> 2b)\n" \ >>>>> + ".long (%1 - >>>>> 2b)\n" \ >>>>> ".hword " __stringify(line) ", >>>>> 0\n" \ >>>>> - >>>>> ".popsection"); \ >>>>> + ".popsection" :: "i" (ptr), "i" >>>>> (msg)); \ >>>>> } while (0) >>>> >>>> The comment ahead of the construct now looks to be at best stale, if >>>> not entirely pointless. The reference to %c looks quite strange here >>>> to me anyway - I can only guess it appeared here because on x86 one >>>> has to use %c to output constants as operands for .long and alike, >>>> and this was then tried to use on Arm as well without there really >>>> being a need. >>> >>> Well, %c is one the reason why we can't have a common BUG_FRAME >>> implementation. So I would like to retain this information before >>> someone wants to consolidate the code and missed this issue. >> >> Fair enough, albeit I guess this then could do with re-wording. > > I agree. > >> >>> Regarding the mergeable string section, I agree that the comment in now >>> stale. However, could someone confirm that "i" will still retain the >>> same behavior as using mergeable string sections? >> >> That's depend on compiler settings / behavior. > > Ok. I wanted to see the difference between before and after but it looks > like I can't compile Xen after applying the patch: > > > In file included from > /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/xen/lib.h:23:0, > from > /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/xen/bitmap.h:6, > from bitmap.c:10: > bitmap.c: In function ‘bitmap_allocate_region’: > /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:44:5: > error: asm operand 0 probably doesn’t match constraints [-Werror] > asm ("1:"BUG_INSTR"\n" \ > ^ > /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:60:5: > note: in expansion of macro ‘BUG_FRAME’ > BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, ""); \ > ^~~~~~~~~ > /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/xen/lib.h:25:42: > note: in expansion of macro ‘BUG’ > #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0) > ^~~ > bitmap.c:330:2: note: in expansion of macro ‘BUG_ON’ > BUG_ON(pages > BITS_PER_LONG); > ^~~~~~ > /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:44:5: > error: asm operand 1 probably doesn’t match constraints [-Werror] > asm ("1:"BUG_INSTR"\n" \ > ^ > /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:60:5: > note: in expansion of macro ‘BUG_FRAME’ > BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, ""); \ > ^~~~~~~~~ > /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/xen/lib.h:25:42: > note: in expansion of macro ‘BUG’ > #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0) > ^~~ > bitmap.c:330:2: note: in expansion of macro ‘BUG_ON’ > BUG_ON(pages > BITS_PER_LONG); > ^~~~~~ > /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:44:5: > error: impossible constraint in ‘asm’ > asm ("1:"BUG_INSTR"\n" \ > ^ > /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/asm/bug.h:60:5: > note: in expansion of macro ‘BUG_FRAME’ > BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, ""); \ > ^~~~~~~~~ > /home/ANT.AMAZON.COM/jgrall/works/oss/xen/xen/include/xen/lib.h:25:42: > note: in expansion of macro ‘BUG’ > #define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0) > ^~~ > bitmap.c:330:2: note: in expansion of macro ‘BUG_ON’ > BUG_ON(pages > BITS_PER_LONG); > ^~~~~~ > cc1: all warnings being treated as errors > > I am using GCC 7.5.0 built by Linaro (cross-compiler). Native > compilation with GCC 10.2.1 leads to the same error. > > @Juergen, which compiler did you use? > gcc 7.4.0 aarch64 cross-compiler (SUSE) Juergen
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 22bd1bd4c6..912b9a3d77 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/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/bug.h b/xen/include/asm-arm/bug.h index 36c803357c..4610835ac3 100644 --- a/xen/include/asm-arm/bug.h +++ b/xen/include/asm-arm/bug.h @@ -15,65 +15,62 @@ 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, 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" \ - "3:\n" \ - ".if " #has_msg "\n" \ - "\t.asciz " #msg "\n" \ - ".endif\n" \ - ".popsection\n" \ - ".pushsection .bug_frames." __stringify(type) ", \"a\", %progbits\n"\ - "4:\n" \ + ".pushsection .bug_frames." __stringify(type) ", \"a\", %%progbits\n"\ + "2:\n" \ ".p2align 2\n" \ - ".long (1b - 4b)\n" \ - ".long (2b - 4b)\n" \ - ".long (3b - 4b)\n" \ + ".long (1b - 2b)\n" \ + ".long (%0 - 2b)\n" \ + ".long (%1 - 2b)\n" \ ".hword " __stringify(line) ", 0\n" \ - ".popsection"); \ + ".popsection" :: "i" (ptr), "i" (msg)); \ } while (0) -#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, "") +#define run_in_exception_handler(fn) BUG_FRAME(BUGFRAME_run_fn, 0, fn, "") + +#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, "") #define BUG() do { \ - BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0, ""); \ + BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, ""); \ unreachable(); \ } while (0) #define assert_failed(msg) do { \ - BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg); \ + BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, msg); \ unreachable(); \ } while (0) 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__ */ /*