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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

Hi Jan!

Thanks a lot for starting the review of the patch series!

On Mon, 2023-02-13 at 13:24 +0100, Jan Beulich wrote:
> On 03.02.2023 18:05, Oleksii Kurochko wrote:
> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -92,6 +92,12 @@ config STATIC_MEMORY
> >  
> >           If unsure, say N.
> >  
> > +config GENERIC_DO_BUG_FRAME
> > +       bool
> > +       help
> > +         Generic do_bug_frame() function is needed to handle the
> > type of bug
> > +         frame and print an information about it.
> 
> Generally help text for prompt-less functions is not very useful.
> Assuming
> it is put here to inform people actually reading the source file, I'm
> okay
> for it to be left here, but please at least drop the stray "an". I
> also
> think this may want moving up in the file, e.g. ahead of all the
> HAS_*
> controls (which, as you will notice, all have no help text either).
> Plus
> finally how about shorter and more applicable GENERIC_BUG_FRAME -
> after
> all what becomes generic is more than just do_bug_frame()?
Thanks for the comments. I will take them into account.
Right now only do_bug_frame() is expected to be generic.

> 
> > --- /dev/null
> > +++ b/xen/common/bug.c
> > @@ -0,0 +1,88 @@
> > +#include <xen/bug.h>
> > +#include <xen/errno.h>
> > +#include <xen/types.h>
> > +#include <xen/kernel.h>
> 
> Please order headers alphabetically unless there's anything
> preventing
> that from being done.
Sure, thanks.

> 
> > +#include <xen/string.h>
> > +#include <xen/virtual_region.h>
> > +
> > +#include <asm/processor.h>
> > +
> > +int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
> 
> I know Arm is using vaddr_t and RISC-V now also has it, but in UNIX-
> like
> environments that's redundant with "unsigned long", and it's also
> redundant with C99's uintptr_t. Hence when seeing the type I always
> wonder whether it's really a host virtual address which is meant (and
> not perhaps a guest one, which in principle could differ from the
> host
> one for certain guest types). In any event the existence of this type
> should imo not be a prereq to using this generic piece of
> infrastructure.
> 
I am OK with changing vaddr_t to 'unsigned long' and agree with your
point
so if no one is against that I'll change it.

