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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew