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()?
> --- /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.
> +#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.
> +{
> + 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.
> + 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)
> --- /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?
> + 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.
> +};
> +
> +#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.)
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.
> +#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).
> + ".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.
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);
> + 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?)
Jan