> > +{
> > +    const struct bug_frame *bug = NULL;
> > +    const char *prefix = "", *filename, *predicate;
> > +    unsigned long fixup;
> > +    int id = -1, lineno;
> 
> For both of these I question them needing to be of a signed type.
I took the code from ARM but it looks like the type of id and lineno
can be changed to 'unsgined int'. So I'll update the code
correspondingly.

> 
> > +    const struct virtual_region *region;
> > +
> > +    region = find_text_region(pc);
> > +    if ( region )
> > +    {
> > +        for ( id = 0; id < BUGFRAME_NR; id++ )
> > +        {
> > +            const struct bug_frame *b;
> > +            unsigned int i;
> > +
> > +            for ( i = 0, b = region->frame[id].bugs;
> > +                  i < region->frame[id].n_bugs; b++, i++ )
> > +            {
> > +                if ( ((vaddr_t)bug_loc(b)) == pc )
> 
> Afaics this is the sole use of bug_loc(). If so, better change the
> macro
> to avoid the need for a cast here:
> 
> #define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp)
> 
Thanks for the recommendation. It makes sense to change the macro so
I'll update it too.

> > --- /dev/null
> > +++ b/xen/include/xen/bug.h
> > @@ -0,0 +1,127 @@
> > +#ifndef __XEN_BUG_H__
> > +#define __XEN_BUG_H__
> > +
> > +#define BUG_DISP_WIDTH    24
> > +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> > +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> > +
> > +#define BUGFRAME_run_fn 0
> > +#define BUGFRAME_warn   1
> > +#define BUGFRAME_bug    2
> > +#define BUGFRAME_assert 3
> > +
> > +#define BUGFRAME_NR     4
> > +
> > +#ifndef __ASSEMBLY__
> > +
> > +#include <xen/errno.h>
> > +#include <xen/stringify.h>
> > +#include <xen/types.h>
> > +#include <xen/lib.h>
> 
> Again - please sort headers.
> 
> > +#include <asm/bug.h>
> > +
> > +#ifndef BUG_FRAME_STUFF
> > +struct bug_frame {
> 
> Please can we have a blank line between the above two ones and then
> similarly
> ahead of the #endif?
Sure.

> 
> > +    signed int loc_disp;    /* Relative address to the bug address
> > */
> > +    signed int file_disp;   /* Relative address to the filename */
> > +    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 */
> 
> Already the original comment in Arm code is wrong: The padding
> doesn't
> guarantee 8-byte alignment; it merely guarantees a multiple of 8
> bytes
> size. Aiui there's also no need for 8-byte alignment anywhere here
> (in
> fact there's ".p2align 2" in the generator macros).
> 
> I also wonder why this needs to be a named bitfield: Either make it
> plain uint16_t, or if you use a bitfield, then omit the name.
> 
It will better to change it to 'uint16_t' as I don't see any reason to
use 'uint32_t' with bitfield here.
I'll check if we need the alignment. If there  is '.p2align 2' we
really don't need it.

> > +};
> > +
> > +#define bug_loc(b) ((const void *)(b) + (b)->loc_disp)
> > +#define bug_file(b) ((const void *)(b) + (b)->file_disp);
> > +#define bug_line(b) ((b)->line)
> > +#define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
> > +#endif /* BUG_FRAME_STUFF */
> > +
> > +#ifndef BUG_FRAME
> > +/* 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. BUGFRAME_run_fn needs to be handled separately.
> > + */
> 
> When generalizing the logic, I wonder in how far the comment doesn't
> want re-wording some. For example, while Arm prefixes constant insn
> operands with # (and x86 uses $), there's no such prefix in RISC-V.
> At
> which point there's no need to use %c in the first place. (Which in
> turn means x86'es more compact representation may also be usable
> there.
> And yet in turn the question arises whether RISC-V wouldn't better
> have
> its own derivation of the machinery, rather than trying to generalize
> things. RISC-V's would then likely be closer to x86'es, just without
> the use of %c on asm() operands. Which might then suggest to rather
> generalize x86'es variant down the road.)
ARM version is more portable because of the '%c' modifier which is not
present everywhere, so I decided to use it as a generic implementation.
Moreover I don't see any reason why we can't switch x86 implementation
to 'generic/ARM'.
And one more thing if you look at WARN/BUG/ASSERT_FAILED/BUG_FRAME
macros implementation to use it in assembly code the implementations
are closer to 'generic/ARM'.

> 
> At the very least the comment's style wants correcting, and in the
> first
> sentence s/doesn't/don't/. Also %c isn't a parameter, but a modifier.
> 
Thanks!

> > +#define BUG_FRAME(type, line, file, has_msg, msg) do
> > {                      \
> > +    BUILD_BUG_ON((line) >>
> > 16);                                             \
> > +    BUILD_BUG_ON((type) >=
> > BUGFRAME_NR);                                    \
> > +    asm
> > ("1:"BUG_INSTR"\n"                                                 
> > \
> 
> Nit: Style (missing blank after opening parenthesis, and then also at
> the
> end of the construct ahead of the closing parenthesis).
> 
I'll fix it.

> > +         ".pushsection .rodata.str, \"aMS\", %progbits,
> > 1\n"                \
> > +         "2:\t.asciz " __stringify(file)
> > "\n"                               \
> 
> file is always a string literal; really it always is __FILE__ in this
> non-x86 implementation. So first preference ought to be to drop the
> macro parameter and use __FILE__ here (same then for line vs
> __LINE__).
> Yet even if not done like that, the __stringify() is largely unneeded
> (unless we expect file names to have e.g. backslashes in their names)
> and looks somewhat odd here. So how about
> 
>          "2:\t.asciz \"" __FILE__ "\"\n"
> 
> ? But wait - peeking ahead to the x86 patch I notice that __FILE__
> and
> __LINE__ need to remain arguments. But then the second preference
> would
> still be
> 
>          "2:\t.asciz \"" file "\"\n"
> 
> imo. Yet maybe others disagree ...
> 
> > +        
> > "3:\n"                                                            
> > \
> > +         ".if " #has_msg
> > "\n"                                               \
> > +         "\t.asciz " #msg
> > "\n"                                              \
> > +        
> > ".endif\n"                                                        
> > \
> > +        
> > ".popsection\n"                                                   
> > \
> > +         ".pushsection .bug_frames." __stringify(type) ", \"a\",
> > %progbits\n"\
> > +        
> > "4:\n"                                                            
> > \
> > +         ".p2align
> > 2\n"                                                     \
> > +         ".long (1b -
> > 4b)\n"                                                \
> > +         ".long (2b -
> > 4b)\n"                                                \
> > +         ".long (3b -
> > 4b)\n"                                                \
> > +         ".hword " __stringify(line) ",
> > 0\n"                                \
> 
> Hmm, .hword. How generic is support for that in assemblers? I notice
> even
> very old gas has support for it, but architectures may not consider
> it
> two bytes wide. (On x86, for example, it's bogus to be two bytes,
> since
> .word also produces 2 bytes of data. And indeed MIPS and NDS32
> override it
> in gas to produce 1 byte of data only, at least in certain cases.)
> I'd
> like to suggest to use a fourth .long here, and to drop the padding
> field
> from struct bug_frame in exchange.
Changing .hword to .half can make the code more portable and generic,
as .half is a more standard and widely supported assembler directive
for declaring 16-bit data. 
But probably you are right and it will be easier to change it to .long.

> 
> Furthermore, once assembly code is generalized, you need to pay
> attention
> to formatting: Only labels may start at the beginning of a line; in
> particular directives never should.
> 
> > +        
> > ".popsection");                                                   
> > \
> > +} while (0)
> 
> Nit: Style (missing blanks, and preferably "false" instead of "0").
> 
> > +#endif /* BUG_FRAME */
> > +
> > +#ifndef run_in_exception_handler
> > +/*
> > + * GCC will not allow to use "i"  when PIE is enabled (Xen doesn't
> > set the
> > + * flag but instead rely on the default value from the compiler).
> > So the
> > + * easiest way to implement run_in_exception_handler() is to pass
> > the to
> > + * be called function in a fixed register.
> > + */
> > +#define  run_in_exception_handler(fn) do
> > {                                  \
> 
> Nit: Just one blank please after #define (and on the first comment
> line
> there's also a double blank where only one should be).
> 
> > +    register void *fn_ asm(__stringify(BUG_FN_REG)) =
> > (fn);                 \
> 
> x86 makes sure "fn" is of correct type. Arm doesn't, which likely is
> a
> mistake. May I ask that you use the correct type here (which is even
> better than x86'es extra checking construct):
> 
>     register void (*fn_)(struct cpu_user_regs *)
> asm(__stringify(BUG_FN_REG)) = (fn);
> 
the type of 'fn' should be updated. I'll take into account in the next
version of patch.

> > +    asm
> > ("1:"BUG_INSTR"\n"                                                 
> > \
> > +         ".pushsection .bug_frames." __stringify(BUGFRAME_run_fn)
> > ","       \
> > +         "             \"a\",
> > %%progbits\n"                                 \
> > +        
> > "2:\n"                                                            
> > \
> > +         ".p2align
> > 2\n"                                                     \
> > +         ".long (1b -
> > 2b)\n"                                                \
> > +         ".long 0, 0,
> > 0\n"                                                  \
> > +         ".popsection" :: "r"
> > (fn_));                                       \
> > +} while (0)
> > +#endif /* run_in_exception_handler */
> > +
> > +#ifndef WARN
> > +#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, "")
> > +#endif /* WARN */
> > +
> > +#ifndef BUG
> > +#define BUG() do {                                              \
> > +    BUG_FRAME(BUGFRAME_bug,  __LINE__, __FILE__, 0, "");        \
> > +    unreachable();                                              \
> > +} while (0)
> > +#endif
> > +
> > +#ifndef assert_failed
> > +#define assert_failed(msg) do {                                 \
> > +    BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg);     \
> > +    unreachable();                                              \
> > +} while (0)
> > +#endif
> > +
> > +extern const struct bug_frame __start_bug_frames[],
> > +                              __stop_bug_frames_0[],
> > +                              __stop_bug_frames_1[],
> > +                              __stop_bug_frames_2[],
> > +                              __stop_bug_frames_3[];
> > +
> > +#else /* !__ASSEMBLY__ */
> > +
> > +#ifdef CONFIG_X86
> > +#include <asm/bug.h>
> > +#endif
> 
> Why this x86 special? (To be precise: Why can this not be done
> uniformly?)
> 
We can leave only '#include <asm/bug.h>'. I had an issue during making
the code more generic. I removed it and re-run tests and it looks like
it is fine to go w/o '#ifdef...':
https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/777288336

> Jan
~ Oleksii