[v3] riscv: skip csr restore if vcpu preempted reload

[PATCH v3] riscv: skip csr restore if vcpu preempted reload

Posted by Jinyu Tang 1 month, 1 week ago

The kvm_arch_vcpu_load() function is called in two cases for riscv:
1. When entering KVM_RUN from userspace ioctl.
2. When a preempted VCPU is scheduled back.

In the second case, if no other KVM VCPU has run on this CPU since the
current VCPU was preempted, the guest CSR (including AIA CSRS and HGTAP) 
values are still valid in the hardware and do not need to be restored.

This patch is to skip the CSR write path when:
1. The VCPU was previously preempted
(vcpu->scheduled_out == 1).
2. It is being reloaded on the same physical CPU
(vcpu->arch.last_exit_cpu == cpu).
3. No other KVM VCPU has used this CPU in the meantime
(vcpu == __this_cpu_read(kvm_former_vcpu)).

This reduces many CSR writes with frequent preemption on the same CPU.

Signed-off-by: Jinyu Tang <tjytimi@163.com>
Reviewed-by: Nutty Liu <nutty.liu@hotmail.com>
---
 v2 -> v3:
 v2 was missing a critical check because I generated the patch from my
 wrong (experimental) branch. This is fixed in v3. Sorry for my trouble.

 v1 -> v2:
 Apply the logic to aia csr load. Thanks for
 Andrew Jones's advice.

 arch/riscv/kvm/vcpu.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
index f001e5640..66bd3ddd5 100644
--- a/arch/riscv/kvm/vcpu.c
+++ b/arch/riscv/kvm/vcpu.c
@@ -25,6 +25,8 @@
 #define CREATE_TRACE_POINTS
 #include "trace.h"
 
+static DEFINE_PER_CPU(struct kvm_vcpu *, kvm_former_vcpu);
+
 const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = {
 	KVM_GENERIC_VCPU_STATS(),
 	STATS_DESC_COUNTER(VCPU, ecall_exit_stat),
@@ -581,6 +583,10 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 	struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
 	struct kvm_vcpu_config *cfg = &vcpu->arch.cfg;
 
+	if (vcpu->scheduled_out && vcpu == __this_cpu_read(kvm_former_vcpu) &&
+		vcpu->arch.last_exit_cpu == cpu)
+		goto csr_restore_done;
+
 	if (kvm_riscv_nacl_sync_csr_available()) {
 		nsh = nacl_shmem();
 		nacl_csr_write(nsh, CSR_VSSTATUS, csr->vsstatus);
@@ -624,6 +630,9 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 
 	kvm_riscv_mmu_update_hgatp(vcpu);
 
+	kvm_riscv_vcpu_aia_load(vcpu, cpu);
+
+csr_restore_done:
 	kvm_riscv_vcpu_timer_restore(vcpu);
 
 	kvm_riscv_vcpu_host_fp_save(&vcpu->arch.host_context);
@@ -633,8 +642,6 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 	kvm_riscv_vcpu_guest_vector_restore(&vcpu->arch.guest_context,
 					    vcpu->arch.isa);
 
-	kvm_riscv_vcpu_aia_load(vcpu, cpu);
-
 	kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
 
 	vcpu->cpu = cpu;
@@ -645,6 +652,8 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 	void *nsh;
 	struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
 
+	__this_cpu_write(kvm_former_vcpu, vcpu);
+
 	vcpu->cpu = -1;
 
 	kvm_riscv_vcpu_aia_put(vcpu);
-- 
2.43.0

Re: [PATCH v3] riscv: skip csr restore if vcpu preempted reload

Posted by Anup Patel 3 weeks, 3 days ago

On Mon, Aug 25, 2025 at 5:44 PM Jinyu Tang <tjytimi@163.com> wrote:
>
> The kvm_arch_vcpu_load() function is called in two cases for riscv:
> 1. When entering KVM_RUN from userspace ioctl.
> 2. When a preempted VCPU is scheduled back.
>
> In the second case, if no other KVM VCPU has run on this CPU since the
> current VCPU was preempted, the guest CSR (including AIA CSRS and HGTAP)
> values are still valid in the hardware and do not need to be restored.
>
> This patch is to skip the CSR write path when:
> 1. The VCPU was previously preempted
> (vcpu->scheduled_out == 1).
> 2. It is being reloaded on the same physical CPU
> (vcpu->arch.last_exit_cpu == cpu).
> 3. No other KVM VCPU has used this CPU in the meantime
> (vcpu == __this_cpu_read(kvm_former_vcpu)).
>
> This reduces many CSR writes with frequent preemption on the same CPU.

Currently, I see the following issues with this patch:

1) It's making Guest usage of IMSIC VS-files on the QEMU virt
    machine very unstable and Guest never boots. It could be
    some QEMU issue but I don't want to increase instability
    on QEMU since it is our primary development vehicle.

2) We have CSRs like hedeleg which can be updated by KVM
    user space via set_guest_debug() ioctl.

