From: Julien Grall <julien@xen.org>
To: Oleksii Kurochko <oleksii.kurochko@gmail.com>,
xen-devel@lists.xenproject.org
Cc: Jan Beulich <jbeulich@suse.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
Stefano Stabellini <sstabellini@kernel.org>,
Gianluca Guida <gianluca@rivosinc.com>,
Bertrand Marquis <bertrand.marquis@arm.com>,
Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
Date: Fri, 31 Mar 2023 22:05:12 +0100 [thread overview]
Message-ID: <3a94ad32-159d-d06f-cba6-3bb40f9f2085@xen.org> (raw)
In-Reply-To: <8fdb98350ae4fc6029738d0aabe13a57e1945a50.1680086655.git.oleksii.kurochko@gmail.com>
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).
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?
Cheers,
[1] aarch64-linux-gnu-gcc (Linaro GCC 7.5-2019.12) 7.5.0
[2] arm-none-linux-gnueabihf-gcc (GNU Toolchain for the A-profile
Architecture 10.3-2021.07 (arm-10.29)) 10.3.1 20210621
--
Julien Grall
next prev parent reply other threads:[~2023-03-31 21:05 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-29 10:50 [PATCH v9 0/5] introduce generic implementation of macros from bug.h Oleksii Kurochko
2023-03-29 10:50 ` [PATCH v9 1/5] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
2023-03-29 11:42 ` Jan Beulich
2023-03-31 20:32 ` Julien Grall
2023-03-29 10:50 ` [PATCH v9 2/5] xen/arm: remove unused defines in <asm/bug.h> Oleksii Kurochko
2023-03-31 20:32 ` Julien Grall
2023-03-29 10:50 ` [PATCH v9 3/5] xen: change <asm/bug.h> to <xen/bug.h> Oleksii Kurochko
2023-03-31 20:36 ` Julien Grall
2023-03-29 10:50 ` [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h Oleksii Kurochko
2023-03-31 21:05 ` Julien Grall [this message]
2023-04-02 23:15 ` Stefano Stabellini
2023-04-05 16:34 ` Julien Grall
2023-04-06 21:03 ` Stefano Stabellini
2023-04-03 18:40 ` Oleksii
2023-04-04 6:41 ` Jan Beulich
2023-04-04 8:09 ` Oleksii
2023-04-05 16:39 ` Julien Grall
2023-04-06 6:35 ` Jan Beulich
2023-04-06 8:44 ` Julien Grall
2023-03-29 10:50 ` [PATCH v9 5/5] xen/x86: switch x86 to use generic implemetation " Oleksii Kurochko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3a94ad32-159d-d06f-cba6-3bb40f9f2085@xen.org \
--to=julien@xen.org \
--cc=Volodymyr_Babchuk@epam.com \
--cc=andrew.cooper3@citrix.com \
--cc=bertrand.marquis@arm.com \
--cc=gianluca@rivosinc.com \
--cc=jbeulich@suse.com \
--cc=oleksii.kurochko@gmail.com \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.