Re: [PATCH v1 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME

[Jan Beulich <jbeulich@xxxxxxxx>]

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