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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> 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@xxxxxxxxxx> 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());
