Re: [PATCH v9 4/5] xen/riscv: enable GENERIC_BUG_FRAME

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2024-07-10 at 12:01 +0200, Jan Beulich wrote:
> On 02.07.2024 13:23, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> >  xen/arch/riscv/Kconfig |  1 +
> >  xen/arch/riscv/traps.c | 31 +++++++++++++++++++++++++++++++
> >  xen/common/bug.c       |  1 +
> >  3 files changed, 33 insertions(+)
> > 
> > diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
> > index b4b354a778..74ad019fe7 100644
> > --- a/xen/arch/riscv/Kconfig
> > +++ b/xen/arch/riscv/Kconfig
> > @@ -5,6 +5,7 @@ config RISCV
> >  config RISCV_64
> >     def_bool y
> >     select 64BIT
> > +   select GENERIC_BUG_FRAME
> 
> Any particular reason to put this here, and not higher up with RISCV?
Yes, you are right it would be better to put it inside "config RISCV".

> 
> > @@ -101,8 +102,38 @@ static void do_unexpected_trap(const struct
> > cpu_user_regs *regs)
> >      die();
> >  }
> >  
> > +static bool is_valid_bug_insn(uint32_t insn)
> > +{
> > +    return insn == BUG_INSN_32 ||
> > +           (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;
> > +}
> > +
> > +/* Should be used only on Xen code */
> > +static uint32_t read_instr(unsigned long pc)
> > +{
> > +    uint16_t instr16 = *(uint16_t *)pc;
> > +
> > +    ASSERT(is_kernel_text(pc + 1) || is_kernel_inittext(pc + 1));
> > +
> > +    if ( GET_INSN_LENGTH(instr16) == 2 )
> > +        return instr16;
> > +
> > +    ASSERT(is_kernel_text(pc + 3) || is_kernel_inittext(pc + 3));
> > +
> > +    return *(uint32_t *)pc;
> > +}
> 
> Related to the point made further down: If either of these assertions
> fails,
> won't we come back again right here? If either of the
> is_kernel_*text()
> wasn't working quite right, wouldn't we be at risk of entering an
> infinite
> loop (presumably not quite infinite because of the stack overflowing
> at some
> point)?
It is really possible to have infinite loop here so it should be better
to use 'if' with die() or panic().

> 
> >  void do_trap(struct cpu_user_regs *cpu_regs)
> >  {
> > +    register_t pc = cpu_regs->sepc;
> > +    uint32_t instr = read_instr(pc);
> > +
> > +    if ( ( is_valid_bug_insn(instr) ) && ( do_bug_frame(cpu_regs,
> > pc) >= 0 ) )
> 
> No consideration of the kind of exception? I'd expect it is one very
> specific one which the BUG insn would raise, and then there's no
> point
> fetching the insn when it's a different kind of exception.
Good point.

We should have 0x3 ( breakpoint exception ) in scause register. We can
just check that without reading instruction and then also
is_valid_bug_insn could be dropped too.


> 
> Further, nit: Certainly no need for the parentheses on the lhs of the
> &&.
> Having them on the rhs is a matter of taste, so okay, but then the
> blanks immediately inside will want dropping.

> 
> 
> > --- a/xen/common/bug.c
> > +++ b/xen/common/bug.c
> > @@ -1,6 +1,7 @@
> >  #include <xen/bug.h>
> >  #include <xen/errno.h>
> >  #include <xen/kernel.h>
> > +#include <xen/lib.h>
> >  #include <xen/livepatch.h>
> >  #include <xen/string.h>
> >  #include <xen/types.h>
> 
> Unrelated change? Or did you simply forget to mention in the
> description
> why it's needed?
I added it to "Changes in ..." which I forgot to add, but I will add an
explanation to the description. It is better place for it.

<xen/lib.h> is needed to be included for the reason that panic() and
printk() is used in common/bug.c and RISC-V fails if it is not included
with the following errors:
   common/bug.c:69:9: error: implicit declaration of function 'printk'
   [-Werror=implicit-function-declaration]
      69 |         printk("Xen WARN at %s%s:%d\n", prefix, filename,
   lineno);
         |         ^~~~~~
   common/bug.c:77:9: error: implicit declaration of function 'panic'
   [-Werror=implicit-function-declaration]
      77 |         panic("Xen BUG at %s%s:%d\n", prefix, filename,
   lineno);
> 
> ~ Oleksii