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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2023-02-23 at 11:11 +0100, Jan Beulich wrote:
> 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.
I checked the source code I found that debugger_trap_fatal() is
generalized:
https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/include/xen/debugger.h
So we can use in generic version of do_bug_frame().

About fixup_exception_return() you are right as we can it handle(call)
by the caller so it's needed only to return a suitable return value for
do_bug_frame().

> 
> > > > +{
> > > > +    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.
Ahh, yeah. I am always missing CONFIG_...
> 
> > > > +        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.)
Thanks for the explanation.
> 
> > > > +    ".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.
Thank you very much for the explanation. 
> 
> Jan
~ Oleksii