[PATCH] riscv: KVM: Remove unnecessary vcpu kick

[xiangwencheng]

> From: "Radim Krčmář"<rkrcmar at ventanamicro.com>
> Date:  Wed, Feb 19, 2025, 16:51
> Subject:  Re: [PATCH] riscv: KVM: Remove unnecessary vcpu kick
> To: "BillXiang"<xiangwencheng at lanxincomputing.com>, <anup at brainfault.org>
> Cc: <ajones at ventanamicro.com>, <kvm-riscv at lists.infradead.org>, <kvm at vger.kernel.org>, <linux-riscv at lists.infradead.org>, <linux-kernel at vger.kernel.org>, <atishp at atishpatra.org>, <paul.walmsley at sifive.com>, <palmer at dabbelt.com>, <aou at eecs.berkeley.edu>, "linux-riscv"<linux-riscv-bounces at lists.infradead.org>
> 2025-02-19T09:54:26+08:00, BillXiang <xiangwencheng at lanxincomputing.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.
>