[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
Hi,
On 04/04/2023 09:09, Oleksii wrote:
On Tue, 2023-04-04 at 08:41 +0200, Jan Beulich wrote:
On 03.04.2023 20:40, Oleksii wrote:
Hello Julien,
On Fri, 2023-03-31 at 22:05 +0100, Julien Grall wrote:
Hi Oleksii,
I was going to ack the patch but then I spotted something that
would
want some clarification.
On 29/03/2023 11:50, Oleksii Kurochko wrote:
diff --git a/xen/arch/arm/include/asm/bug.h
b/xen/arch/arm/include/asm/bug.h
index cacaf014ab..3fb0471a9b 100644
--- a/xen/arch/arm/include/asm/bug.h
+++ b/xen/arch/arm/include/asm/bug.h
@@ -1,6 +1,24 @@
#ifndef __ARM_BUG_H__
#define __ARM_BUG_H__
+/*
+ * Please do not include in the header any header that might
+ * use BUG/ASSERT/etc maros asthey will be defined later after
+ * the return to <xen/bug.h> from the current header:
+ *
+ * <xen/bug.h>:
+ * ...
+ * <asm/bug.h>:
+ * ...
+ * <any_header_which_uses_BUG/ASSERT/etc macros.h>
+ * ...
+ * ...
+ * #define BUG() ...
+ * ...
+ * #define ASSERT() ...
+ * ...
+ */
+
#include <xen/types.h>
#if defined(CONFIG_ARM_32)
@@ -11,76 +29,7 @@
# error "unknown ARM variant"
#endif
-#define BUG_FRAME_STRUCT
-
-struct bug_frame {
- signed int loc_disp; /* Relative address to the bug
address
*/
- signed int file_disp; /* Relative address to the
filename */
- 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_line(b) ((b)->line)
-#define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
-
-/* 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. BUGFRAME_run_fn needs to be handled separately.
- */
Given this comment ...
-#define BUG_FRAME(type, line, 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" \
-
"3:\n"
\
- ".if " #has_msg
"\n" \
- "\t.asciz " #msg
"\n" \
-
".endif\n"
\
-
".popsection\n"
\
- ".pushsection .bug_frames." __stringify(type) ",
\"a\",
%progbits\n"\
-
"4:\n"
\
- ".p2align
2\n" \
- ".long (1b -
4b)\n" \
- ".long (2b -
4b)\n" \
- ".long (3b -
4b)\n" \
- ".hword " __stringify(line) ",
0\n" \
-
".popsection");
\
-} while (0)
-
-/*
- * GCC will not allow to use "i" when PIE is enabled (Xen
doesn't
set the
- * flag but instead rely on the default value from the
compiler).
So the
- * easiest way to implement run_in_exception_handler() is to
pass
the to
- * be called function in a fixed register.
- */
-#define run_in_exception_handler(fn) do
{ \
- asm ("mov " __stringify(BUG_FN_REG) ",
%0\n" \
-
"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, 0,
0\n" \
- ".popsection" :: "r" (fn) : __stringify(BUG_FN_REG)
); \
-} while (0)
-
-#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0,
"")
-
-#define BUG() do
{ \
- BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0,
""); \
-
unreachable(); \
-} while (0)
-
-#define assert_failed(msg) do
{ \
- BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1,
msg); \
-
unreachable(); \
-} while (0)
+#define BUG_ASM_CONST "c"
... you should explain in the commit message why this is needed
and
the
problem described above is not a problem anymore.
For instance, I managed to build it without 'c' on arm64 [1]. But
it
does break on arm32 [2]. I know that Arm is also where '%c' was
an
issue.
Skimming through linux, the reason seems to be that GCC may add
'#'
when
it should not. That said, I haven't look at the impact on the
generic
implementation. Neither I looked at which version may be affected
(the
original message was from 2011).
You are right that some compilers add '#' when it shouldn't. The
same
thing happens with RISC-V.
I am a bit confused with this answer. My point was that on at least on
Arm 32-bit we need to use 'c' otherwise it breaks.
AFAIU, this is not the same problem on RISC-V...
RISC-V doesn't know of a '#' prefix, does it? '#' is a comment
character
there afaik, like for many other architectures.
It doesn't and for RISC-V it's a comment character.
afaik '%c' is needed to skip prefix('sign' ) (# or $ ( in case of
x86)).
I mean that RISC-V doesn't put anything before immediate so there is no
need to use %c as we don't need to skip prefix/'sign' before immediate
but if start to use '%c' it will cause an compiler issue.
... because here you say it will break when using 'c'. Did I miss anything?
Anyway, it sounds like to me that the default definition in xen/bug.h
should be using 'c' rather than been empty because this seems to be the
more common approach.
To reduce the amount of patch to resend, I was actually thinking to
merge patch #1-3 and #5 (so leave this patch alone) and modify the
default in a follow-up. Any thoughts?
Cheers,
--
Julien Grall
|
