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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.07.2024 14:14, Oleksii wrote:
> 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?

I think so, as long as you retain the assertion in some way, ahead of the
deref of pc.

Jan