[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v1 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
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). 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 |