|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 3/5] xen/riscv: introduce an implementation of macros from <asm/bug.h>
On Mon, 2023-02-27 at 13:59 +0100, Jan Beulich wrote:
> On 24.02.2023 12:35, Oleksii Kurochko wrote:
> > The patch introduces macros: BUG(), WARN(), run_in_exception(),
> > assert_failed.
> >
> > The implementation uses "ebreak" instruction in combination with
> > diffrent bug frame tables (for each type) which contains useful
> > information.
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes in V4:
> > - Updates in RISC-V's <asm/bug.h>:
> > * Add explanatory comment about why there is only defined for
> > 32-bits length
> > instructions and 16/32-bits BUG_INSN_{16,32}.
> > * Change 'unsigned long' to 'unsigned int' inside
> > GET_INSN_LENGTH().
> > * Update declaration of is_valid_bugaddr(): switch return type
> > from int to bool
> > and the argument from 'unsigned int' to 'vaddr'.
> > - Updates in RISC-V's traps.c:
> > * replace /xen and /asm includes
> > * update definition of is_valid_bugaddr():switch return type
> > from int to bool
> > and the argument from 'unsigned int' to 'vaddr'. Code style
> > inside function
> > was updated too.
> > * do_bug_frame() refactoring:
> > * local variables start and bug became 'const struct
> > bug_frame'
> > * bug_frames[] array became 'static const struct bug_frame[]
> > = ...'
> > * remove all casts
> > * remove unneeded comments and add an explanatory comment
> > that the do_bug_frame()
> > will be switched to a generic one.
> > * do_trap() refactoring:
> > * read 16-bits value instead of 32-bits as compressed
> > instruction can
> > be used and it might happen than only 16-bits may be
> > accessible.
> > * code style updates
> > * re-use instr variable instead of re-reading instruction.
> > - Updates in setup.c:
> > * add blank line between xen/ and asm/ includes.
> > ---
> > Changes in V3:
> > - Rebase the patch "xen/riscv: introduce an implementation of
> > macros
> > from <asm/bug.h>" on top of patch series [introduce generic
> > implementation
> > of macros from bug.h]
> > ---
> > Changes in V2:
> > - Remove __ in define namings
> > - Update run_in_exception_handler() with
> > register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn);
> > - Remove bug_instr_t type and change it's usage to uint32_t
> > ---
> > xen/arch/riscv/include/asm/bug.h | 48 ++++++++++
> > xen/arch/riscv/include/asm/processor.h | 2 +
> > xen/arch/riscv/setup.c | 1 +
> > xen/arch/riscv/traps.c | 125
> > +++++++++++++++++++++++++
> > xen/arch/riscv/xen.lds.S | 10 ++
> > 5 files changed, 186 insertions(+)
> > create mode 100644 xen/arch/riscv/include/asm/bug.h
> >
> > diff --git a/xen/arch/riscv/include/asm/bug.h
> > b/xen/arch/riscv/include/asm/bug.h
> > new file mode 100644
> > index 0000000000..67ade6f895
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/bug.h
> > @@ -0,0 +1,48 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2012 Regents of the University of California
> > + * Copyright (C) 2021-2023 Vates
> > + *
> > + */
> > +#ifndef _ASM_RISCV_BUG_H
> > +#define _ASM_RISCV_BUG_H
> > +
> > +#include <xen/types.h>
> > +
> > +#ifndef __ASSEMBLY__
>
> This #ifndef contradicts the xen/types.h inclusion. Either the
> #include
> moves down below here, or the #ifndef goes away as pointless. This is
> because xen/types.h cannot be include in assembly files.
I will remove it. It looks like it leave when as generic implementation
was taken ARM.
>
> > +#define BUG_INSTR "ebreak"
> > +
> > +/*
> > + * The base instruction set has a fixed length of 32-bit naturally
> > aligned
> > + * instructions.
> > + *
> > + * There are extensions of variable length ( where each
> > instruction can be
> > + * any number of 16-bit parcels in length ) but they aren't used
> > in Xen
> > + * and Linux kernel ( where these definitions were taken from ).
> > + *
> > + * Compressed ISA is used now where the instruction length is 16
> > bit and
> > + * 'ebreak' instruction, in this case, can be either 16 or 32 bit
> > (
> > + * depending on if compressed ISA is used or not )
> > + */
> > +#define INSN_LENGTH_MASK _UL(0x3)
> > +#define INSN_LENGTH_32 _UL(0x3)
> > +
> > +#define BUG_INSN_32 _UL(0x00100073) /* ebreak */
> > +#define BUG_INSN_16 _UL(0x9002) /* c.ebreak */
> > +#define COMPRESSED_INSN_MASK _UL(0xffff)
> > +
> > +#define GET_INSN_LENGTH(insn) \
> > +({ \
> > + unsigned int len; \
> > + len = ((insn & INSN_LENGTH_MASK) == INSN_LENGTH_32) ? \
> > + 4UL : 2UL; \
>
> Why the UL suffixes?
>
> > + len; \
> > +})
>
> And anyway - can't the whole macro be written without using any
> extension:
>
> #define GET_INSN_LENGTH(insn) \
> (((insn) & INSN_LENGTH_MASK) == INSN_LENGTH_32 ? 4 : 2) \
>
> ? (Note also that uses of macro parameters should be properly
> parenthesized.)
It can. I'll update the macros.
>
> > --- a/xen/arch/riscv/setup.c
> > +++ b/xen/arch/riscv/setup.c
> > @@ -1,3 +1,4 @@
> > +#include <xen/bug.h>
> > #include <xen/compile.h>
> > #include <xen/init.h>
>
> Why? Eventually this will be needed, I agree, but right now?
Agree. <xen/bug.h> should be removed fro the current patch.
Probably merge error during rebase.
>
> > @@ -99,7 +100,131 @@ static void do_unexpected_trap(const struct
> > cpu_user_regs *regs)
> > die();
> > }
> >
> > +void show_execution_state(const struct cpu_user_regs *regs)
> > +{
> > + early_printk("implement show_execution_state(regs)\n");
> > +}
> > +
> > +/*
> > + * TODO: change early_printk's function to early_printk with
> > format
> > + * when s(n)printf() will be added.
> > + *
> > + * Probably the TODO won't be needed as generic do_bug_frame() (at
> > + * least, for ARM and RISC-V) has been introduced and current
> > + * implementation will be replaced with generic one when panic(),
> > + * printk() and find_text_region() (virtual memory?) will be
> > + * ready/merged
> > + */
> > +int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
> > +{
> > + const struct bug_frame *start, *end;
> > + const struct bug_frame *bug = NULL;
> > + unsigned int id = 0;
> > + const char *filename, *predicate;
> > + int lineno;
> > +
> > + static const struct bug_frame* bug_frames[] = {
> > + &__start_bug_frames[0],
> > + &__stop_bug_frames_0[0],
> > + &__stop_bug_frames_1[0],
> > + &__stop_bug_frames_2[0],
> > + &__stop_bug_frames_3[0],
> > + };
> > +
> > + for ( id = 0; id < BUGFRAME_NR; id++ )
> > + {
> > + start = bug_frames[id];
> > + end = bug_frames[id + 1];
> > +
> > + while ( start != end )
> > + {
> > + if ( (vaddr_t)bug_loc(start) == pc )
> > + {
> > + bug = start;
> > + goto found;
> > + }
> > +
> > + start++;
> > + }
> > + }
> > +
> > + found:
> > + if ( bug == NULL )
> > + return -ENOENT;
> > +
> > + if ( id == BUGFRAME_run_fn )
> > + {
> > + void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
> > +
> > + fn(regs);
> > +
> > + goto end;
> > + }
> > +
> > + /* WARN, BUG or ASSERT: decode the filename pointer and line
> > number. */
> > + filename = bug_ptr(bug);
> > + lineno = bug_line(bug);
> > +
> > + switch ( id )
> > + {
> > + case BUGFRAME_warn:
> > + early_printk("Xen WARN at ");
> > + early_printk(filename);
> > + early_printk("\n");
> > +
> > + show_execution_state(regs);
> > +
> > + goto end;
> > +
> > + case BUGFRAME_bug:
> > + early_printk("Xen BUG at ");
> > + early_printk(filename);
> > + early_printk("\n");
> > +
> > + show_execution_state(regs);
> > + early_printk("change wait_for_interrupt to panic() when
> > common is available\n");
> > + die();
> > +
> > + case BUGFRAME_assert:
> > + /* ASSERT: decode the predicate string pointer. */
> > + predicate = bug_msg(bug);
> > +
> > + early_printk("Assertion \'");
> > + early_printk(predicate);
> > + early_printk("\' failed at ");
> > + early_printk(filename);
> > + early_printk("\n");
> > +
> > + show_execution_state(regs);
> > + early_printk("change wait_for_interrupt to panic() when
> > common is available\n");
> > + die();
> > + }
> > +
> > + return -EINVAL;
> > +
> > + end:
> > + return 0;
> > +}
>
> Wasn't the goal to use the generic do_bug_frame()?
It's the goal but at the current stage we can't switch to
do_bug_frame() because it is based on function from xen/common which we
don't have now for RISC-V. So it will be switched when xen/common will
be available.
>
> > +bool is_valid_bugaddr(vaddr_t insn)
>
> Why is insn of type vaddr_t?
Initially address of insn was passed here. But now we should change
vaddr_t to 'uint32_t' as the instruction length can be either 16 or 32
and update instr variable inside do_trap():
static uint32_t read_instr(unsigned long pc)
{
uint16_t instr16 = *(uint16_t *)pc;
if ( GET_INSN_LENGTH(instr16) == 2 )
return (uint32_t)instr16;
else
return *(uint32_t *)pc;
}
void do_trap(struct cpu_user_regs *cpu_regs)
{
register_t pc = cpu_regs->sepc;
uint32_t instr = read_instr(pc);
...
>
> > +{
> > + if ( (insn & INSN_LENGTH_MASK) == INSN_LENGTH_32 )
> > + return ( insn == BUG_INSN_32 );
> > + else
> > + return ( (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16 );
>
> Nit (style): The kind-of-operand to return is an expression. Hence
> you have stray blanks there immediately inside the parentheses.
> (This is unlike e.g. if(), where you've formatted things correctly.)
To be 100% sure, should it be:
return ( ( insn & COMPRESSED_INSN_MASK ) == BUG_INSN_16 );
> > +}
> > +
> > void do_trap(struct cpu_user_regs *cpu_regs)
> > {
> > + register_t pc = cpu_regs->sepc;
> > + uint16_t instr = *(uint16_t *)pc;
> > +
> > + if ( is_valid_bugaddr(instr) ) {
> > + if ( !do_bug_frame(cpu_regs, cpu_regs->sepc) ) {
>
> Nit: Brace placement (twice).
Thanks. I'll update then.
>
> Jan
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |