[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v1 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
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 >
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |