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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250915115333.1344626-1-tjytimi@163.com>
Date: Mon, 15 Sep 2025 19:53:33 +0800
From: Jinyu Tang <tjytimi@....com>
To: anup@...infault.org
Cc: ajones@...tanamicro.com,
	atish.patra@...ux.dev,
	conor.dooley@...rochip.com,
	kvm-riscv@...ts.infradead.org,
	kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	linux-riscv@...ts.infradead.org,
	nutty.liu@...mail.com,
	paul.walmsley@...ive.com,
	tjytimi@....com,
	yongxuan.wang@...ive.com
Subject: Re:Re: [PATCH v3] riscv: skip csr restore if vcpu preempted reload

At 2025-09-09 14:49:43,"Anup Patel" <anup@...infault.org>, said: 
>On Mon, Aug 25, 2025 at 5:44 PM Jinyu Tang <tjytimi@....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@....com>
>> Reviewed-by: Nutty Liu <nutty.liu@...mail.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
>>