Re: [PATCH dovetail v8 11/11] riscv: dovetail: add core support

Hi Florian,

On 6/1/26 10:53, Florian Bezdeka wrote:
> On Sun, 2026-05-31 at 07:53 +0200, Tobias Schaffner wrote:
> 
> [snip]
> 
>> -void do_trap(struct pt_regs *regs, int signo, int code, unsigned long addr)
>> +static void do_trap_raw(struct pt_regs *regs, int signo, int code, unsigned long addr)
> 
> I wonder why this can't just stay do_trap() ...
> 
>>   {
>>   	struct task_struct *tsk = current;
>>   
>> -	if (!mark_trap_entry(regs))
>> -		return;
>> -
> 
> We could call mark_trap_entry() here ...
> 
>>   	if (show_unhandled_signals && unhandled_signal(tsk, signo)
>>   	    && printk_ratelimit()) {
>>   		pr_info("%s[%d]: unhandled signal %d code 0x%x at 0x" REG_FMT,
>> @@ -150,6 +152,14 @@ void do_trap(struct pt_regs *regs, int signo, int code, unsigned long addr)
>>   	}
>>   
>>   	force_sig_fault(signo, code, (void __user *)addr);
> 
> and we add the mark_trap_exit() call here .
> 
>> +}
>> +
>> +void do_trap(struct pt_regs *regs, int signo, int code, unsigned long addr)
>> +{
>> +	if(!mark_trap_entry(regs))
>> +		return;
>> +
>> +	do_trap_raw(regs, signo, code, addr);
>>   
>>   	mark_trap_exit(regs);
>>   }
> 
> That part would get unnecessary.
> 
>> @@ -160,7 +170,7 @@ static void do_trap_error(struct pt_regs *regs, int signo, int code,
>>   	current->thread.bad_cause = regs->cause;
>>   
>>   	if (user_mode(regs)) {
>> -		do_trap(regs, signo, code, addr);
>> +		do_trap_raw(regs, signo, code, addr);
> 
> This is the single caller of do_trap_raw() and I'm quite sure that this
> is not correct as we would not enable hard_irqs when handling a error
> trap over userland...
> 

In this case it is already done. The callers of do_trap_error already 
called mark_trap_entry and we want to make sure we do not call it a 
second time. The do_trap handler with the mark_trap_entry/exit calls is 
used in the handle_page_fault function in mm/fault.c.

But I will try to further simplify the page fault handling. Maybe I can 
get rid of the page fault special treatment.

>>   	} else {
>>   		/*
>>   		 * Dovetail: If we trapped from kernel space, either
>> @@ -177,6 +187,8 @@ static void do_trap_error(struct pt_regs *regs, int signo, int code,
>>   #define DO_ERROR_INFO(name, signo, code, str)	
> 
> That was my final finding. I think I'm done with reviewing.
> 
> Btw: I'm seeing some strange behavior on 7.1-rc5 on x86. The system is
> slower than expected and triggering some test timeouts. Rebase to -rc6
> is already done. Trying to update all the necessary branches now.
> 
> Best regards,
> Florian