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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2024-07-11 at 11:25 +0200, Jan Beulich wrote:
> On 11.07.2024 10:50, Oleksii wrote:
> > On Wed, 2024-07-10 at 12:01 +0200, Jan Beulich wrote:
> > > On 02.07.2024 13:23, Oleksii Kurochko wrote:
> > > > @@ -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.
> 
> Just that then you'll also lose the is_kernel_*text() checking, which
> I
> understand is there to remind you/us that one this becomes reachable
> from non-Xen code, adjustments are going to be needed.
One thing I wrote incorrectly is that we still need fetch instruction
or at least 16 bits to identify the length of instruction to set proper
sepc:
    cpu_regs->sepc += GET_INSN_LENGTH(instr);

We could write that in the following way:
    cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc);
Would it be okay?

~ Oleksii