|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v5 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME
On Mon, 2023-03-06 at 11:17 +0100, Jan Beulich wrote:
> > On 03.03.2023 11:38, Oleksii Kurochko wrote:
> > > > --- a/xen/common/Kconfig
> > > > +++ b/xen/common/Kconfig
> > > > @@ -28,6 +28,9 @@ config ALTERNATIVE_CALL
> > > > config ARCH_MAP_DOMAIN_PAGE
> > > > bool
> > > >
> > > > +config GENERIC_BUG_FRAME
> > > > + bool
> >
> > With Arm now also going to use the generic logic, do we actually
> > need
> > this
> > control anymore (provided things have been proven to work on Arm
> > for
> > the
> > compiler range we support there)? It looks like because of the way
> > the
> > series is partitioned it may be necessary transiently, but it
> > should
> > be
> > possible to drop it again in a new 5th patch.
We still need it because RISC-V doesn't ready yet to use do_bug_frame()
from xen/common/bug.c as it requires find_text_region(),
virtual_region() and panic().
The mentioned functionality isn't ready now for RISC-V so RISC-V will
use only generic things from <xen/bug.h> only for some time.
Another one reason I would like to leave the config it is probably the
same situation will be for future architectures.
This is not something mandatory if the config isn't really necessary
than it should be removed after RISC-V will be ready to migrate full on
generic implementation.
> >
> > > > --- /dev/null
> > > > +++ b/xen/common/bug.c
> > > > @@ -0,0 +1,103 @@
> > > > +#include <xen/bug.h>
> > > > +#include <xen/debugger.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>
> > > > +
> > > > +/*
> > > > + * Returns a negative value in case of an error otherwise the
> > > > bug
> > > > type.
> > > > + */
> >
> > Nit: Style (this is a single line comment). But see also below
> > related
> > comment on the function's declaration.
Thanks. I'll fix that.
> >
> > > > +int do_bug_frame(struct cpu_user_regs *regs, unsigned long pc)
> > > > +{
> > > > + const struct bug_frame *bug = NULL;
> > > > + const struct virtual_region *region;
> > > > + const char *prefix = "", *filename, *predicate;
> > > > + unsigned long fixup;
> > > > + unsigned int id = BUGFRAME_NR, lineno;
> > > > +
> > > > + region = find_text_region(pc);
> > > > + if ( !region )
> > > > + return -EINVAL;
> > > > +
> > > > + for ( id = 0; id < BUGFRAME_NR; id++ )
> > > > + {
> > > > + const struct bug_frame *b;
> > > > + size_t i;
> > > > +
> > > > + for ( i = 0, b = region->frame[id].bugs;
> > > > + i < region->frame[id].n_bugs; b++, i++ )
> >
> > Nit: Indentation (the "i" on the 2nd line wants to align with that
> > on the 1st one).
Thanks. I'll update the indentation.
> >
> > > > + {
> > > > + if ( bug_loc(b) == pc )
> > > > + {
> > > > + bug = b;
> > > > + goto found;
> > > > + }
> > > > + }
> > > > + }
> > > > +
> > > > + found:
> > > > + if ( !bug )
> > > > + return -EINVAL;
> >
> > While I'm generally unhappy with many uses of -EINVAL (it's used to
> > indicate way too many different kinds of errors), can we at least
> > consider using -ENOENT here instead? (I'm sorry, I should have
> > asked
> > for this earlier on.)
Sure. Done.
> >
> > > > --- /dev/null
> > > > +++ b/xen/include/xen/bug.h
> > > > @@ -0,0 +1,158 @@
> > > > +#ifndef __XEN_BUG_H__
> > > > +#define __XEN_BUG_H__
> > > > +
> > > > +#define BUGFRAME_run_fn 0
> > > > +#define BUGFRAME_warn 1
> > > > +#define BUGFRAME_bug 2
> > > > +#define BUGFRAME_assert 3
> > > > +
> > > > +#define BUGFRAME_NR 4
> > > > +
> > > > +#ifndef BUG_FRAME_STRUCT
> >
> > This check won't help when it comes ahead of ...
> >
> > > > +#define BUG_DISP_WIDTH 24
> > > > +#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
> > > > +#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
> > > > +#endif
> > > > +
> > > > +#include <asm/bug.h>
> >
> > ... this. But is the #ifdef actually necessary? Or can the #define-
> > s
> > be moved ...
> >
> > > > +#ifndef BUG_DEBUGGER_TRAP_FATAL
> > > > +#define BUG_DEBUGGER_TRAP_FATAL(regs) 0
> > > > +#endif
> > > > +
> > > > +#ifndef __ASSEMBLY__
> > > > +
> > > > +#include <xen/lib.h>
> > > > +
> > > > +#ifndef BUG_FRAME_STRUCT
> >
> > ... here? (I guess having them defined early, but unconditionally
> > is
> > better. If an arch wants to override them, they can #undef and then
> > #define.)
We can't move it to #indef __ASSEMBLY__ because in this case x86
compilation will fail as in x86's <asm/bug.h> BUG_DISP_WIDTH,
BUG_LINE_LO_WIDTH, BUG_LINE_HI_WIDTH are used in case when the header
is included in assembly code.
I agree that there is no any sense to have the defines under "#ifndef
BUG_FRAME_STUCT" before <asm/bug.h> but it is necessary to define them
before <asm/bug.h>. The defines were put under "#ifndef
BUG_FRAME_STUCT" because it seems to me that logically the defines
should go only with definition of 'struct bug_frame'.
So it looks like the only one way we have. It is define them
unconditionally before <asm/bug.h> and #undef if it will be necessary
for specific architecture.
As an option we can move the defines to #ifndef BUG_FRAME_STRUCT under
#ifndef __ASSEMBLY__ and then define them explicitly in x86's
<asm/bug.h> for case when the header is included some where in assembly
code. But this option looks really weird.
> >
> > Above from here the "#ifndef __ASSEMBLY__" also wants to move up,
> > to
> > further enclose BUG_DEBUGGER_TRAP_FATAL (which is useless in
> > assembly
> > code).
Yeah, sure. Will move up "#ifndef __ASSEMBLY__"
> >
> > > > +struct bug_frame {
> > > > + signed int loc_disp:BUG_DISP_WIDTH;
> > > > + unsigned int line_hi:BUG_LINE_HI_WIDTH;
> > > > + signed int ptr_disp:BUG_DISP_WIDTH;
> > > > + unsigned int line_lo:BUG_LINE_LO_WIDTH;
> > > > + signed int msg_disp[];
> > > > +};
> > > > +
> > > > +#define bug_loc(b) ((unsigned long)(b) + (b)->loc_disp)
> > > > +
> > > > +#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp)
> > > > +
> > > > +#define bug_line(b) (((((b)->line_hi + ((b)->loc_disp < 0))
> > > > & \
> > > > + ((1 << BUG_LINE_HI_WIDTH) - 1))
> > > > << \
> > > > + BUG_LINE_LO_WIDTH)
> > > > + \
> > > > + (((b)->line_lo + ((b)->ptr_disp < 0))
> > > > & \
> > > > + ((1 << BUG_LINE_LO_WIDTH) - 1)))
> > > > +
> > > > +#define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1])
> > > > +
> > > > +#endif /* BUG_FRAME_STRUCT */
> > > > +
> > > > +/*
> > > > + * Generic implementation has been based on x86 implementation
> > > > where
> > > > + * '%c' to deal with punctation sign ($, # depending on
> > > > architecture)
> > > > + * before immediate.
> > > > + *
> > > > + * Not all architecture's compilers have full support of '%c'
> > > > and
> > > > not for all
> > > > + * assemblers punctation sign is used before immediate.
> > > > + * Thereby it was decided to introduce BUG_ASM_CONST.
> > > > + */
> >
> > I have to admit that I'm not really happy with this comment. At the
> > very
> > least the last sentence imo doesn't belong there. But overall how
> > about
> > something like
> >
> > "Some architectures mark immediate instruction operands in a
> > special
> > way.
> > In such cases the special marking may need omitting when
> > specifying
> > directive operands. Allow architectures to specify a suitable
> > modifier."
Your comment looks really better. So I'll use it.
> >
> > > > +#ifndef BUG_ASM_CONST
> > > > +#define BUG_ASM_CONST ""
> > > > +#endif
> > > > +
> > > > +#if !defined(_ASM_BUGFRAME_TEXT) ||
> > > > !defined(_ASM_BUGFRAME_INFO)
> >
> > Please don't make the conditional more complicated than necessary:
> > Checking for just _ASM_BUGFRAME_TEXT is enough here.
Sure. I'll update that.
> >
> > > > +#define
> > > > _ASM_BUGFRAME_TEXT(second_frame)
> > > >
> > > > \
> > > > +
> > > > ".Lbug%=:"BUG_INSTR"\n"
> > > >
> > > > \
> > > > + " .pushsection .bug_frames.%"BUG_ASM_CONST"[bf_type],
> > > > \"a\",
> > > > %%progbits\n" \
> > > > + " .p2align
> > > > 2\n"
> > > >
> > > > \
> > > > +
> > > > ".Lfrm%=:\n"
> > > >
> > > > \
> > > > + " .long (.Lbug%= - .Lfrm%=) +
> > > > %"BUG_ASM_CONST"[bf_line_hi]\n" \
> > > > + " .long (%"BUG_ASM_CONST"[bf_ptr] - .Lfrm%=) +
> > > > %"BUG_ASM_CONST"[bf_line_lo]\n"\
> > > > + " .if " #second_frame
> > > > "\n" \
> > > > + " .long 0, %"BUG_ASM_CONST"[bf_msg] -
> > > > .Lfrm%=\n" \
> > > > + "
> > > > .endif\n"
> > > >
> > > > \
> > > > + " .popsection\n"
> > > > +
> > > > +#define _ASM_BUGFRAME_INFO(type, line, ptr,
> > > > msg) \
> > > > + [bf_type] "i"
> > > > (type), \
> > > > + [bf_ptr] "i"
> > > > (ptr), \
> > > > + [bf_msg] "i"
> > > > (msg), \
> > > > + [bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) -
> > > > 1)) \
> > > > + <<
> > > > BUG_DISP_WIDTH), \
> > > > + [bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) <<
> > > > BUG_DISP_WIDTH)
> > > > +
> > > > +#endif /* _ASM_BUGFRAME_TEXT || _ASM_BUGFRAME_INFO */
> > > > +
> > > > +#ifndef BUG_FRAME
> > > > +
> > > > +#define BUG_FRAME(type, line, ptr, second_frame, msg) do
> > > > { \
> > > > + BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH +
> > > > BUG_LINE_HI_WIDTH)); \
> > > > + BUILD_BUG_ON((type) >=
> > > > BUGFRAME_NR); \
> > > > + asm volatile (
> > > > _ASM_BUGFRAME_TEXT(second_frame) \
> > > > + :: _ASM_BUGFRAME_INFO(type, line, ptr, msg)
> > > > ); \
> > > > +} while (0)
> >
> > Isn't this tied to BUG_FRAME_STRUCT being defined (or not)? At
> > least
> > the 1st BUILD_BUG_ON() looks problematic if an arch wasn't to use
> > the generic struct: With how you have things right now
> > BUG_LINE_{LO,HI}_WIDTH may not be defined, and the check would also
> > be at risk of causing false positives. Perhaps it's appropriate to
> > have a separate #ifdef (incl the distinct identifier used), but
> > that
> > first BUILD_BUG_ON() needs abstracting out.
Missed that. Thanks.
I'll introduce that a separate #ifdef before BUG_FRAME:
#ifndef BUILD_BUG_ON_LINE_WIDTH
#define BUILD_BUG_ON_LINE_WIDTH \
BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH))
#endif
> >
> > Also nit: Style (see ...
> >
> > > > +#endif
> > > > +
> > > > +#ifndef run_in_exception_handler
> > > > +
> > > > +/*
> > > > + * TODO: untangle header dependences, break BUILD_BUG_ON() out
> > > > of
> > > > xen/lib.h,
> > > > + * and use a real static inline here to get proper type
> > > > checking
> > > > of fn().
> > > > + */
> > > > +#define
> > > > run_in_exception_handler(fn) \
> > > > + do
> > > > { \
> > > > + (void)((fn) == (void (*)(struct cpu_user_regs
> > > > *))NULL); \
> > > > + BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0,
> > > > NULL); \
> > > > + } while ( 0 )
> >
> > ... here). Also, considering the boolean nature, I guess we're in
> > the
> > process of moving such to be "} while ( false );".
> >
> > Also please be consistent with formatting of the two adjacent
> > macros,
> > both indentation-wise and where "do {" is placed. Which of the two
> > forms you use is secondary.
Thanks. Done.
> >
> > > > +#endif /* run_in_exception_handler */
> > > > +
> > > > +#ifndef WARN
> > > > +#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0,
> > > > NULL)
> > > > +#endif
> > > > +
> > > > +#ifndef BUG
> > > > +#define BUG() do
> > > > { \
> > > > + BUG_FRAME(BUGFRAME_bug, __LINE__, __FILE__, 0,
> > > > NULL); \
> > > > +
> > > > unreachable(); \
> > > > +} while (0)
> > > > +#endif
> > > > +
> > > > +#ifndef assert_failed
> > > > +#define assert_failed(msg) do
> > > > { \
> > > > + BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1,
> > > > msg); \
> > > > +
> > > > unreachable(); \
> > > > +} while (0)
> > > > +#endif
> > > > +
> > > > +#ifdef CONFIG_GENERIC_BUG_FRAME
> > > > +
> > > > +struct cpu_user_regs;
> > > > +
> > > > +/*
> > > > + * Returns a negative value in case of an error otherwise the
> > > > bug
> > > > type.
> > > > + */
> >
> > Perhaps add "(BUGFRAME_*)", which would then also make this a
> > properly
> > multi-line comment (right now it's a style violation, as is the
> > case
> > for the function definition as pointed out above)?
> >
Updated. Thanks.
~ Oleksii
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |