Re: [PATCH 2/3] lib/riscv: clear SDT when entering exception handling

From: "Clément Léger" <cleger@rivosinc.com>
To: Andrew Jones <andrew.jones@linux.dev>
Cc: kvm@vger.kernel.org, kvm-riscv@lists.infradead.org,
	Andrew Jones <ajones@ventanamicro.com>,
	Ved Shanbhogue <ved@rivosinc.com>
Subject: Re: [PATCH 2/3] lib/riscv: clear SDT when entering exception handling
Date: Tue, 3 Jun 2025 10:22:07 +0200	[thread overview]
Message-ID: <bcb3cffe-5fca-45fc-aa30-c167f6cf3c5c@rivosinc.com> (raw)
In-Reply-To: <20250603-8e4c2aa217e6be3b3ee43972@orel>

On 03/06/2025 10:16, Andrew Jones wrote:
> On Fri, May 23, 2025 at 09:53:09AM +0200, Clément Léger wrote:
>> In order to avoid taking double trap once we have entered a trap and
>> saved everything, clear SDT at the end of entry. This is not exactly
>> required when double trap is disabled (probably most of the time), but
>> that's not harmful.
> 
> Hmm... I wonder if this shouldn't be left to the handlers. Maybe
> we should just provide a couple helpers in processor.h, such as
> 
> local_dlbtrp_enable()
> local_dlbtrp_disable()

Hi Drew,

Yeah indeed, that makes sense, some of them might have to save more info
before re-enabling trap in a near future.

I'll drop this patch and use that proposal.

> 
> If we do need to manage this at save_context time, then I have
> a couple comments below
> 
>>
>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>> ---
>>  riscv/cstart.S | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/riscv/cstart.S b/riscv/cstart.S
>> index 575f929b..a86f97f0 100644
>> --- a/riscv/cstart.S
>> +++ b/riscv/cstart.S
>> @@ -212,14 +212,15 @@ secondary_entry:
>>  	REG_S	t6, PT_T6(a0)			// x31
>>  	csrr	a1, CSR_SEPC
>>  	REG_S	a1, PT_EPC(a0)
>> -	csrr	a1, CSR_SSTATUS
>> -	REG_S	a1, PT_STATUS(a0)
>>  	csrr	a1, CSR_STVAL
>>  	REG_S	a1, PT_BADADDR(a0)
>>  	csrr	a1, CSR_SCAUSE
>>  	REG_S	a1, PT_CAUSE(a0)
>>  	REG_L	a1, PT_ORIG_A0(a0)
>>  	REG_S	a1, PT_A0(a0)
>> +	li t0, 	SR_SDT
>           ^    ^ should not be a tab
>           ^ should be tabs
> 
> SR_SDT isn't defined until the next patch so this breaks compiling at this
> point, which could break bisection. You can do a quick check of a series
> for this with
> 
>  git rebase -i -x 'make' <base>

Missed that, sorry, I'll check next time.

Thanks,

Clément

> 
>> +	csrrc 	a1, CSR_SSTATUS, t0
>> +	REG_S	a1, PT_STATUS(a0)
>>  .endm
>>  
>>  /*
>> @@ -227,6 +228,8 @@ secondary_entry:
>>   * Also restores a0.
>>   */
>>  .macro restore_context
>> +	REG_L	a1, PT_STATUS(a0)
>> +	csrw	CSR_SSTATUS, a1
>>  	REG_L	ra, PT_RA(a0)			// x1
>>  	REG_L	sp, PT_SP(a0)			// x2
>>  	REG_L	gp, PT_GP(a0)			// x3
>> @@ -260,8 +263,6 @@ secondary_entry:
>>  	REG_L	t6, PT_T6(a0)			// x31
>>  	REG_L	a1, PT_EPC(a0)
>>  	csrw	CSR_SEPC, a1
>> -	REG_L	a1, PT_STATUS(a0)
>> -	csrw	CSR_SSTATUS, a1
>>  	REG_L	a1, PT_BADADDR(a0)
>>  	csrw	CSR_STVAL, a1
>>  	REG_L	a1, PT_CAUSE(a0)
>> -- 
>> 2.49.0
>>
> 
> Thanks,
> drew

-- 
kvm-riscv mailing list
kvm-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kvm-riscv