Re: [PATCH v13 1/2] xen/riscv: enable GENERIC_BUG_FRAME

[oleksii.kurochko@xxxxxxxxx]

On Wed, 2024-08-07 at 16:39 +0200, Jan Beulich wrote:
> On 06.08.2024 18:37, Oleksii Kurochko wrote:
> > Enable GENERIC_BUG_FRAME to support BUG(), WARN(), ASSERT,
> > and run_in_exception_handler().
> > 
> > "UNIMP" is used for BUG_INSTR, which, when macros from <xen/bug.h>
> > are used, triggers an exception with the ILLEGAL_INSTRUCTION cause.
> > This instruction is encoded as a 2-byte instruction when
> > CONFIG_RISCV_ISA_C is enabled:
> >   ffffffffc0046ba0:       0000                    unimp
> > and is encoded as a 4-byte instruction when CONFIG_RISCV_ISA_C
> > ins't enabled:
> >   ffffffffc005a460:       c0001073                unimp
> > 
> > Using 'ebreak' as BUG_INSTR does not guarantee proper handling of
> > macros
> > from <xen/bug.h>. If a debugger inserts a breakpoint (using the
> > 'ebreak'
> > instruction) at a location where Xen already uses 'ebreak', it
> > creates ambiguity. Xen cannot distinguish whether the 'ebreak'
> > instruction is inserted by the debugger or is part of Xen's own
> > code.
> > 
> > Remove BUG_INSN_32 and BUG_INSN_16 macros as they encode the ebreak
> > instruction, which is no longer used for BUG_INSN.
> > 
> > Update the comment above the definition of INS_LENGTH_MASK as
> > instead of
> > 'ebreak' instruction 'unimp' instruction is used.
> > 
> > <xen/lib.h> is included for the reason that panic() and printk()
> > are
> > used in common/bug.c and RISC-V fails if it is not included.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> 
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
Thanks.

> 
> Just one more (cosmetic) question:
> 
> > --- a/xen/arch/riscv/include/asm/bug.h
> > +++ b/xen/arch/riscv/include/asm/bug.h
> > @@ -9,7 +9,7 @@
> >  
> >  #ifndef __ASSEMBLY__
> >  
> > -#define BUG_INSTR "ebreak"
> > +#define BUG_INSTR "UNIMP"
> 
> Deliberately all uppercase?
It could be lowercase without any issue. It was mentioned in uppercase
in RISC-V assembly manual:
```
To better diagnose situations where the program flow reaches an
unexpected
location, you might want to emit there an instruction that's known to
trap. You
can use an `UNIMP` pseudoinstruction, ...
```

~ Oleksii