Re: [PATCH v5 1/3] xen/arm: add support for run_in_exception_handler()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.12.2020 14:39, Julien Grall wrote:
> On 15/12/2020 09:02, Jan Beulich wrote:
>> On 15.12.2020 07:33, Juergen Gross wrote:
>>> --- a/xen/include/asm-arm/bug.h
>>> +++ b/xen/include/asm-arm/bug.h
>>> @@ -15,65 +15,62 @@
>>>   
>>>   struct bug_frame {
>>>       signed int loc_disp;    /* Relative address to the bug address */
>>> -    signed int file_disp;   /* Relative address to the filename */
>>> +    signed int ptr_disp;    /* Relative address to the filename or 
>>> function */
>>>       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_ptr(b) ((const void *)(b) + (b)->ptr_disp);
>>>   #define bug_line(b) ((b)->line)
>>>   #define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
>>>   
>>> -#define BUGFRAME_warn   0
>>> -#define BUGFRAME_bug    1
>>> -#define BUGFRAME_assert 2
>>> +#define BUGFRAME_run_fn 0
>>> +#define BUGFRAME_warn   1
>>> +#define BUGFRAME_bug    2
>>> +#define BUGFRAME_assert 3
>>>   
>>> -#define BUGFRAME_NR     3
>>> +#define BUGFRAME_NR     4
>>>   
>>>   /* 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.
>>>    */
>>> -#define BUG_FRAME(type, line, file, has_msg, msg) do {                     
>>>  \
>>> +#define BUG_FRAME(type, line, ptr, 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"                                                            
>>>  \
>>> +         ".pushsection .bug_frames." __stringify(type) ", \"a\", 
>>> %%progbits\n"\
>>> +         "2:\n"                                                            
>>>  \
>>>            ".p2align 2\n"                                                   
>>>   \
>>> -         ".long (1b - 4b)\n"                                               
>>>  \
>>> -         ".long (2b - 4b)\n"                                               
>>>  \
>>> -         ".long (3b - 4b)\n"                                               
>>>  \
>>> +         ".long (1b - 2b)\n"                                               
>>>  \
>>> +         ".long (%0 - 2b)\n"                                               
>>>  \
>>> +         ".long (%1 - 2b)\n"                                               
>>>  \
>>>            ".hword " __stringify(line) ", 0\n"                              
>>>   \
>>> -         ".popsection");                                                   
>>>  \
>>> +         ".popsection" :: "i" (ptr), "i" (msg));                           
>>>  \
>>>   } while (0)
>>
>> The comment ahead of the construct now looks to be at best stale, if
>> not entirely pointless. The reference to %c looks quite strange here
>> to me anyway - I can only guess it appeared here because on x86 one
>> has to use %c to output constants as operands for .long and alike,
>> and this was then tried to use on Arm as well without there really
>> being a need.
> 
> Well, %c is one the reason why we can't have a common BUG_FRAME 
> implementation. So I would like to retain this information before 
> someone wants to consolidate the code and missed this issue.

Fair enough, albeit I guess this then could do with re-wording.

> Regarding the mergeable string section, I agree that the comment in now 
> stale. However,  could someone confirm that "i" will still retain the 
> same behavior as using mergeable string sections?

That's depend on compiler settings / behavior.

Jan