"Mancini, Riccardo" <mancio@xxxxxxxxxx> 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@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)) {
My bad: I completely forgot KVM_PV_REASON_PAGE_READY is actually not
used for interrupt-based delivery:
commit 9ce372b33a2ebbd0b965148879ae169a0015d3f3
Author: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
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
