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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.02.2023 17:16, Oleksii wrote:
> On Wed, 2023-02-22 at 13:46 +0100, Jan Beulich wrote:
>> On 20.02.2023 17:40, Oleksii Kurochko wrote:
>>> --- /dev/null
>>> +++ b/xen/common/bug.c
>>> @@ -0,0 +1,113 @@
>>> +#include <xen/bug.h>
>>> +#include <xen/errno.h>
>>> +#include <xen/kernel.h>
>>> +#include <xen/livepatch.h>
>>> +#include <xen/string.h>
>>> +#include <xen/types.h>
>>> +#include <xen/virtual_region.h>
>>> +
>>> +#include <asm/processor.h>
>>
>> Is this really needed here?
> Yes, it is.
> 
> Function show_execution_state() is declared in this header for all
> architectures and is used by handle_bug_frame().

Ugly, but yes, you're right.

>>> +const struct bug_frame* find_bug_frame(unsigned long pc, unsigned
>>> int *id)
>>
>> Is this function going to be needed outside of this CU? IOW why is it
>> not
>> static?
>>
> It's not static because there is not possible to use do_bug_frame() as
> is for x86 as x86 has some additional checks for some cases which
> aren't in generic implementation:
> [1]
> https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/x86/traps.c#L1217
> [2]
> https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/x86/traps.c#L1238
> [3]
> https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/x86/traps.c#L1259
> 
> Probably to make generic do_bug_frame() more re-usable for x86 we can
> implement as stubs fixup_exception_return() and debugger_trap_fatal()
> under #ifndef X86 ... #endif inside common/bug.c.
> 
> Could you please share your thoughts about that?

Isn't all that's needed a suitable return value from the function,
for the caller to take appropriate further action? (Maybe even that
isn't really necessary.)

That said, debugger_trap_fatal() imo may sensibly be generalized,
and hence could be left in common code. Arm may simply stub this to
nothing then for the time being.

>>> +{
>>> +    const char *prefix = "", *filename, *predicate;
>>> +    unsigned long fixup;
>>> +    unsigned int lineno;
>>> +
>>> +    if ( id == BUGFRAME_run_fn )
>>> +    {
>>> +#ifdef ARM        
>>
>> Who or what defines ARM? Anyway, seeing ...
> it is defined by default in Kconfig:
> https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/arm/Kconfig#L13

That'll be CONFIG_ARM then in uses in C code.

>>> +        void (*fn)(const struct cpu_user_regs *) = (void *)regs-
>>>> BUG_FN_REG;
>>
>> ... this, wouldn't it be better (and independent of the specific
>> arch) if
>> you checked for BUG_FN_REG being defined?
> If I understand Kconfig correctly than there is no significant
> difference what check. But probably BUG_FN_REG would be a little bit
> better if someone will decide to change a way how pointer to function
> will be passed in case of ARM than we will get compilation error and so
> won't miss to fix the line in do_bug_frame().

Indeed - #ifdef like this one generally want to check for the precise
aspect in question, without making assumptions about something implying
something else. (Pretty certainly there are exceptions to this rule,
but it holds here.)

>>> +    ".p2align
>>> 2\n"                                                          \
>>> +   
>>> ".Lfrm%=:\n"                                                       
>>>      \
>>> +    ".long (.Lbug%= - .Lfrm%=) +
>>> %"MODIFIER"[bf_line_hi]\n"                  \
>>> +    ".long (%"MODIFIER"[bf_ptr] - .Lfrm%=) +
>>> %"MODIFIER"[bf_line_lo]\n"       \
>>> +    ".if " #second_frame
>>> "\n"                                               \
>>> +    ".long 0, %"MODIFIER"[bf_msg] -
>>> .Lfrm%=\n"                               \
>>> +   
>>> ".endif\n"                                                         
>>>      \
>>> +    ".popsection\n"
>>
>> I think I said so in reply to an earlier version already: The moment
>> assembly code moves to a common header, it should be adjusted to be
>> as
>> portable as possible. In particular directives should never start at
>> the
>> beginning of a line, while labels always should. (Whether .long is
>> actually portable is another question, which I'll be happy to leave
>> aside for now.)
> I am not sure that I understand about which one directive we are
> talking about... Are we talking about .{push/pop}section and .p2align?
> Probably you can show me an example in Xen or other project?

I'm talking about all directives here. Fundamentally assembly language
source lines are like this (assuming colons follow labels):

[<label>:|<blank>][<directive>|<insn>]

Both parts can optionally be empty, but if the right one isn't, then
the left one also shouldn't be (and hence minimally a blank is needed;
commonly it would be a tab). Directives, unlike insns, start with a
dot in most dialects. Labels can also start with a dot, but are
disambiguated by the colon after them (yet more strict placement of
items is therefore required when labels are not followed by colons -
there are such dialects -, which is why it is generally a good idea
to follow that simple formatting rule). Also, ftaod,
- <insn> covers both actual insns as well as assembler macro
  invocations,
- there can of course be multiple labels on a single line (iirc this
  requires that colons follow labels).

As to examples: I'm afraid I'm unaware of arch-independent assembly
code anywhere in Xen so far.

Jan