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

[Julien Grall <julien@xxxxxxx>]

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?

--
Julien Grall