> From: "Radim Krčmář"<rkrcmar@xxxxxxxxxxxxxxxx> > Date: Wed, Feb 19, 2025, 16:51 > Subject: Re: [PATCH] riscv: KVM: Remove unnecessary vcpu kick > To: "BillXiang"<xiangwencheng@xxxxxxxxxxxxxxxxxxx>, <anup@xxxxxxxxxxxxxx> > Cc: <ajones@xxxxxxxxxxxxxxxx>, <kvm-riscv@xxxxxxxxxxxxxxxxxxx>, <kvm@xxxxxxxxxxxxxxx>, <linux-riscv@xxxxxxxxxxxxxxxxxxx>, <linux-kernel@xxxxxxxxxxxxxxx>, <atishp@xxxxxxxxxxxxxx>, <paul.walmsley@xxxxxxxxxx>, <palmer@xxxxxxxxxxx>, <aou@xxxxxxxxxxxxxxxxx>, "linux-riscv"<linux-riscv-bounces@xxxxxxxxxxxxxxxxxxx> > 2025-02-19T09:54:26+08:00, BillXiang <xiangwencheng@xxxxxxxxxxxxxxxxxxx>: > > 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. >
