RE: [RFC PATCH 4.14] KVM: x86: Backport support for interrupt-based APF page-ready delivery in guest

["Mancini, Riccardo" <mancio@amazon.com>]

Hey,

Thank you both for the quick feedback.

> > I've backported the guest-side of the patchset to 4.14.326, could you 
> > help us and take a look at the backport?
> > I only backported the original patchset, I'm not sure if there's any 
> > other patch (bug fix) that needs to be included in the backpotrt.
>
> I remember us fixing PV feature enablement/disablement for hibernation/kdump later, see e.g.
>
> commit 8b79feffeca28c5459458fe78676b081e87c93a4
> Author: Vitaly Kuznetsov <vkuznets@redhat.com>
> Date:   Wed Apr 14 14:35:41 2021 +0200
>
>     x86/kvm: Teardown PV features on boot CPU as well
>
> commit 3d6b84132d2a57b5a74100f6923a8feb679ac2ce
> Author: Vitaly Kuznetsov <vkuznets@redhat.com>
> Date:   Wed Apr 14 14:35:43 2021 +0200
>
>     x86/kvm: Disable all PV features on crash
>
> if you're interested in such use-cases. I don't recall any required fixes for normal operation.

These look like issues already present in 4.14, not introduced by the
interrupt-based mechanism, correct?
If so, I wouldn't chase them.
Furthermore, I don't even think we hit those use cases in our scenario.

> 
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > On 10/16/23 16:18, Vitaly Kuznetsov wrote:
> >> In case keeping legacy mechanism is a must, I would suggest you
> >> somehow record the fact that the guest has opted for interrupt-based
> >> delivery (e.g. set a global variable or use a static key) and
> >> short-circuit
> >> do_async_page_fault() to immediately return and not do anything in
> >> this case.
> >
> > I guess you mean "not do anything for KVM_PV_REASON_PAGE_READY in this
> > case"?
> 
> Yes, of course: KVM_PV_REASON_PAGE_NOT_PRESENT is always a #PF.

I agree this is a difference with the upstream asyncpf-int implementation and
it's theoretically incorrect. I think this shouldn't happen in a normal case, 
but it's better to keep it consistent.
I'll add a check that asyncpf-int is _not_ enabled before processing 
KVM_PV_REASON_PAGE_READY. Draft diff below.

Thanks,
Riccardo

diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 582a366b82d8..bdfdffd35939 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -79,6 +79,8 @@ static DEFINE_PER_CPU(struct kvm_vcpu_pv_apf_data, apf_reason) __aligned(64);
 static DEFINE_PER_CPU(struct kvm_steal_time, steal_time) __aligned(64);
 static int has_steal_clock = 0;
 
+static DEFINE_PER_CPU(u32, kvm_apf_int_enabled);
+
 /*
  * No need for any "IO delay" on KVM
  */
@@ -277,7 +279,8 @@ do_async_page_fault(struct pt_regs *regs, unsigned long error_code)
                prev_state = exception_enter();
                kvm_async_pf_task_wait((u32)read_cr2(), !user_mode(regs));
                exception_exit(prev_state);
-       } else if (reason & KVM_PV_REASON_PAGE_READY) {
+       } else if (!__this_cpu_read(kvm_apf_int_enabled) && (reason & KVM_PV_REASON_PAGE_READY)) {
+               /* this event is only possible if interrupt-based mechanism is disabled */
                rcu_irq_enter();
                kvm_async_pf_task_wake((u32)read_cr2());
                rcu_irq_exit();
@@ -367,6 +370,7 @@ static void kvm_guest_cpu_init(void)
                if (kvm_para_has_feature(KVM_FEATURE_ASYNC_PF_INT)) {
                        pa |= KVM_ASYNC_PF_DELIVERY_AS_INT;
                        wrmsrl(MSR_KVM_ASYNC_PF_INT, HYPERVISOR_CALLBACK_VECTOR);
+                       __this_cpu_write(kvm_apf_int_enabled, 1);
                }
 
                wrmsrl(MSR_KVM_ASYNC_PF_EN, pa);
@@ -396,6 +400,7 @@ static void kvm_pv_disable_apf(void)
 
        wrmsrl(MSR_KVM_ASYNC_PF_EN, 0);
        __this_cpu_write(apf_reason.enabled, 0);
+       __this_cpu_write(kvm_apf_int_enabled, 0);
 
        printk(KERN_INFO"Unregister pv shared memory for cpu %d\n",
               smp_processor_id());