Re: [PATCH v2 12/14] xen/riscv: introduce an implementation of macros from <asm/bug.h>

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Fri, 2023-01-27 at 15:34 +0100, Jan Beulich wrote:
> On 27.01.2023 14:59, 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:
> >   - 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
> 
> But that's not correct - as said before, you can't assume you can
> access
> 32 bits, there maybe only a 16-bit insn at the end of a page, with
> nothing
> mapped to the VA of the subsequent page. Even more ...
> 
Agree that it will be an issue if 16-bit insn will be at the end of a
page.
The code is based on Linux
(https://elixir.bootlin.com/linux/latest/source/arch/riscv/kernel/traps.c#L152)
and it seems they might have the same issue.
> > + end:
> > +    regs->sepc += GET_INSN_LENGTH(*(uint32_t *)pc);
> 
> ... you obtain insn length you don't even need to read 32 bits.
> 
It looks you are right so I'll change that in the new version of the
patch series.

> Jan
Oleksii