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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

"Mancini, Riccardo" <mancio@amazon.com> writes:

> 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)) {

My bad: I completely forgot KVM_PV_REASON_PAGE_READY is actually not
used for interrupt-based delivery:

commit 9ce372b33a2ebbd0b965148879ae169a0015d3f3
Author: Vitaly Kuznetsov <vkuznets@redhat.com>
Date:   Thu May 7 16:36:02 2020 +0200

    KVM: x86: drop KVM_PV_REASON_PAGE_READY case from kvm_handle_page_fault()

and in fact there's nothing else but KVM_PV_REASON_PAGE_NOT_PRESENT in
apf_reason.flags so I agree that this should never happen (but
'kvm_apf_int_enabled' is a good hardening anyway :-)

> +               /* 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());
>

-- 
Vitaly