On 06.08.2024 12:11, [email protected] 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
