Re: [PATCH v6 4/4] xen/x86: switch x86 to use generic implemetation of bug.h
On Wed, 2023-03-08 at 16:33 +0100, Jan Beulich wrote:
> On 07.03.2023 16:50, Oleksii Kurochko wrote:
> > @@ -1166,12 +1167,9 @@ void cpuid_hypervisor_leaves(const struct
> > vcpu *v, uint32_t leaf,
> >
> > void do_invalid_op(struct cpu_user_regs *regs)
> > {
> > - const struct bug_frame *bug = NULL;
> > u8 bug_insn[2];
> > - const char *prefix = "", *filename, *predicate, *eip = (char
> > *)regs->rip;
> > - unsigned long fixup;
> > - int id = -1, lineno;
> > - const struct virtual_region *region;
> > + void *eip = (void *)regs->rip;
>
> As said elsewhere already: "const" please whenever possible. The more
> that
> the variable was pointer-to-const before.
Sure, I will change it than to:
const void *eip = (const void *)regs->rip;
>
> > @@ -1185,83 +1183,20 @@ void do_invalid_op(struct cpu_user_regs
> > *regs)
> > memcmp(bug_insn, "\xf\xb", sizeof(bug_insn)) )
> > goto die;
> >
> > - region = find_text_region(regs->rip);
> > - 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 ( bug_loc(b) == eip )
> > - {
> > - bug = b;
> > - goto found;
> > - }
> > - }
> > - }
> > - }
> > -
> > - found:
> > - if ( !bug )
> > + id = do_bug_frame(regs, regs->rip);
> > + if ( id < 0 )
> > goto die;
> > - eip += sizeof(bug_insn);
> > - if ( id == BUGFRAME_run_fn )
> > - {
> > - void (*fn)(struct cpu_user_regs *) = bug_ptr(bug);
> > -
> > - fn(regs);
> > - fixup_exception_return(regs, (unsigned long)eip);
> > - return;
> > - }
> >
> > - /* WARN, BUG or ASSERT: decode the filename pointer and line
> > number. */
> > - filename = bug_ptr(bug);
> > - if ( !is_kernel(filename) && !is_patch(filename) )
> > - goto die;
> > - fixup = strlen(filename);
> > - if ( fixup > 50 )
> > - {
> > - filename += fixup - 47;
> > - prefix = "...";
> > - }
> > - lineno = bug_line(bug);
> > + eip = (unsigned char *)eip + sizeof(bug_insn);
>
> Why don't you keep the original
>
> eip += sizeof(bug_insn);
>
> ? As also said before we use the GNU extension of arithmetic on
> pointers
> to void pretty extensively elsewhere; there's no reason at all to
> introduce an unnecessary and questionable cast here.
Just missed that during rebase. ( I experimented with the branch and
received conflicts that were resolved incorrectly )
I will update that. Thanks
>
> With these adjustments and with the re-basing over the changes
> requested
> to patch 2
> Reviewed-by: Jan Beulich
>
> Jan
~ Oleksii
Re: [PATCH v6 4/4] xen/x86: switch x86 to use generic implemetation of bug.h
On 07.03.2023 16:50, Oleksii Kurochko wrote:
> @@ -1166,12 +1167,9 @@ void cpuid_hypervisor_leaves(const struct vcpu *v,
> uint32_t leaf,
>
> void do_invalid_op(struct cpu_user_regs *regs)
> {
> -const struct bug_frame *bug = NULL;
> u8 bug_insn[2];
> -const char *prefix = "", *filename, *predicate, *eip = (char *)regs->rip;
> -unsigned long fixup;
> -int id = -1, lineno;
> -const struct virtual_region *region;
> +void *eip = (void *)regs->rip;
As said elsewhere already: "const" please whenever possible. The more that
the variable was pointer-to-const before.
> @@ -1185,83 +1183,20 @@ void do_invalid_op(struct cpu_user_regs *regs)
> memcmp(bug_insn, "\xf\xb", sizeof(bug_insn)) )
> goto die;
>
> -region = find_text_region(regs->rip);
> -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 ( bug_loc(b) == eip )
> -{
> -bug = b;
> -goto found;
> -}
> -}
> -}
> -}
> -
> - found:
> -if ( !bug )
> +id = do_bug_frame(regs, regs->rip);
> +if ( id < 0 )
> goto die;
> -eip += sizeof(bug_insn);
> -if ( id == BUGFRAME_run_fn )
> -{
> -void (*fn)(struct cpu_user_regs *) = bug_ptr(bug);
> -
> -fn(regs);
> -fixup_exception_return(regs, (unsigned long)eip);
> -return;
> -}
>
> -/* WARN, BUG or ASSERT: decode the filename pointer and line number. */
> -filename = bug_ptr(bug);
> -if ( !is_kernel(filename) && !is_patch(filename) )
> -goto die;
> -fixup = strlen(filename);
> -if ( fixup > 50 )
> -{
> -filename += fixup - 47;
> -prefix = "...";
> -}
> -lineno = bug_line(bug);
> +eip = (unsigned char *)eip + sizeof(bug_insn);
Why don't you keep the original
eip += sizeof(bug_insn);
? As also said before we use the GNU extension of arithmetic on pointers
to void pretty extensively elsewhere; there's no reason at all to
introduce an unnecessary and questionable cast here.
With these adjustments and with the re-basing over the changes requested
to patch 2
Reviewed-by: Jan Beulich
Jan
[PATCH v6 4/4] xen/x86: switch x86 to use generic implemetation of bug.h
The following changes were made:
* Make GENERIC_BUG_FRAME mandatory for X86
* Update asm/bug.h using generic implementation in
* Update do_invalid_op using generic do_bug_frame()
* Define BUG_DEBUGGER_TRAP_FATAL to debugger_trap_fatal(X86_EXC_GP,regs)
* type of eip variable was changed to 'void *'
Signed-off-by: Oleksii Kurochko
---
Changes in V6:
* update the commit message
* update the type of eip to 'void *' in do_invalid_op()
* fix the logic of do_invalid_op()
* move macros BUG_DEBUGGER_TRAP_FATAL under #ifndef __ASSEMBLY__ as
it is not necessary to be in assembly code.
---
Changes in V5:
* Nothing changed
---
Changes in V4:
* Back comment /* !__ASSEMBLY__ */ for #else case in
* Remove changes related to x86/.../asm/debuger.h as do_bug_frame() prototype
was updated and cpu_user_regs isn't const any more.
---
Changes in V3:
* As prototype and what do_bug_frame() returns was changed so patch 3 and 4
was updated to use a new version of do_bug_frame
* MODIFIER was change to BUG_ASM_CONST to align with generic implementation
---
Changes in V2:
* Remove all unnecessary things from as they were introduced in
.
* Define BUG_INSTR = 'ud2' and MODIFIER = 'c' ( it is needed to skip '$'
when use an imidiate in x86 assembly )
* Update do_invalid_op() to re-use handle_bug_frame() and find_bug_frame()
from generic implemetation of CONFIG_GENERIC_BUG_FRAME
* Code style fixes.
---
xen/arch/x86/Kconfig | 1 +
xen/arch/x86/include/asm/bug.h | 77 ++--
xen/arch/x86/traps.c | 81 --
3 files changed, 12 insertions(+), 147 deletions(-)
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 6a7825f4ba..b0ff1f3ee6 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -11,6 +11,7 @@ config X86
select ARCH_MAP_DOMAIN_PAGE
select ARCH_SUPPORTS_INT128
select CORE_PARKING
+ select GENERIC_BUG_FRAME
select HAS_ALTERNATIVE
select HAS_COMPAT
select HAS_CPUFREQ
diff --git a/xen/arch/x86/include/asm/bug.h b/xen/arch/x86/include/asm/bug.h
index ff5cca1f19..f852cd0ee9 100644
--- a/xen/arch/x86/include/asm/bug.h
+++ b/xen/arch/x86/include/asm/bug.h
@@ -1,83 +1,12 @@
#ifndef __X86_BUG_H__
#define __X86_BUG_H__
-#undef BUG_DISP_WIDTH
-#undef BUG_LINE_LO_WIDTH
-#undef BUG_LINE_HI_WIDTH
-
-#define BUG_DISP_WIDTH24
-#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
-#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
-
#ifndef __ASSEMBLY__
-#define BUG_FRAME_STRUCT
-
-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) ((const void *)(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])
-
-#define _ASM_BUGFRAME_TEXT(second_frame) \
-".Lbug%=: ud2\n" \
-".pushsection .bug_frames.%c[bf_type], \"a\", @progbits\n" \
-".p2align 2\n" \
-".Lfrm%=:\n" \
-".long (.Lbug%= - .Lfrm%=) + %c[bf_line_hi]\n" \
-".long (%c[bf_ptr] - .Lfrm%=) + %c[bf_line_lo]\n"\
-".if " #second_frame "\n"\
-".long 0, %c[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)
-
-#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);
