From: Oleksii <oleksii.kurochko@gmail.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: 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>,
xen-devel@lists.xenproject.org, Julien Grall <julien@xen.org>
Subject: Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h
Date: Tue, 04 Apr 2023 11:09:49 +0300 [thread overview]
Message-ID: <d351a7b6d673b70d45e809123e6e42abbf7b8014.camel@gmail.com> (raw)
In-Reply-To: <9c4ca4a1-1b68-5ee0-0434-e6c9ec7d1ef6@suse.com>
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.
>
> 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.
~ Oleksii
next prev parent reply other threads:[~2023-04-04 8:10 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
2023-04-04 6:41 ` Jan Beulich
2023-04-04 8:09 ` Oleksii [this message]
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=d351a7b6d673b70d45e809123e6e42abbf7b8014.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.