From: Oleksii <oleksii.kurochko@gmail.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: George Dunlap <george.dunlap@citrix.com>,
Julien Grall <julien@xen.org>,
Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
xen-devel@lists.xenproject.org,
Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH v1 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
Date: Fri, 17 Feb 2023 11:33:13 +0200 [thread overview]
Message-ID: <75a018704e212c043370269d10e4b4a45146e9b3.camel@gmail.com> (raw)
In-Reply-To: <20191ac6-3c1c-8c9f-39ed-308bc22d5b3f@suse.com>
On Fri, 2023-02-17 at 08:12 +0100, Jan Beulich wrote:
> On 16.02.2023 21:44, Oleksii wrote:
> > On Thu, 2023-02-16 at 12:19 +0000, Andrew Cooper wrote:
> > > On 16/02/2023 12:09 pm, Oleksii wrote:
> > > > On Thu, 2023-02-16 at 12:44 +0200, Oleksii wrote:
> > > > > On Thu, 2023-02-16 at 08:31 +0100, Jan Beulich wrote:
> > > > > > On 15.02.2023 18:59, Oleksii wrote:
> > > > > > > Hello Jan and community,
> > > > > > >
> > > > > > > I experimented and switched RISC-V to x86 implementation.
> > > > > > > All
> > > > > > > that
> > > > > > > I
> > > > > > > changed in x86 implementation for RISC-V was
> > > > > > > _ASM_BUGFRAME_TEXT.
> > > > > > > Other
> > > > > > > things are the same as for x86.
> > > > > > >
> > > > > > > For RISC-V it is fine to skip '%c' modifier so
> > > > > > > _ASM_BUGFRAME_TEXT
> > > > > > > will
> > > > > > > look like:
> > > > > > >
> > > > > > > #define _ASM_BUGFRAME_TEXT(second_frame) \
> > > > > > > ".Lbug%=: ebreak\n"
> > > > > > > ".pushsection .bug_frames.%[bf_type], \"a\",
> > > > > > > @progbits\n"
> > > > > > > ".p2align 2\n"
> > > > > > > ".Lfrm%=:\n"
> > > > > > > ".long (.Lbug%= - .Lfrm%=) + %[bf_line_hi]\n"
> > > > > > > ".long (%[bf_ptr] - .Lfrm%=) + %[bf_line_lo]\n"
> > > > > > > ".if " #second_frame "\n"
> > > > > > > ".long 0, %[bf_msg] - .Lfrm%=\n"
> > > > > > > ".endif\n"
> > > > > > > ".popsection\n"
> > > > > > I expect this could be further abstracted such that only
> > > > > > the
> > > > > > actual
> > > > > > instruction is arch-specific.
> > > > > Generally, the only thing that can't be abstracted ( as you
> > > > > mentioned
> > > > > is an instruction ).
> > > > >
> > > > > So we can make additional defines:
> > > > > 1. #define MODIFIER "" or "c" (depends on architecture) and
> > > > > use
> > > > > it
> > > > >
> > > > > the following way:
> > > > > ".pushsection .bug_frames.%"MODIFIER"[bf_type], \"a\",
> > > > > @progbits\n"
> > > > > ...
> > > > > 2. #define BUG_INSTR "ebreak" | "ud2" | "..." and use it in
> > > > > the
> > > > > following way:
> > > > > ".Lbug%=: "BUG_ISNTR"\n"
> > > > > ...
> > > > > Except for these defines which should be specified for each
> > > > > architecture
> > > > > all other code will be the same for all architectures.
> > > > >
> > > > > But as I understand with modifier 'c' can be issues that not
> > > > > all
> > > > > compilers support but for ARM and x86 before immediate is
> > > > > present
> > > > > punctuation # or $ which are removed by modifier 'c'. And I
> > > > > am
> > > > > wondering what other ways to remove punctuation before
> > > > > immediate
> > > > > exist.
> > > > >
> > > > > Do you think we should consider that as an issue?
> > > > >
> > > > > > > The only thing I am worried about is:
> > > > > > >
> > > > > > > #define _ASM_BUGFRAME_INFO(type, line, ptr, msg) \
> > > > > > > [bf_type] "i" (type), ...
> > > > > > > because as I understand it can be an issue with 'i'
> > > > > > > modifier
> > > > > > > in
> > > > > > > case of
> > > > > > > PIE. I am not sure that Xen enables PIE somewhere but
> > > > > > > still...
> > > > > > > If it is not an issue then we can use x86 implementation
> > > > > > > as a
> > > > > > > generic
> > > > > > > one.
> > > > > > "i" is not generally an issue with PIE, it only is when the
> > > > > > value
> > > > > > is
> > > > > > the
> > > > > > address of a symbol. Here "type" is a constant in all
> > > > > > cases.
> > > > > > (Or
> > > > > > else
> > > > > > how would you express an immediate operand of an
> > > > > > instruction in
> > > > > > an
> > > > > > asm()?)
> > > > > Hmm. I don't know other ways to express an immediate operand
> > > > > except
> > > > > 'i'.
> > > > >
> > > > > It looks like type, line, msg are always 'constants'
> > > > > (
> > > > > they possibly can be passed without 'i' and used directly
> > > > > inside
> > > > > asm():
> > > > > ...
> > > > > ".pushsection .bug_frames." __stringify(type) ", \"a\",
> > > > > %progbits\n"\
> > > > > ...
> > > > > ) but the issue will be with 'ptr' for which we have to use
> > > > > 'i'.
> > > > >
> > > > > And I am not sure about all 'constants'. For example, I think
> > > > > that it
> > > > > can be an issue with 'line' = '((line & ((1 <<
> > > > > BUG_LINE_LO_WIDTH)
> > > > > -
> > > > > 1))
> > > > > << BUG_DISP_WIDTH)' if we will use that directly inside
> > > > > asm(...).
> > > > >
> > > > I think we can avoid 'i' by using 'r' + some kind of mov
> > > > instruction to
> > > > write a register value to memory. The code below is pseudo-
> > > > code:
> > > > #define _ASM_BUGFRAME_TEXT(second_frame)
> > > > ...
> > > > "loc_disp:\n"
> > > > " .long 0x0"
> > > > "mov [loc_disp], some_register"
> > > > ...
> > > > And the we have to define mov for each architecture.
> > >
> > > No, you really cannot. This is the asm equivalent of writing
> > > something
> > > like this:
> > >
> > > static struct bugframe __section(.bug_frames.1) foo = {
> > > .loc = insn - &foo + LINE_LO,
> > > .msg = "bug string" - &foo + LINE_HI,
> > > };
> > >
> > > It is a data structure, not executable code.
> > Thanks for the clarification.
> >
> > What about mainly using C for BUG_FRAME and only allocating
> > bug_frame
> > sections in assembly?
> >
> > Please look at POC below:
>
> That's still using statements (assignments), not initializers. We
> expect
> the table to be populated at build time, and we expect it to be read-
> only
> at runtime. Plus your approach once again increases overall size
> (just
> that this time you add code, not data).
If we have such requirements then the best option is to use as a
generic implementation the implementation provided by x86 ( as I
mentioned before, it mainly can be re-used for RISC-V ) and leave ARM
implementation mostly as is ( except for some minor changes ).
>
> Jan
>
> > #include <stdio.h>
> > #include <stdint.h>
> >
> > #define BUG_DISP_WIDTH 24
> > #define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> > #define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> >
> > struct bug_frame {
> > signed int loc_disp:BUG_DISP_WIDTH;
> > unsigned int line_hi:BUG_LINE_HI_WIDTH;
> > signed int ptr_disp:BUG_DISP_WIDTH;
> > unsigned int line_lo:BUG_LINE_LO_WIDTH;
> > signed int msg_disp[];
> > };
> >
> > #define bug_loc(b) ((const void *)(b) + (b)->loc_disp)
> > #define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp)
> > #define bug_line(b) (((((b)->line_hi + ((b)->loc_disp < 0)) & \
> > ((1 << BUG_LINE_HI_WIDTH) - 1)) << \
> > BUG_LINE_LO_WIDTH) + \
> > (((b)->line_lo + ((b)->ptr_disp < 0))
> > &
> > \
> > ((1 << BUG_LINE_LO_WIDTH) - 1)))
> > #define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1])
> >
> > #define ALLOCATE_BF_SECTION(has_msg) do
> > { \
> > asm (".pushsection bug_frames
> > \n"
> > \
> > ".long 0, 0
> > \n"
> > \
> > ".if " #has_msg
> > "\n"
> > \
> > "\t.long 0
> > \n"
> > \
> > "\t.long 0
> > \n"
> > \
> >
> > ".endif\n"
> > \
> > ".popsection"
> > );
> > \
> > } while (0)
> >
> > #define MERGE_(a,b) a##b
> > #define UNIQUE_BUG_INSTR_LABEL(a) MERGE_(unique_name_, a)
> >
> > #define BUG_FRAME(type, line, file, has_msg, msg) do
> > {
> > \
> > unsigned int line_lo = (((line) >> BUG_LINE_LO_WIDTH) <<
> > BUG_DISP_WIDTH);
> >
> > \
> > unsigned int line_hi = ((line & ((1 << BUG_LINE_LO_WIDTH) - 1))
> > <<
> > BUG_DISP_WIDTH);
> >
> > \
> >
> > ALLOCATE_BF_SECTION(1);
> > \
> > *(signed int *)(&bug_frames) = (unsigned long)
> > &&UNIQUE_BUG_INSTR_LABEL(line) - (unsigned long)&bug_frames +
> > line_lo;
> > \
> > *((signed int *)(&bug_frames) + 1) = (unsigned long)file -
> > (unsigned long)&bug_frames +
> > line_hi;
> > \
> > bug_var.msg_disp[1] = (signed int)((unsigned long)#msg -
> > (unsigned
> > long)&bug_frames);
> >
> > \
> >
> > UNIQUE_BUG_INSTR_LABEL(line):
> > \
> > asm volatile
> > ("nop");
> > } while (0)
> >
> > extern char bug_frames[];
> >
> > static struct bug_frame bug_var
> > __attribute__((section("bug_frames")));
> >
> > int main() {
> > BUG_FRAME(1, __LINE__, __FILE__, 1, "TEST");
> >
> > printf("bug_loc: %p\n", bug_loc(&bug_var));
> > printf("bug_ptr: 0x%x\n", bug_ptr(&bug_var));
> > printf("__FILE__: %s\n", (char *)bug_ptr(&bug_var));
> > printf("bug_line: %d\n", bug_line(&bug_var));
> > printf("msg: %s\n", bug_msg(&bug_var));
> >
> > BUG_FRAME(1, __LINE__, __FILE__, 1, "NEW TEST");
> >
> > printf("bug_loc: %p\n", bug_loc(&bug_var));
> > printf("bug_ptr: 0x%x\n", bug_ptr(&bug_var));
> > printf("__FILE__: %s\n", (char *)bug_ptr(&bug_var));
> > printf("bug_line: %d\n", bug_line(&bug_var));
> > printf("msg: %s\n", bug_msg(&bug_var));
> >
> > return 0;
> > }
> >
> > Do you think it makes any sense? In this case, all BUG_FRAME can be
> > re-
> > used among all architectures, and only bug instructions should be
> > changed.
> >
> > >
> > > ~Andrew
> > ~ Oleksii
>
next prev parent reply other threads:[~2023-02-17 9:33 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-03 17:05 [PATCH v1 0/4] introduce generic implementation of macros from bug.h Oleksii Kurochko
2023-02-03 17:05 ` [PATCH v1 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
2023-02-13 12:24 ` Jan Beulich
2023-02-13 13:19 ` Julien Grall
2023-02-13 13:30 ` Jan Beulich
2023-02-13 13:56 ` Julien Grall
2023-02-13 15:02 ` Jan Beulich
2023-02-13 15:07 ` Julien Grall
2023-02-13 15:14 ` Jan Beulich
2023-02-13 13:33 ` Andrew Cooper
2023-02-13 13:55 ` Julien Grall
2023-02-14 16:22 ` Oleksii
2023-02-14 16:55 ` Jan Beulich
2023-02-15 15:06 ` Oleksii
2023-02-15 17:59 ` Oleksii
2023-02-16 7:31 ` Jan Beulich
2023-02-16 10:44 ` Oleksii
2023-02-16 12:09 ` Oleksii
2023-02-16 12:19 ` Andrew Cooper
2023-02-16 20:44 ` Oleksii
2023-02-17 7:12 ` Jan Beulich
2023-02-17 9:33 ` Oleksii [this message]
2023-02-16 14:55 ` Jan Beulich
2023-02-16 10:48 ` Andrew Cooper
2023-02-16 12:21 ` Oleksii
2023-02-16 14:53 ` Jan Beulich
2023-02-03 17:05 ` [PATCH v1 2/4] xen/arm: switch ARM to use generic implementation of bug.h Oleksii Kurochko
2023-02-13 13:02 ` Jan Beulich
2023-02-14 16:28 ` Oleksii
2023-02-03 17:05 ` [PATCH v1 3/4] xen/x86: switch x86 to use generic implemetation " Oleksii Kurochko
2023-02-13 13:10 ` Jan Beulich
2023-02-14 16:41 ` Oleksii
2023-02-03 17:05 ` [PATCH v1 4/4] xen: change <asm/bug.h> to <xen/bug.h> Oleksii Kurochko
2023-02-13 13:13 ` Jan Beulich
2023-02-14 16:44 ` Oleksii
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=75a018704e212c043370269d10e4b4a45146e9b3.camel@gmail.com \
--to=oleksii.kurochko@gmail.com \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=jbeulich@suse.com \
--cc=julien@xen.org \
--cc=sstabellini@kernel.org \
--cc=wl@xen.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.