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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan