Re: [PATCH v2 12/14] xen/riscv: introduce an implementation of macros from <asm/bug.h>

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.02.2023 23:11, Julien Grall wrote:
> On 01/02/2023 17:40, Oleksii wrote:
>> I wrote the following macros and they have been compiled without any
>> errors:
>>                          .....
>> #define _ASM_BUGFRAME_TEXT(second_frame)                       \
>>      ".Lbug%=: ebreak\n"                                        \
>>      ".pushsection .bug_frames.%c[bf_type], \"a\", @progbits\n" \
>>      ".p2align 2\n"                                             \
>>      ".Lfrm%=:\n"                                               \
>>      ".long (.Lfrm%=)\n"                                        \
>>      ".long (0x55555555)\n"                                     \
>>      ".long (.Lbug%=)\n"                                        \
>>      ".long (0x55555555)\n"                                     \
>>      ".long %c[bf_line_hi]\n"                                   \
>>      ".long (0x55555555)\n"                                     \
>>      ".long %[bf_line_hi]\n"                                    \
>>      ".long (0x55555555)\n"                                     \
>>      ".long %[bf_line_lo]\n"                                    \
>>      ".long (0x55555555)\n"                                     \
>>      ".long %[bf_ptr]\n"                                        \
>>      ".long (0x55555555)\n"                                     \
>>      ".long (.Lbug%= - .Lfrm%=) + %c[bf_line_hi]\n"             \
>>      ".popsection\n"                                            \
>>
>> #define _ASM_BUGFRAME_INFO(type, line, ptr, msg)               \
>>      [bf_type]    "i" (type),                                   \
>>      [bf_ptr]     "i" (ptr),                                    \
>>      [bf_msg]     "i" (msg),                                    \
>>      [bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) - 1))  \
>>                        << BUG_DISP_WIDTH),                      \
>>      [bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH)
>>
>> #define BUG_FRAME(type, line, ptr, second_frame, msg) do {     \
>>      __asm__ __volatile__ ( _ASM_BUGFRAME_TEXT(second_frame)    \
>>                     :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) ); \
>> } while (0)
>>                            ....
>>
>>
>> But if add ".long %c[bf_ptr]\n" then the following compilation error
>> will occur: [ error: invalid 'asm': invalid use of '%c'. ]
>>
>> If you look at the dissembler of _bug_frames_...
>>                                 ......
>>      80201010:   1010                    addi    a2,sp,32   # .Lfrm%=
>>      80201012:   8020                    .2byte  0x8020
>>      80201014:   5555                    li      a0,-11
>>      80201016:   5555                    li      a0,-11
>>      80201018:   3022                    .2byte  0x3022  # .Lbug%=
>>      8020101a:   8020                    .2byte  0x8020
>>      8020101c:   5555                    li      a0,-11
>>      8020101e:   5555                    li      a0,-11
>>      80201020:   0000                    unimp          # %c[bf_line_hi]
>>      80201022:   0000                    unimp
>>      80201024:   5555                    li      a0,-11
>>      80201026:   5555                    li      a0,-11
>>      80201028:   0000                    unimp           # %[bf_line_hi]
>>      8020102a:   0000                    unimp
>>      8020102c:   5555                    li      a0,-11
>>      8020102e:   5555                    li      a0,-11
>>      80201030:   0000                    unimp           # %[bf_line_lo]
>>      80201032:   1600                    addi    s0,sp,800
>>      80201034:   5555                    li      a0,-11
>>      80201036:   5555                    li      a0,-11
>>      80201038:   10b8                    addi    a4,sp,104   # %[bf_ptr]
>>      8020103a:   8020                    .2byte  0x8020
>>      8020103c:   5555                    li      a0,-11
>>      8020103e:   5555                    li      a0,-11
>>      80201040:   2012                    .2byte  0x2012   # (.Lbug%= -
>> .Lfrm%=) + %c[bf_line_hi]
>>                                 .....
>> ... it looks like the error will be generated if the name in %c[name]
>> isn't equal to 0.
>>
>> Another thing I noticed is that %[name] can be used instead of %c[name]
>> for RISC-V ( i did a quick check and it works) so it is still possible
>> to follow Intel implementation but required a redefinition of
>> _ASM_BUGFRAME_TEXT where %c[...] won't be used. All the rest will be
>> the same as in x86 implementation:
>>                                  .....
>> #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"                                           \
>>                                   .....
>>
>> One thing I would like to ask you is why it's better to follow/use x86
>> implementation instead of ARM?
> 
> IMHO, the x86 version is cleaner. But...
> 
>> It seems that "%c[...]" has the best support only for Intel GCC and
>> thereby ARM implementation looks more universal, doesn't it?
> 
> ... you are right, the Arm version is more portable. Although, I do 
> wonder how GCC managed to have a different behavior between architecture 
> as I would have expected %c to be a generic implementation.

While the implementation is common, the condition when 'c' is legitimate
can be customized by arch-specific code. While all code for all of the
four architectures does so, perhaps x86'es goes farther with what it
permits.

Jan