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

[oleksii.kurochko@xxxxxxxxx]

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();
        }
```

Probably, to have the message is enough ( "this is not Xen's ebreak\n"
):
```
        if ( do_bug_frame(cpu_regs, pc) >= 0 )
        {
                ...
        }
        else
            printk("this is not Xen's ebreak\n");

        cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc);
```

> 
> > And if QEMU is being used, GDB isn't actually inserting ebreak
> > instructions because QEMU has an infinite amount of "hardware"
> > breakpoints.
> > 
> > If it answers your original question I could add this to commit
> > message/code in the next patch version.
> 
> I'm afraid it still doesn't.
So if we are speaking about the case QEMU + GDB then no ebreak
instruction will be inserted. So what happens is:
1. QEMU notices PC hits breakpoint ( "hardware" breakpoints are used +
GDB keeps track of what addresses it has breakpoints at )
2. QEMU tells GDB that the program received SIGTRAP
3. GDB does debugger tasks.

In case of real hardware it will be JTAG and OpenOCD to interface with
it and GDB will use OpenOCD for debugging purpose which follows RISC-V
debug spec and everything will work as I described in the commit
message/code.

Or it could be still without external h/w debugger ( using ebreak
instruction ) but then hypervisor should have an infrastrucutre to
notify GDB that a breakpoint is hited ( something like KGDB in Linux
kernel ) and for this case we will print the message mentioned above (
+ stop Xen if it is necessary )

~ Oleksii