On 3/21/25 18:17, Sean Christopherson wrote:
> On Fri, Mar 21, 2025, Tom Lendacky wrote:
>> On 3/18/25 08:47, Tom Lendacky wrote:
>>> On 3/18/25 07:43, Tom Lendacky wrote:
>>>>>> Very off-the-cuff, but I assume KVM_REQ_UPDATE_PROTECTED_GUEST_STATE just needs
>>>>>> to be annotated with KVM_REQUEST_WAIT.
>>>>>
>>>>> Ok, nice. I wasn't sure if KVM_REQUEST_WAIT would be appropriate here.
>>>>> This is much simpler. Let me test it out and resend if everything goes ok.
>>>>
>>>> So that doesn't work. I can still get an occasional #VMEXIT_INVALID. Let
>>>> me try to track down what is happening with this approach...
>>>
>>> Looks like I need to use kvm_make_vcpus_request_mask() instead of just a
>>> plain kvm_make_request() followed by a kvm_vcpu_kick().
>
> Ugh, I was going to say "you don't need to do that", but I forgot that
> kvm_vcpu_kick() subtly doesn't honor KVM_REQUEST_WAIT.
>
> Ooof, I'm 99% certain that's causing bugs elsewhere. E.g. arm64's KVM_REQ_SLEEP
> uses the same "broken" pattern (LOL, which means that of course RISC-V does too).
> In quotes, because kvm_vcpu_kick() is the one that sucks.
>
> I would rather fix that a bit more directly and obviously. IMO, converting to
> smp_call_function_single() isntead of bastardizing smp_send_reschedule() is worth
> doing regardless of the WAIT mess. This will allow cleaning up a bunch of
> make_request+kick pairs, it'll just take a bit of care to make sure we don't
> create a WAIT where one isn't wanted (though those probably should have a big fat
> comment anyways).
>
> Compiled tested only.
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 5de20409bcd9..fd9d9a3ee075 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1505,7 +1505,16 @@ bool kvm_vcpu_block(struct kvm_vcpu *vcpu);
> void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu);
> void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu);
> bool kvm_vcpu_wake_up(struct kvm_vcpu *vcpu);
> -void kvm_vcpu_kick(struct kvm_vcpu *vcpu);
> +
> +#ifndef CONFIG_S390
> +void __kvm_vcpu_kick(struct kvm_vcpu *vcpu, bool wait);
> +
> +static inline void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
> +{
> + __kvm_vcpu_kick(vcpu, false);
> +}
> +#endif
> +
> int kvm_vcpu_yield_to(struct kvm_vcpu *target);
> void kvm_vcpu_on_spin(struct kvm_vcpu *vcpu, bool yield_to_kernel_mode);
>
> @@ -2253,6 +2262,14 @@ static __always_inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
> __kvm_make_request(req, vcpu);
> }
>
> +#ifndef CONFIG_S390
> +static inline void kvm_make_request_and_kick(int req, struct kvm_vcpu *vcpu)
> +{
> + kvm_make_request(req, vcpu);
> + __kvm_vcpu_kick(vcpu, req & KVM_REQUEST_WAIT);
> +}
> +#endif
> +
> static inline bool kvm_request_pending(struct kvm_vcpu *vcpu)
> {
> return READ_ONCE(vcpu->requests);
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 201c14ff476f..2a5120e2e6b4 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3734,7 +3734,7 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_wake_up);
> /*
> * Kick a sleeping VCPU, or a guest VCPU in guest mode, into host kernel mode.
> */
> -void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
> +void __kvm_vcpu_kick(struct kvm_vcpu *vcpu, bool wait)
> {
> int me, cpu;
>
> @@ -3764,12 +3764,12 @@ void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
> if (kvm_arch_vcpu_should_kick(vcpu)) {
> cpu = READ_ONCE(vcpu->cpu);
> if (cpu != me && (unsigned)cpu < nr_cpu_ids && cpu_online(cpu))
> - smp_send_reschedule(cpu);
> + smp_call_function_single(cpu, ack_kick, NULL, wait);
In general, this approach works. However, this change triggered
WARN_ON_ONCE(cpu_online(this_cpu) && irqs_disabled()
&& !oops_in_progress);
in kernel/smp.c.
Call path was:
WARNING: CPU: 13 PID: 3467 at kernel/smp.c:652 smp_call_function_single+0x100/0x120
...
Call Trace:
<TASK>
? show_regs+0x69/0x80
? __warn+0x8d/0x130
? smp_call_function_single+0x100/0x120
? report_bug+0x182/0x190
? handle_bug+0x63/0xa0
? exc_invalid_op+0x19/0x70
? asm_exc_invalid_op+0x1b/0x20
? __pfx_ack_kick+0x10/0x10 [kvm]
? __pfx_ack_kick+0x10/0x10 [kvm]
? smp_call_function_single+0x100/0x120
? srso_alias_return_thunk+0x5/0xfbef5
? migrate_folio_done+0x7f/0x90
__kvm_vcpu_kick+0xa1/0xb0 [kvm]
svm_complete_interrupt_delivery+0x93/0xa0 [kvm_amd]
svm_deliver_interrupt+0x3e/0x50 [kvm_amd]
__apic_accept_irq+0x17f/0x2a0 [kvm]
kvm_irq_delivery_to_apic_fast+0x149/0x1b0 [kvm]
kvm_arch_set_irq_inatomic+0x9b/0xd0 [kvm]
irqfd_wakeup+0xf4/0x230 [kvm]
? __pfx_kvm_set_msi+0x10/0x10 [kvm]
__wake_up_common+0x7b/0xa0
__wake_up_locked_key+0x18/0x20
eventfd_write+0xbe/0x1d0
? srso_alias_return_thunk+0x5/0xfbef5
? security_file_permission+0x134/0x150
vfs_write+0xfb/0x3f0
? srso_alias_return_thunk+0x5/0xfbef5
? __handle_mm_fault+0x930/0x1040
ksys_write+0x6a/0xe0
__x64_sys_write+0x19/0x20
x64_sys_call+0x16af/0x2140
do_syscall_64+0x6b/0x110
? srso_alias_return_thunk+0x5/0xfbef5
? count_memcg_events.constprop.0+0x1e/0x40
? srso_alias_return_thunk+0x5/0xfbef5
? handle_mm_fault+0x18c/0x270
? srso_alias_return_thunk+0x5/0xfbef5
? srso_alias_return_thunk+0x5/0xfbef5
? irqentry_exit_to_user_mode+0x2f/0x170
? srso_alias_return_thunk+0x5/0xfbef5
? irqentry_exit+0x1d/0x30
? srso_alias_return_thunk+0x5/0xfbef5
? exc_page_fault+0x89/0x160
entry_SYSCALL_64_after_hwframe+0x76/0x7e
Thanks,
Tom
> }
> out:
> put_cpu();
> }
> -EXPORT_SYMBOL_GPL(kvm_vcpu_kick);
> +EXPORT_SYMBOL_GPL(__kvm_vcpu_kick);
> #endif /* !CONFIG_S390 */
>
> int kvm_vcpu_yield_to(struct kvm_vcpu *target)
>
