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

[oleksii.kurochko@xxxxxxxxx]

On Fri, 2024-08-02 at 12:30 +0200, Jan Beulich wrote:
> On 02.08.2024 12:22, oleksii.kurochko@xxxxxxxxx wrote:
> > On Fri, 2024-08-02 at 11:21 +0200, Jan Beulich wrote:
> > > On 02.08.2024 11:14, oleksii.kurochko@xxxxxxxxx wrote:
> > > > On Mon, 2024-07-29 at 15:00 +0200, Jan Beulich wrote:
> > > > > > To have working BUG(), WARN(), ASSERT,
> > > > > > run_in_exception_handler()
> > > > > > it is needed to enable GENERIC_BUG_FRAME.
> > > > > > 
> > > > > > ebreak is used as BUG_insn so when macros from <xen/bug.h>
> > > > > > are
> > > > > > used, an exception with BREAKPOINT cause will be generated.
> > > > > > 
> > > > > > ebreak triggers a debug trap, which starts in debug mode
> > > > > > and is
> > > > > > then filtered by every mode. A debugger will first handle
> > > > > > the
> > > > > > debug trap and check if ebreak was set by it or not. If
> > > > > > not,
> > > > > > CAUSE_BREAKPOINT will be generated for the mode where the
> > > > > > ebreak
> > > > > > instruction was executed.
> > > > > 
> > > > > Here and in the similar code comment you talk about an
> > > > > external
> > > > > debugger, requiring debug mode, which is an optional feature.
> > > > > In
> > > > > my earlier comments what I was referring to was pure software
> > > > > debugging. I continue to fail to see how properly
> > > > > distinguishing
> > > > > ebreak uses for breakpoints from ebreak uses for e.g. BUG()
> > > > > and
> > > > > WARN() is going to be feasible.
> > > > GDB keeps track of what addresses it has breakpoints at.
> > > 
> > > Of course it does. But in Xen how do you decide whether to
> > > trigger
> > > the debugger when you've hit an ebreak? (Just to repeat, my
> > > question
> > > is about the purely software debugging case; no hardware
> > > debugging
> > > extensions. In such a case, aiui, Xen gains control first and
> > > then
> > > decides whether to trigger the debugger, or whether to handle the
> > > exception internally. Sure, none of this infrastructure is in
> > > place
> > > right now, but imo it wants taking into consideration.)
> > Well, then something like KGDB is needed for Xen and mechanism to
> > notify guests to something similar to:
> > 
> > Right now Xen detects that 'ebreak' is inserted by using the
> > function
> > do_bug_frame():
> > ```
> >     case CAUSE_BREAKPOINT:
> >         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);
> >         }
> > ```
> > 
> > So if do_bug_frame() return < 0 then it should be ebreak inserted
> > by
> > the debugger so need to notify GDB that he should handle that.
> > At the moment I think we can add():
> > ```
> >         if ( do_bug_frame(cpu_regs, pc) >= 0 )
> >         {
> >             ...
> > 
> >             cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc);
> >         }
> >     else
> >     {
> >         printk("this is not Xen's ebreak\n");
> >             die();
> >         }
> > ```
> 
> Except that, as previously said, this will break if the debugger
> inserted
> a breakpoint where Xen already has an ebreak. That's where we started
> from
> in this discussion. Recall that my question was how the use of ebreak
> can
> be correct / reliable here. It is for a reason that on x86 we do not
> use
> the shorter INT3 instruction, but the longer UD2 one for BUG() etc.
Okay, in this case it will be an issue but only in case if the debugger
really will insert ebreak instruction but not something else. I wasn't
able to find a case when it is really inserted ebreak in case of RISC-
V.

However I agree that it would be safer to use something like UD2 but
RISC-V doesn't have such instruction ( at least I don't see such in a
spec).

There is something what we need is mentioned here:
https://github.com/riscv-non-isa/riscv-asm-manual/blob/main/riscv-asm.md#instruction-aliases
But the problem is that is "de-facto" standard, but not really standard
instruction. Anyway, I will update the patch to use (C0001073) instead
of ebreak.

~ Oleksii