|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 12/14] xen/riscv: introduce an implementation of macros from <asm/bug.h>
Hello Jan,
Thanks for the comments!
On Tue, 2023-02-07 at 16:07 +0100, Jan Beulich wrote:
> On 07.02.2023 15:46, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/bug.h
> > @@ -0,0 +1,38 @@
> > +/* 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__
> > +
> > +#define BUG_FN_REG t0
> > +
> > +#define BUG_INSTR "ebreak"
> > +
> > +#define INSN_LENGTH_MASK _UL(0x3)
> > +#define INSN_LENGTH_32 _UL(0x3)
>
> I assume these are deliberately over-simplified (not accounting for
> wider than 32-bit insns in any way)?
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 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 long len; \
> > + len = ((insn & INSN_LENGTH_MASK) == INSN_LENGTH_32) ? \
> > + 4UL : 2UL; \
> > + len; \
>
> Any reason for the use of "unsigned long" (not "unsigned int") here?
>
There is no specific reason (at least I don't see it now). It looks
like it can be used here even smaller type than 'unsigned int' as len,
in current case, can be either 4 or 2.
> > +})
> > +
> > +/* These are defined by the architecture */
> > +int is_valid_bugaddr(uint32_t addr);
>
> A function by this name very likely wants to return bool. Also -
> uint32_t (not "unsigned long") for an address?
>
It should be unsigned long.
> > --- a/xen/arch/riscv/setup.c
> > +++ b/xen/arch/riscv/setup.c
> > @@ -1,6 +1,6 @@
> > +#include <xen/bug.h>
> > #include <xen/compile.h>
> > #include <xen/init.h>
> > -
> > #include <asm/csr.h>
> > #include <asm/early_printk.h>
> > #include <asm/traps.h>
>
> I think it is good practice to have a blank line between xen/ and
> asm/
> includes.
>
Thanks. I will bring a blank line back.
> > --- a/xen/arch/riscv/traps.c
> > +++ b/xen/arch/riscv/traps.c
> > @@ -8,6 +8,7 @@
> > #include <asm/early_printk.h>
> > #include <asm/processor.h>
> > #include <asm/traps.h>
> > +#include <xen/bug.h>
> > #include <xen/errno.h>
> > #include <xen/lib.h>
>
> Perhaps wants adjusting earlier in the series: Preferably all xen/
> ahead of all asm/.
>
I'll fix it in the next version of the patch series.
> > @@ -97,7 +98,136 @@ 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");
> > +}
> > +
> > +int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
> > +{
> > + struct bug_frame *start, *end;
> > + struct bug_frame *bug = NULL;
>
> const?
regs aren't changed in the function so I decided to put it as const.
>
> > + unsigned int id = 0;
> > + const char *filename, *predicate;
> > + int lineno;
> > +
> > + unsigned long bug_frames[] = {
> > + (unsigned long)&__start_bug_frames[0],
> > + (unsigned long)&__stop_bug_frames_0[0],
> > + (unsigned long)&__stop_bug_frames_1[0],
> > + (unsigned long)&__stop_bug_frames_2[0],
> > + (unsigned long)&__stop_bug_frames_3[0],
> > + };
> > +
> > + for ( id = 0; id < BUGFRAME_NR; id++ )
> > + {
> > + start = (struct bug_frame *)bug_frames[id];
> > + end = (struct bug_frame *)bug_frames[id + 1];
>
> Nit: Stray double blanks. But I'd like to suggest that you get away
> without casts here in the first place. Such casts are always a
> certain
> risk going forward.
Do you mean that it is better to re-write bug_frame[] to:
struct bug_frane bug_frames[] = {
&__start_bug_frame[0],
...
>
> > + 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 *) = (void *)regs-
> > >BUG_FN_REG;
> > +
> > + fn(regs);
> > +
> > + goto end;
> > + }
> > +
> > + /* WARN, BUG or ASSERT: decode the filename pointer and line
> > number. */
> > + filename = bug_file(bug);
> > + lineno = bug_line(bug);
> > +
> > + switch ( id )
> > + {
> > + case BUGFRAME_warn:
> > + /*
> > + * TODO: change early_printk's function to early_printk
> > with format
> > + * when s(n)printf() will be added.
> > + */
> > + early_printk("Xen WARN at ");
> > + early_printk(filename);
> > + early_printk(":");
> > + // early_printk_hnum(lineno);
>
> What's this? At the very least the comment is malformed.
It's an old code that should be removed.
>
> > + show_execution_state(regs);
> > +
> > + goto end;
> > +
> > + case BUGFRAME_bug:
> > + /*
> > + * TODO: change early_printk's function to early_printk
> > with format
> > + * when s(n)printf() will be added.
> > + */
> > + early_printk("Xen BUG at ");
> > + early_printk(filename);
> > + early_printk(":");
> > + // early_printk_hnum(lineno);
> > +
> > + 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);
> > +
> > + /*
> > + * TODO: change early_printk's function to early_printk
> > with format
> > + * when s(n)printf() will be added.
> > + */
> > + early_printk("Assertion \'");
> > + early_printk(predicate);
> > + early_printk("\' failed at ");
> > + early_printk(filename);
> > + early_printk(":");
> > + // early_printk_hnum(lineno);
> > +
> > + show_execution_state(regs);
> > + early_printk("change wait_for_interrupt to panic() when
> > common is available\n");
> > + die();
> > + }
> > +
> > + return -EINVAL;
> > +
> > + end:
> > + return 0;
> > +}
> > +
> > +int is_valid_bugaddr(uint32_t insn)
> > +{
> > + if ((insn & INSN_LENGTH_MASK) == INSN_LENGTH_32)
>
> Nit: Style.
Thanks. I'll fix it.
>
> > + return (insn == BUG_INSN_32);
> > + else
> > + return ((insn & COMPRESSED_INSN_MASK) == BUG_INSN_16);
> > +}
> > +
> > void do_trap(struct cpu_user_regs *cpu_regs)
> > {
> > + register_t pc = cpu_regs->sepc;
> > + uint32_t instr = *(uint32_t *)pc;
>
> You still read a 32-bit value when only 16 bits may be accessible.
Since there were a lot of comments on the previous version of patch
series, this comment slipped out of my head.
I'll fix in new version of the patch series.
>
> > + if (is_valid_bugaddr(instr)) {
> > + if (!do_bug_frame(cpu_regs, cpu_regs->sepc)) {
>
> I'm pretty sure I did point out the style issues here.
>
> > + cpu_regs->sepc += GET_INSN_LENGTH(*(uint32_t *)pc);
>
> Why would you re-read the insn here, when you have it in a local
> variable
> already?
Inattention. I'll re-use local variable.
>
> Jan
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |