Re: [patches] [PATCH 1/9] RISC-V: Init and Halt Code

[Palmer Dabbelt]

On Tue, 04 Jul 2017 14:54:01 PDT (-0700), j.neuschaefer@xxxxxxx wrote:
> Hi, below are some small comments.
>
> On Tue, Jul 04, 2017 at 12:50:54PM -0700, Palmer Dabbelt wrote:
>> This contains the various __init C functions, the initial assembly
>> kernel entry point, and the code to reset the system.  When a file was
>> init-related, it contains
>
> It contains what?
>
>> Signed-off-by: Palmer Dabbelt <palmer@xxxxxxxxxxx>
> [...]
>
>> +#ifdef CONFIG_GENERIC_BUG
>> +#define __BUG_INSN	_AC(0x00100073, UL) /* sbreak */
>
> This should be ebreak, not sbreak, in Priv Spec 1.10, AFAICT.
> I guess binutils still understands sbreak, but it's nicer to stick to
> the spec, IMHO.

I agree.  IIRC they're the same instruction, we just alias sbreak->ebreak in
binutils (like scall->ecall).

>> +#define BUG()							\
>> +do {								\
>> +	__asm__ __volatile__ (					\
>> +		"1:\n\t"					\
>> +			"sbreak\n"				\
>
> ebreak
>
>> +			".pushsection __bug_table,\"a\"\n\t"	\
>> +		"2:\n\t"					\
>> +			__BUG_ENTRY "\n\t"			\
>> +			".org 2b + %2\n\t"			\
>> +			".popsection"				\
>> +		:						\
>> +		: "i" (__FILE__), "i" (__LINE__),		\
>> +		  "i" (sizeof(struct bug_entry)));		\
>> +	unreachable();						\
>> +} while (0)
>> +#endif /* !__ASSEMBLY__ */
>> +#else /* CONFIG_GENERIC_BUG */
>> +#ifndef __ASSEMBLY__
>> +#define BUG()							\
>> +do {								\
>> +	__asm__ __volatile__ ("sbreak\n");			\
>
> ebreak
>
>> +	unreachable();						\
>> +} while (0)
>> +#endif /* !__ASSEMBLY__ */
>> +#endif /* CONFIG_GENERIC_BUG */
> [...]
>
>> +#define DO_ERROR_INFO(name, signo, code, str)				\
>> +asmlinkage void name(struct pt_regs *regs)				\
>> +{									\
>> +	do_trap_error(regs, signo, code, regs->sepc, "Oops - " str);	\
>> +}
>> +
>> +DO_ERROR_INFO(do_trap_unknown,
>> +	SIGILL, ILL_ILLTRP, "unknown exception");
>> +DO_ERROR_INFO(do_trap_insn_misaligned,
>> +	SIGBUS, BUS_ADRALN, "instruction address misaligned");
>> +DO_ERROR_INFO(do_trap_insn_fault,
>> +	SIGBUS, BUS_ADRALN, "instruction access fault");
>
> For a general instruction access fault, BUS_ADRALN seems wrong. A
> variant of SIGSEGV seems more appropriate, IMHO.

How does this look?

  diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
  index 4c693b5b9980..3ce9ac6e736e 100644
  --- a/arch/riscv/kernel/traps.c
  +++ b/arch/riscv/kernel/traps.c
  @@ -112,7 +112,7 @@ DO_ERROR_INFO(do_trap_unknown,
   DO_ERROR_INFO(do_trap_insn_misaligned,
          SIGBUS, BUS_ADRALN, "instruction address misaligned");
   DO_ERROR_INFO(do_trap_insn_fault,
  -       SIGBUS, BUS_ADRALN, "instruction access fault");
  +       SIGBUS, SEGV_ACCERR, "instruction access fault");
   DO_ERROR_INFO(do_trap_insn_illegal,
          SIGILL, ILL_ILLOPC, "illegal instruction");
   DO_ERROR_INFO(do_trap_load_misaligned,