Re: [PATCH 1/3] KVM: riscv: selftests: Add stval to exception handling

From: Atish Patra <atish.patra@linux.dev>
To: Andrew Jones <ajones@ventanamicro.com>
Cc: Anup Patel <anup@brainfault.org>,
	Atish Patra <atishp@atishpatra.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Shuah Khan <shuah@kernel.org>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Alexandre Ghiti <alex@ghiti.fr>,
	kvm@vger.kernel.org, kvm-riscv@lists.infradead.org,
	linux-riscv@lists.infradead.org, linux-kselftest@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] KVM: riscv: selftests: Add stval to exception handling
Date: Mon, 28 Apr 2025 15:47:47 -0700	[thread overview]
Message-ID: <3b05d1a0-c873-4e61-b98c-b725d0dbb350@linux.dev> (raw)
In-Reply-To: <20250425-dc44cb547ab5e2f994c94e80@orel>

On 4/25/25 6:50 AM, Andrew Jones wrote:
> On Mon, Mar 24, 2025 at 05:40:29PM -0700, Atish Patra wrote:
>> Save stval during exception handling so that it can be decoded to
>> figure out the details of exception type.
>>
>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>> ---
>>   tools/testing/selftests/kvm/include/riscv/processor.h | 1 +
>>   tools/testing/selftests/kvm/lib/riscv/handlers.S      | 2 ++
>>   2 files changed, 3 insertions(+)
>>
>> diff --git a/tools/testing/selftests/kvm/include/riscv/processor.h b/tools/testing/selftests/kvm/include/riscv/processor.h
>> index 5f389166338c..f4a7d64fbe9a 100644
>> --- a/tools/testing/selftests/kvm/include/riscv/processor.h
>> +++ b/tools/testing/selftests/kvm/include/riscv/processor.h
>> @@ -95,6 +95,7 @@ struct ex_regs {
>>   	unsigned long epc;
>>   	unsigned long status;
>>   	unsigned long cause;
>> +	unsigned long stval;
>>   };
>>   
>>   #define NR_VECTORS  2
>> diff --git a/tools/testing/selftests/kvm/lib/riscv/handlers.S b/tools/testing/selftests/kvm/lib/riscv/handlers.S
>> index aa0abd3f35bb..2884c1e8939b 100644
>> --- a/tools/testing/selftests/kvm/lib/riscv/handlers.S
>> +++ b/tools/testing/selftests/kvm/lib/riscv/handlers.S
>> @@ -45,9 +45,11 @@
>>   	csrr  s0, CSR_SEPC
>>   	csrr  s1, CSR_SSTATUS
>>   	csrr  s2, CSR_SCAUSE
>> +	csrr  s3, CSR_STVAL
>>   	sd    s0, 248(sp)
>>   	sd    s1, 256(sp)
>>   	sd    s2, 264(sp)
>> +	sd    s3, 272(sp)
> We can't add stval without also changing how much stack we allocate at the
> top of this macro, but since we need to keep sp 16-byte aligned in order
> to call C code (route_exception()) we'll need to decrement -8*36, not

Yes. Thanks for catching that.

> -8*35. Or, we could just switch struct ex_regs to be the kernel's struct
> pt_regs which has 36 unsigned longs. The 'badaddr' member is for stval and
> the additional long is orig_a0.

I think switching to pt_regs is better in terms of maintainability in 
the future.
I will do that.

>>   .endm
>>   
>>   .macro restore_context
> I guess we should restore stval too.

Do we ?  stval is written by hardware and doesn't contain any state of 
the interrupted program.
Once, the trap handler processes the trap using stval information, there 
is no need to restore it.

Am I missing something ?

> Thanks,
> drew
>
>> -- 
>> 2.43.0
>>
>>
>> -- 
>> kvm-riscv mailing list
>> kvm-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/kvm-riscv