linux-kernel - Re: [PATCH] riscv: KVM: Remove unnecessary vcpu kick

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] [thread-next>] [day] [month] [year] [list]
Message-Id: <38cc241c40a8ef2775e304d366bcd07df733ecf0.f7f1d4c7.545f.42a8.90f5.c5d09b1d32ec@feishu.cn>
Date: Thu, 20 Feb 2025 15:12:58 +0800
From: "xiangwencheng" <xiangwencheng@...xincomputing.com>
To: Radim Krčmář <rkrcmar@...tanamicro.com>
Cc: <anup@...infault.org>, <ajones@...tanamicro.com>, 
	<kvm-riscv@...ts.infradead.org>, <kvm@...r.kernel.org>, 
	<linux-riscv@...ts.infradead.org>, <linux-kernel@...r.kernel.org>, 
	<atishp@...shpatra.org>, <paul.walmsley@...ive.com>, 
	<palmer@...belt.com>, <aou@...s.berkeley.edu>, 
	"linux-riscv" <linux-riscv-bounces@...ts.infradead.org>
Subject: Re: [PATCH] riscv: KVM: Remove unnecessary vcpu kick


> From: "Radim Krčmář"<rkrcmar@...tanamicro.com>
> Date:  Wed, Feb 19, 2025, 16:51
> Subject:  Re: [PATCH] riscv: KVM: Remove unnecessary vcpu kick
> To: "BillXiang"<xiangwencheng@...xincomputing.com>, <anup@...infault.org>
> Cc: <ajones@...tanamicro.com>, <kvm-riscv@...ts.infradead.org>, <kvm@...r.kernel.org>, <linux-riscv@...ts.infradead.org>, <linux-kernel@...r.kernel.org>, <atishp@...shpatra.org>, <paul.walmsley@...ive.com>, <palmer@...belt.com>, <aou@...s.berkeley.edu>, "linux-riscv"<linux-riscv-bounces@...ts.infradead.org>
> 2025-02-19T09:54:26+08:00, BillXiang <xiangwencheng@...xincomputing.com>:

> > Thank you Andrew Jones, forgive my errors in the last email.

> > I'm wondering whether it's necessary to kick the virtual hart

> > after writing to the vsfile of IMSIC.

> > From my understanding, writing to the vsfile should directly

> > forward the interrupt as MSI to the virtual hart. This means that

> > an additional kick should not be necessary, as it would cause the

> > vCPU to exit unnecessarily and potentially degrade performance.

> 
> Andrew proposed to avoid the exit overhead, but do a wakeup if the VCPU

> is "sleeping".  I talked with Andrew and thought so as well, but now I

> agree with you that we shouldn't have anything extra here.

> 
> Direct MSIs from IOMMU or other harts won't perform anything afterwards,

> so what you want to do correct and KVM has to properly handle the memory

> write alone.

> 
> > I've tested this behavior in QEMU, and it seems to work perfectly

> > fine without the extra kick.

> 
> If the rest of KVM behaves correctly is a different question.

> A mistake might result in a very rare race condition, so it's better to

> do verification rather than generic testing.

> 
> For example, is `vsfile_cpu >= 0` the right condition for using direct

> interrupts?

> 
> I don't see KVM setting vsfile_cpu to -1 before descheduling after

It's not necessary to set vsfile_cpu to -1 as it doesn't release it, and
the vsfile still belongs to the vCPU after WFI.

> emulating WFI, which could cause a bug as a MSI would never cause a wake

> up.  It might still look like it works, because something else could be

> waking the VCPU up and then the VCPU would notice this MSI as well.

> 
> Please note that I didn't actualy verify the KVM code, so it can be

> correct, I just used this to give you an example of what can go wrong

> without being able to see it in testing.

> 
> I would like to know if KVM needs fixing before this change is accepted.

> (It could make bad things worse.)

As "KVM:  WFI wake-up using IMSIC VS-files" that described in [1], writing to 
VS-FILE will wake up vCPU.

KVM has also handled the situation of WFI. Here is the WFI emulation process:
kvm_riscv_vcpu_exit 
    -> kvm_riscv_vcpu_virtual_insn 
         -> system_opcode_insn 
              -> wfi_insn 
                 -> kvm_riscv_vcpu_wfi
                     -> kvm_vcpu_halt
                         -> kvm_vcpu_block
                             -> kvm_arch_vcpu_blocking
                                   -> kvm_riscv_aia_wakeon_hgei
                                         -> csr_set(CSR_HGEIE, BIT(hgei));
                             -> set_current_state(TASK_INTERRUPTIBLE);
                             -> schedule

In kvm_arch_vcpu_blocking it will enable guest external interrupt, which
means wirting to VS_FILE will cause an interrupt. And the interrupt handler
hgei_interrupt which is setted in aia_hgei_init will finally call kvm_vcpu_kick
to wake up vCPU.

So I still think is not necessary to call another kvm_vcpu_kick after writing to
VS_FILE.

Waiting for more info. Thanks.

[1]  https://kvm-forum.qemu.org/2022/AIA_Virtualization_in_KVM_RISCV_final.pdf

> 
> > Would appreciate any insights or confirmation on this!

> 
> Your patch is not acceptable because of its commit message, though.

> Please look again at the document that Andrew posted and always reply

> the previous thread if you do not send a new patch version.

> 
> The commit message should be on point.

> Please avoid extraneous information that won't help anyone reading the

> commit.  Greeting and commentary can go below the "---" line.

> (And possibly above a "---8<---" line, although that is not official and

>  may cause issues with some maintainers.)

> 
> Thanks.
>