Re: [PATCH v12 1/3] xen/riscv: enable GENERIC_BUG_FRAME

[oleksii.kurochko@xxxxxxxxx]

On Mon, 2024-08-05 at 17:41 +0200, Jan Beulich wrote:
> On 02.08.2024 15:54, Oleksii Kurochko wrote:
> > Enable GENERIC_BUG_FRAME to support BUG(), WARN(), ASSERT,
> > and run_in_exception_handler().
> > 
> > The 0x0000 opcode is used for BUG_INSTR, which, when macros from
> > <xen/bug.h> are used, triggers an exception with the
> > ILLEGAL_INSTRUCTION cause.
> > This opcode is encoded as a 2-byte instruction and is invalid if
> > CONFIG_RISCV_ISA_C is enabled or not.
> 
> Yes, but there's a caveat: Without the C extension instructions have
> to be aligned on 32-bit boundaries. You can't just go and insert a
> 16-bit item there. When RISCV_ISA_C is not set, I think you want to
> insert two such 16-bit zeroes. Beware of an alignment handling bug
> in the assembler - don't think of using an alignment directive here.
Then probably it will be better to define BUG_INSTR as:
 #define BUG_INSTR "UNIMP"
and let compiler to provide proper opcode.

Or define BUG_INSTRT always as 0x00000000 will be better?
> 
> 
> > --- a/xen/arch/riscv/include/asm/bug.h
> > +++ b/xen/arch/riscv/include/asm/bug.h
> > @@ -9,7 +9,11 @@
> >  
> >  #ifndef __ASSEMBLY__
> >  
> > -#define BUG_INSTR "ebreak"
> > +#include <xen/stringify.h>
> > +
> > +#define BUG_OPCODE  0x0000
> 
> You don't really use this other than ...
> 
> > +#define BUG_INSTR ".hword " __stringify(BUG_OPCODE)
> 
> ... here - does this really warrant a separate #define _and_
> inclusion of
> stringify.h?
> 
> Furthermore you want to avoid using .hword (or any data generating
> directive), to avoid disturbing disassembly. Please use .insn if at
> all
> possible. I understand though that in certain cases you won't be able
> to
> use .insn. Yet for the common case (more recent binutils) you'd still
> better avoid .hword or alike, imo.
> 
> > @@ -103,7 +104,29 @@ static void do_unexpected_trap(const struct
> > cpu_user_regs *regs)
> >  
> >  void do_trap(struct cpu_user_regs *cpu_regs)
> >  {
> > -    do_unexpected_trap(cpu_regs);
> > +    register_t pc = cpu_regs->sepc;
> > +    unsigned long cause = csr_read(CSR_SCAUSE);
> > +
> > +    switch ( cause )
> > +    {
> > +    case CAUSE_ILLEGAL_INSTRUCTION:
> > +        if ( do_bug_frame(cpu_regs, pc) >= 0 )
> > +        {
> > +            if ( !(is_kernel_text(pc) || is_kernel_inittext(pc)) )
> > +            {
> > +                printk("Something wrong with PC: %#lx\n", pc);
> > +                die();
> > +            }
> > +
> > +            cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc);
> > +
> > +            break;
> > +        }
> > +
> > +    default:
> 
> The falling-through here wants annotating, preferably with the
> pseudo-
> keyword.
What kind of pseudo-keyword? I though about /* goto default */ to
underline that CAUSE_ILLEGAL_INSTRUCTION should be close to "default:".

~ Oleksii
> > +        do_unexpected_trap(cpu_regs);
> > +        break;
> > +    }
> >  }
>