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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan