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

[Julien Grall <julien@xxxxxxx>]

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,

--
Julien Grall