Re: [PATCH v12 1/3] xen/riscv: enable GENERIC_BUG_FRAME

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.08.2024 12:11, oleksii.kurochko@xxxxxxxxx wrote:
> On Mon, 2024-08-05 at 17:41 +0200, Jan Beulich wrote:
>> On 02.08.2024 15:54, Oleksii Kurochko wrote:
>>> Enable GENERIC_BUG_FRAME to support BUG(), WARN(), ASSERT,
>>> and run_in_exception_handler().
>>>
>>> The 0x0000 opcode is used for BUG_INSTR, which, when macros from
>>> <xen/bug.h> are used, triggers an exception with the
>>> ILLEGAL_INSTRUCTION cause.
>>> This opcode is encoded as a 2-byte instruction and is invalid if
>>> CONFIG_RISCV_ISA_C is enabled or not.
>>
>> Yes, but there's a caveat: Without the C extension instructions have
>> to be aligned on 32-bit boundaries. You can't just go and insert a
>> 16-bit item there. When RISCV_ISA_C is not set, I think you want to
>> insert two such 16-bit zeroes. Beware of an alignment handling bug
>> in the assembler - don't think of using an alignment directive here.
> Then probably it will be better to define BUG_INSTR as:
>  #define BUG_INSTR "UNIMP"
> and let compiler to provide proper opcode.
> 
> Or define BUG_INSTRT always as 0x00000000 will be better?

I don't know.

>>> --- a/xen/arch/riscv/include/asm/bug.h
>>> +++ b/xen/arch/riscv/include/asm/bug.h
>>> @@ -9,7 +9,11 @@
>>>  
>>>  #ifndef __ASSEMBLY__
>>>  
>>> -#define BUG_INSTR "ebreak"
>>> +#include <xen/stringify.h>
>>> +
>>> +#define BUG_OPCODE  0x0000
>>
>> You don't really use this other than ...
>>
>>> +#define BUG_INSTR ".hword " __stringify(BUG_OPCODE)
>>
>> ... here - does this really warrant a separate #define _and_
>> inclusion of
>> stringify.h?
>>
>> Furthermore you want to avoid using .hword (or any data generating
>> directive), to avoid disturbing disassembly. Please use .insn if at
>> all
>> possible. I understand though that in certain cases you won't be able
>> to
>> use .insn. Yet for the common case (more recent binutils) you'd still
>> better avoid .hword or alike, imo.
>>
>>> @@ -103,7 +104,29 @@ static void do_unexpected_trap(const struct
>>> cpu_user_regs *regs)
>>>  
>>>  void do_trap(struct cpu_user_regs *cpu_regs)
>>>  {
>>> -    do_unexpected_trap(cpu_regs);
>>> +    register_t pc = cpu_regs->sepc;
>>> +    unsigned long cause = csr_read(CSR_SCAUSE);
>>> +
>>> +    switch ( cause )
>>> +    {
>>> +    case CAUSE_ILLEGAL_INSTRUCTION:
>>> +        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);
>>> +
>>> +            break;
>>> +        }
>>> +
>>> +    default:
>>
>> The falling-through here wants annotating, preferably with the
>> pseudo-
>> keyword.
> What kind of pseudo-keyword? I though about /* goto default */ to
> underline that CAUSE_ILLEGAL_INSTRUCTION should be close to "default:".

In compiler.h we define "fallthrough" specifically for purposes like
this.

Jan