[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.