From: Oleksii <oleksii.kurochko@gmail.com>
To: Julien Grall <julien@xen.org>, 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: Mon, 03 Apr 2023 21:40:21 +0300 [thread overview]
Message-ID: <605245331bb93b7e60a4a9d65b19b6642d897034.camel@gmail.com> (raw)
In-Reply-To: <3a94ad32-159d-d06f-cba6-3bb40f9f2085@xen.org>
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.
So I'll update both the commit message and comment.
>
> 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
>
~ Oleksii
next prev parent reply other threads:[~2023-04-03 18:40 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
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 [this message]
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=605245331bb93b7e60a4a9d65b19b6642d897034.camel@gmail.com \
--to=oleksii.kurochko@gmail.com \
--cc=Volodymyr_Babchuk@epam.com \
--cc=andrew.cooper3@citrix.com \
--cc=bertrand.marquis@arm.com \
--cc=gianluca@rivosinc.com \
--cc=jbeulich@suse.com \
--cc=julien@xen.org \
--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.