The direction of the patch is fine but it is very fragile at the moment.

Regards,
Anup

>
> Signed-off-by: Jinyu Tang <tjytimi@163.com>
> Reviewed-by: Nutty Liu <nutty.liu@hotmail.com>
> ---
>  v2 -> v3:
>  v2 was missing a critical check because I generated the patch from my
>  wrong (experimental) branch. This is fixed in v3. Sorry for my trouble.
>
>  v1 -> v2:
>  Apply the logic to aia csr load. Thanks for
>  Andrew Jones's advice.
>
>  arch/riscv/kvm/vcpu.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> index f001e5640..66bd3ddd5 100644
> --- a/arch/riscv/kvm/vcpu.c
> +++ b/arch/riscv/kvm/vcpu.c
> @@ -25,6 +25,8 @@
>  #define CREATE_TRACE_POINTS
>  #include "trace.h"
>
> +static DEFINE_PER_CPU(struct kvm_vcpu *, kvm_former_vcpu);
> +
>  const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = {
>         KVM_GENERIC_VCPU_STATS(),
>         STATS_DESC_COUNTER(VCPU, ecall_exit_stat),
> @@ -581,6 +583,10 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>         struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
>         struct kvm_vcpu_config *cfg = &vcpu->arch.cfg;
>
> +       if (vcpu->scheduled_out && vcpu == __this_cpu_read(kvm_former_vcpu) &&
> +               vcpu->arch.last_exit_cpu == cpu)
> +               goto csr_restore_done;
> +
>         if (kvm_riscv_nacl_sync_csr_available()) {
>                 nsh = nacl_shmem();
>                 nacl_csr_write(nsh, CSR_VSSTATUS, csr->vsstatus);
> @@ -624,6 +630,9 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>
>         kvm_riscv_mmu_update_hgatp(vcpu);
>
> +       kvm_riscv_vcpu_aia_load(vcpu, cpu);
> +
> +csr_restore_done:
>         kvm_riscv_vcpu_timer_restore(vcpu);
>
>         kvm_riscv_vcpu_host_fp_save(&vcpu->arch.host_context);
> @@ -633,8 +642,6 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>         kvm_riscv_vcpu_guest_vector_restore(&vcpu->arch.guest_context,
>                                             vcpu->arch.isa);
>
> -       kvm_riscv_vcpu_aia_load(vcpu, cpu);
> -
>         kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
>
>         vcpu->cpu = cpu;
> @@ -645,6 +652,8 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>         void *nsh;
>         struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
>
> +       __this_cpu_write(kvm_former_vcpu, vcpu);
> +
>         vcpu->cpu = -1;
>
>         kvm_riscv_vcpu_aia_put(vcpu);
> --
> 2.43.0
>

Re:Re: [PATCH v3] riscv: skip csr restore if vcpu preempted reload

Posted by Jinyu Tang 2 weeks, 4 days ago

At 2025-09-09 14:49:43,"Anup Patel" <anup@brainfault.org>, said: 
>On Mon, Aug 25, 2025 at 5:44 PM Jinyu Tang <tjytimi@163.com> wrote:
>>
>> The kvm_arch_vcpu_load() function is called in two cases for riscv:
>> 1. When entering KVM_RUN from userspace ioctl.
>> 2. When a preempted VCPU is scheduled back.
>>
>> In the second case, if no other KVM VCPU has run on this CPU since the
>> current VCPU was preempted, the guest CSR (including AIA CSRS and HGTAP)
>> values are still valid in the hardware and do not need to be restored.
>>
>> This patch is to skip the CSR write path when:
>> 1. The VCPU was previously preempted
>> (vcpu->scheduled_out == 1).
>> 2. It is being reloaded on the same physical CPU
>> (vcpu->arch.last_exit_cpu == cpu).
>> 3. No other KVM VCPU has used this CPU in the meantime
>> (vcpu == __this_cpu_read(kvm_former_vcpu)).
>>
>> This reduces many CSR writes with frequent preemption on the same CPU.
>
>Currently, I see the following issues with this patch:
>
>1) It's making Guest usage of IMSIC VS-files on the QEMU virt
>    machine very unstable and Guest never boots. It could be
>    some QEMU issue but I don't want to increase instability
>    on QEMU since it is our primary development vehicle.
>
>2) We have CSRs like hedeleg which can be updated by KVM
>    user space via set_guest_debug() ioctl.
>
>The direction of the patch is fine but it is very fragile at the moment.
>
>Regards,
>Anup
>
Ok, I will find out why it makes guest never boot when using IMSIC VS-files.
Thanks,
Jinyu
>>
>> Signed-off-by: Jinyu Tang <tjytimi@163.com>
>> Reviewed-by: Nutty Liu <nutty.liu@hotmail.com>
>> ---
>>  v2 -> v3:
>>  v2 was missing a critical check because I generated the patch from my
>>  wrong (experimental) branch. This is fixed in v3. Sorry for my trouble.
>>
>>  v1 -> v2:
>>  Apply the logic to aia csr load. Thanks for
>>  Andrew Jones's advice.
>>
>>  arch/riscv/kvm/vcpu.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
>> index f001e5640..66bd3ddd5 100644
>> --- a/arch/riscv/kvm/vcpu.c
>> +++ b/arch/riscv/kvm/vcpu.c
>> @@ -25,6 +25,8 @@
>>  #define CREATE_TRACE_POINTS
>>  #include "trace.h"
>>
>> +static DEFINE_PER_CPU(struct kvm_vcpu *, kvm_former_vcpu);
>> +
>>  const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = {
>>         KVM_GENERIC_VCPU_STATS(),
>>         STATS_DESC_COUNTER(VCPU, ecall_exit_stat),
>> @@ -581,6 +583,10 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>         struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
>>         struct kvm_vcpu_config *cfg = &vcpu->arch.cfg;
>>
>> +       if (vcpu->scheduled_out && vcpu == __this_cpu_read(kvm_former_vcpu) &&
>> +               vcpu->arch.last_exit_cpu == cpu)
>> +               goto csr_restore_done;
>> +
>>         if (kvm_riscv_nacl_sync_csr_available()) {
>>                 nsh = nacl_shmem();
>>                 nacl_csr_write(nsh, CSR_VSSTATUS, csr->vsstatus);
>> @@ -624,6 +630,9 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>
>>         kvm_riscv_mmu_update_hgatp(vcpu);
>>
>> +       kvm_riscv_vcpu_aia_load(vcpu, cpu);
>> +
>> +csr_restore_done:
>>         kvm_riscv_vcpu_timer_restore(vcpu);
>>
>>         kvm_riscv_vcpu_host_fp_save(&vcpu->arch.host_context);
>> @@ -633,8 +642,6 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>         kvm_riscv_vcpu_guest_vector_restore(&vcpu->arch.guest_context,
>>                                             vcpu->arch.isa);
>>
>> -       kvm_riscv_vcpu_aia_load(vcpu, cpu);
>> -
>>         kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
>>
>>         vcpu->cpu = cpu;
>> @@ -645,6 +652,8 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>>         void *nsh;
>>         struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
>>
>> +       __this_cpu_write(kvm_former_vcpu, vcpu);
>> +
>>         vcpu->cpu = -1;
>>
>>         kvm_riscv_vcpu_aia_put(vcpu);
>> --
>> 2.43.0
>>

Re:Re: [PATCH v3] riscv: skip csr restore if vcpu preempted reload

Posted by Jinyu Tang 2 weeks, 4 days ago

Re: [PATCH v3] riscv: skip csr restore if vcpu preempted reload

Posted by Andrew Jones 1 month, 1 week ago

On Mon, Aug 25, 2025 at 08:14:11PM +0800, Jinyu Tang wrote:
> The kvm_arch_vcpu_load() function is called in two cases for riscv:
> 1. When entering KVM_RUN from userspace ioctl.
> 2. When a preempted VCPU is scheduled back.
> 
> In the second case, if no other KVM VCPU has run on this CPU since the
> current VCPU was preempted, the guest CSR (including AIA CSRS and HGTAP) 
> values are still valid in the hardware and do not need to be restored.
> 
> This patch is to skip the CSR write path when:
> 1. The VCPU was previously preempted
> (vcpu->scheduled_out == 1).
> 2. It is being reloaded on the same physical CPU
> (vcpu->arch.last_exit_cpu == cpu).
> 3. No other KVM VCPU has used this CPU in the meantime
> (vcpu == __this_cpu_read(kvm_former_vcpu)).
> 
> This reduces many CSR writes with frequent preemption on the same CPU.
> 
> Signed-off-by: Jinyu Tang <tjytimi@163.com>
> Reviewed-by: Nutty Liu <nutty.liu@hotmail.com>
> ---
>  v2 -> v3:
>  v2 was missing a critical check because I generated the patch from my
>  wrong (experimental) branch. This is fixed in v3. Sorry for my trouble.
> 
>  v1 -> v2:
>  Apply the logic to aia csr load. Thanks for
>  Andrew Jones's advice.
> 
>  arch/riscv/kvm/vcpu.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>