Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
On Wed, 5 Apr 2023, Julien Grall wrote:
> On 03/04/2023 00:15, Stefano Stabellini wrote:
> > On Fri, 31 Mar 2023, 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 from the current header:
> > > > + *
> > > > + * :
> > > > + * ...
> > > > + * :
> > > > + * ...
> > > > + *
> > > > + * ...
> > > > + * ...
> > > > + * #define BUG() ...
> > > > + * ...
> > > > + * #define ASSERT() ...
> > > > + * ...
> > > > + */
> > > > +
> > > >#include
> > > > #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"
> > >
> > > ..
Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
Hi, On 06/04/2023 07:35, Jan Beulich wrote: On 05.04.2023 18:39, Julien Grall wrote: 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? Well, yes, that's what I did a couple of days ago already. Ah. I didn't check the tree when replying. So ignore me. Cheers, -- Julien Grall
Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
On 05.04.2023 18:39, Julien Grall wrote: > 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? Well, yes, that's what I did a couple of days ago already. Jan
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 from the current header:
+ *
+ * :
+ * ...
+ * :
+ * ...
+ *
+ * ...
+ * ...
+ * #define BUG() ...
+ * ...
+ * #define ASSERT() ...
+ * ...
+ */
+
#include
#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
Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
Hi,
On 03/04/2023 00:15, Stefano Stabellini wrote:
On Fri, 31 Mar 2023, 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 from the current header:
+ *
+ * :
+ * ...
+ * :
+ * ...
+ *
+ * ...
+ * ...
+ * #define BUG() ...
+ * ...
+ * #define ASSERT() ...
+ * ...
+ */
+
#include
#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).
However, without an explanation, I am afraid this can't go in because I am
worry we may break some users (thankfully that might just be a compilation
issues rather than weird behavior).
Bertrand, Stefano, do you know if this is still an issue?
I don't know, but I confirm your observation.
In my system, both ARM64 and ARM32 compile without problems with "c".
Without "c', only ARM64 compiles without problems, while ARM32 breaks.
My ARM32 compiler is:
arm-linux-gnueabihf-gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
Additing a meaningful explanation to the commit message might be
difficult in this case.
Agree. One would need to look at the compiler code to confirm. We sho
Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
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 from the current header:
> > > > + *
> > > > + * :
> > > > + * ...
> > > > + * :
> > > > + * ...
> > > > + *
> > > > + * ...
> > > > + * ...
> > > > + * #define BUG() ...
> > > > + * ...
> > > > + * #define ASSERT() ...
> > > > + * ...
> > > > + */
> > > > +
> > > > #include
> > > >
> > > > #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" \
> > > > -
> > >
Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
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 from the current header:
>>> + *
>>> + * :
>>> + * ...
>>> + * :
>>> + * ...
>>> + *
>>> + * ...
>>> + * ...
>>> + * #define BUG() ...
>>> + * ...
>>> + * #define ASSERT() ...
>>> + * ...
>>> + */
>>> +
>>> #include
>>>
>>> #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)
>
Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
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 from the current header:
> > + *
> > + * :
> > + * ...
> > + * :
> > + * ...
> > + *
> > + * ...
> > + * ...
> > + * #define BUG() ...
> > + * ...
> > + * #define ASSERT() ...
> > + * ...
> > + */
> > +
> > #include
> >
> > #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_w
Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
On Fri, 31 Mar 2023, 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 from the current header:
> > + *
> > + * :
> > + * ...
> > + * :
> > + * ...
> > + *
> > + * ...
> > + * ...
> > + * #define BUG() ...
> > + * ...
> > + * #define ASSERT() ...
> > + * ...
> > + */
> > +
> > #include
> > #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).
>
> However, without an explanation, I am afraid this can't go in because I am
> worry we may break some users (thankfully that might just be a compilation
> issues rather than weird b
Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
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 from the current header:
+ *
+ * :
+ * ...
+ * :
+ * ...
+ *
+ * ...
+ * ...
+ * #define BUG() ...
+ * ...
+ * #define ASSERT() ...
+ * ...
+ */
+
#include
#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 instanc
[PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
The following changes were made:
* make GENERIC_BUG_FRAME mandatory for ARM
* As do_bug_frame() returns -EINVAL in case something goes wrong
otherwise id of bug frame. Thereby 'if' cases where do_bug_frame() was
updated to check if the returned value is less than 0
* Switch ARM's implementation of bug.h macros to generic one
Signed-off-by: Oleksii Kurochko
---
Changes in V9:
* add additional explanation to header
---
Changes in V8:
* Nothing changed.
---
Changes in V7:
* Rebase the patch.
---
Changes in V6:
* Update the "changes in v5"
* Rebase on top of the patch [xen: introduce CONFIG_GENERIC_BUG_FRAME] as
there were minor changes.
---
Changes in V5:
* common/bug.c changes were removed after rebase
(the patch [xen: introduce CONFIG_GENERIC_BUG_FRAME] was reworked to make
ARM implementation to use generic do_bug_frame())
---
Changes in V4:
* Switch ARM implementation to generic one
* Remove BUG_FN_REG from arm{16,32}/bug.h as it isn't needed after switch to
generic implementation
* Update commit message
---
Changes in V3:
* As prototype and what do_bug_frame() returns was changed so patch 3 and 4
was updated to use a new version of do_bug_frame
---
Changes in V2:
* Rename bug_file() in ARM implementation to bug_ptr() as
generic do_bug_frame() uses bug_ptr().
* Remove generic parts from bug.h
* Remove declaration of 'int do_bug_frame(...)'
from as it was introduced in
---
xen/arch/arm/Kconfig | 1 +
xen/arch/arm/arm32/traps.c | 2 +-
xen/arch/arm/include/asm/arm32/bug.h | 2 -
xen/arch/arm/include/asm/arm64/bug.h | 2 -
xen/arch/arm/include/asm/bug.h | 89 ++--
xen/arch/arm/include/asm/traps.h | 2 -
xen/arch/arm/traps.c | 81 +
7 files changed, 22 insertions(+), 157 deletions(-)
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 239d3aed3c..aad6644a7b 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -12,6 +12,7 @@ config ARM_64
config ARM
def_bool y
+ select GENERIC_BUG_FRAME
select HAS_ALTERNATIVE
select HAS_DEVICE_TREE
select HAS_PASSTHROUGH
diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c
index a2fc1c22cb..61c61132c7 100644
--- a/xen/arch/arm/arm32/traps.c
+++ b/xen/arch/arm/arm32/traps.c
@@ -48,7 +48,7 @@ void do_trap_undefined_instruction(struct cpu_user_regs *regs)
if ( instr != BUG_OPCODE )
goto die;
-if ( do_bug_frame(regs, pc) )
+if ( do_bug_frame(regs, pc) < 0 )
goto die;
regs->pc += 4;
diff --git a/xen/arch/arm/include/asm/arm32/bug.h
b/xen/arch/arm/include/asm/arm32/bug.h
index 25cce151dc..3e66f35969 100644
--- a/xen/arch/arm/include/asm/arm32/bug.h
+++ b/xen/arch/arm/include/asm/arm32/bug.h
@@ -10,6 +10,4 @@
#define BUG_INSTR ".word " __stringify(BUG_OPCODE)
-#define BUG_FN_REG r0
-
#endif /* __ARM_ARM32_BUG_H__ */
diff --git a/xen/arch/arm/include/asm/arm64/bug.h
b/xen/arch/arm/include/asm/arm64/bug.h
index 5e11c0dfd5..59f664d7de 100644
--- a/xen/arch/arm/include/asm/arm64/bug.h
+++ b/xen/arch/arm/include/asm/arm64/bug.h
@@ -6,6 +6,4 @@
#define BUG_INSTR "brk " __stringify(BRK_BUG_FRAME_IMM)
-#define BUG_FN_REG x0
-
#endif /* __ARM_ARM64_BUG_H__ */
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 from the current header:
+ *
+ * :
+ * ...
+ * :
+ * ...
+ *
+ * ...
+ * ...
+ * #define BUG() ...
+ * ...
+ * #define ASSERT() ...
+ * ...
+ */
+
#include
#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.
- */
-#define BUG_FRAME(type, line, file, has_msg, msg) do { \
-BUILD_BUG_ON((line) >> 16);
