| Message ID | 20200525144125.143875-11-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Interrupt-based mechanism for async_pf 'page present' notifications | expand |
On 25/05/20 16:41, Vitaly Kuznetsov wrote: > Concerns were expressed around APF events delivery when CPU is not > in user mode (KVM_ASYNC_PF_SEND_ALWAYS), e.g. > https://lore.kernel.org/kvm/ed71d0967113a35f670a9625a058b8e6e0b2f104.1583547991.git.luto@kernel.org/ > > 'Page ready' events are already free from '#PF abuse' but 'page not ready' > notifications still go through #PF (to be changed in the future). Make the > probability of running into issues when APF collides with regular #PF lower > by deprecating KVM_ASYNC_PF_SEND_ALWAYS. The feature doesn't seem to be > important enough for any particular workload to notice the difference. This has been disabled already in guest code, but I don't see a particular reason to deprecate it in the host too. Supporting it on the host is just one line of code; if it's a problem *for the guest*, you just don't use KVM_ASYNC_PF_SEND_ALWAYS. Also, note that #VE will always be delivered to the guest even at CPL0; the decision on whether to do sync or async page fault at CPL0 will move to the guest, but enabling #VE will probably _require_ the KVM_ASYNC_PF_SEND_ALWAYS bit to be set (and KVM_ASYNC_PF_DELIVERY_AS_INT as well). Thanks, Paolo > Suggested-by: Andy Lutomirski <luto@kernel.org> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > arch/x86/include/asm/kvm_host.h | 1 - > arch/x86/include/uapi/asm/kvm_para.h | 2 +- > arch/x86/kernel/kvm.c | 3 --- > arch/x86/kvm/x86.c | 13 +++++++++---- > 4 files changed, 10 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 356c02bfa587..f491214b7667 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -769,7 +769,6 @@ struct kvm_vcpu_arch { > u64 msr_int_val; /* MSR_KVM_ASYNC_PF_INT */ > u16 vec; > u32 id; > - bool send_user_only; > u32 host_apf_flags; > unsigned long nested_apf_token; > bool delivery_as_pf_vmexit; > diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h > index 812e9b4c1114..3cae0faac2b8 100644 > --- a/arch/x86/include/uapi/asm/kvm_para.h > +++ b/arch/x86/include/uapi/asm/kvm_para.h > @@ -82,7 +82,7 @@ struct kvm_clock_pairing { > #define KVM_MAX_MMU_OP_BATCH 32 > > #define KVM_ASYNC_PF_ENABLED (1 << 0) > -#define KVM_ASYNC_PF_SEND_ALWAYS (1 << 1) > +#define KVM_ASYNC_PF_SEND_ALWAYS (1 << 1) /* deprecated */ > #define KVM_ASYNC_PF_DELIVERY_AS_PF_VMEXIT (1 << 2) > #define KVM_ASYNC_PF_DELIVERY_AS_INT (1 << 3) > > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c > index 79730eaef1e1..add123302122 100644 > --- a/arch/x86/kernel/kvm.c > +++ b/arch/x86/kernel/kvm.c > @@ -324,9 +324,6 @@ static void kvm_guest_cpu_init(void) > if (kvm_para_has_feature(KVM_FEATURE_ASYNC_PF_INT) && kvmapf) { > u64 pa = slow_virt_to_phys(this_cpu_ptr(&apf_reason)); > > -#ifdef CONFIG_PREEMPTION > - pa |= KVM_ASYNC_PF_SEND_ALWAYS; > -#endif > pa |= KVM_ASYNC_PF_ENABLED | KVM_ASYNC_PF_DELIVERY_AS_INT; > > if (kvm_para_has_feature(KVM_FEATURE_ASYNC_PF_VMEXIT)) > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index cc1bf6cfc5e0..8222133bf5be 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -2648,7 +2648,10 @@ static int kvm_pv_enable_async_pf(struct kvm_vcpu *vcpu, u64 data) > { > gpa_t gpa = data & ~0x3f; > > - /* Bits 4:5 are reserved, Should be zero */ > + /* > + * Bits 4:5 are reserved and should be zero. Bit 1 > + * (KVM_ASYNC_PF_SEND_ALWAYS) was deprecated and is being ignored. > + */ > if (data & 0x30) > return 1; > > @@ -2664,7 +2667,6 @@ static int kvm_pv_enable_async_pf(struct kvm_vcpu *vcpu, u64 data) > sizeof(u64))) > return 1; > > - vcpu->arch.apf.send_user_only = !(data & KVM_ASYNC_PF_SEND_ALWAYS); > vcpu->arch.apf.delivery_as_pf_vmexit = data & KVM_ASYNC_PF_DELIVERY_AS_PF_VMEXIT; > > kvm_async_pf_wakeup_all(vcpu); > @@ -10433,8 +10435,11 @@ static bool kvm_can_deliver_async_pf(struct kvm_vcpu *vcpu) > if (!vcpu->arch.apf.delivery_as_pf_vmexit && is_guest_mode(vcpu)) > return false; > > - if (!kvm_pv_async_pf_enabled(vcpu) || > - (vcpu->arch.apf.send_user_only && kvm_x86_ops.get_cpl(vcpu) == 0)) > + /* > + * 'Page not present' APF events are only delivered when CPU is in > + * user mode and APF mechanism is enabled. > + */ > + if (!kvm_pv_async_pf_enabled(vcpu) || kvm_x86_ops.get_cpl(vcpu) == 0) > return false; > > return true; >
Paolo Bonzini <pbonzini@redhat.com> writes: > On 25/05/20 16:41, Vitaly Kuznetsov wrote: >> Concerns were expressed around APF events delivery when CPU is not >> in user mode (KVM_ASYNC_PF_SEND_ALWAYS), e.g. >> https://lore.kernel.org/kvm/ed71d0967113a35f670a9625a058b8e6e0b2f104.1583547991.git.luto@kernel.org/ >> >> 'Page ready' events are already free from '#PF abuse' but 'page not ready' >> notifications still go through #PF (to be changed in the future). Make the >> probability of running into issues when APF collides with regular #PF lower >> by deprecating KVM_ASYNC_PF_SEND_ALWAYS. The feature doesn't seem to be >> important enough for any particular workload to notice the difference. > > This has been disabled already in guest code, but I don't see a > particular reason to deprecate it in the host too. Supporting it on the > host is just one line of code; if it's a problem *for the guest*, you > just don't use KVM_ASYNC_PF_SEND_ALWAYS. > > Also, note that #VE will always be delivered to the guest even at CPL0; > the decision on whether to do sync or async page fault at CPL0 will move > to the guest, but enabling #VE will probably _require_ the > KVM_ASYNC_PF_SEND_ALWAYS bit to be set (and KVM_ASYNC_PF_DELIVERY_AS_INT > as well). I actually missed the fact that guest side is already disabled (I can see it now in the queue), feel free to ignore this patch then.
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 356c02bfa587..f491214b7667 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -769,7 +769,6 @@ struct kvm_vcpu_arch { u64 msr_int_val; /* MSR_KVM_ASYNC_PF_INT */ u16 vec; u32 id; - bool send_user_only; u32 host_apf_flags; unsigned long nested_apf_token; bool delivery_as_pf_vmexit; diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h index 812e9b4c1114..3cae0faac2b8 100644 --- a/arch/x86/include/uapi/asm/kvm_para.h +++ b/arch/x86/include/uapi/asm/kvm_para.h @@ -82,7 +82,7 @@ struct kvm_clock_pairing { #define KVM_MAX_MMU_OP_BATCH 32 #define KVM_ASYNC_PF_ENABLED (1 << 0) -#define KVM_ASYNC_PF_SEND_ALWAYS (1 << 1) +#define KVM_ASYNC_PF_SEND_ALWAYS (1 << 1) /* deprecated */ #define KVM_ASYNC_PF_DELIVERY_AS_PF_VMEXIT (1 << 2) #define KVM_ASYNC_PF_DELIVERY_AS_INT (1 << 3) diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c index 79730eaef1e1..add123302122 100644 --- a/arch/x86/kernel/kvm.c +++ b/arch/x86/kernel/kvm.c @@ -324,9 +324,6 @@ static void kvm_guest_cpu_init(void) if (kvm_para_has_feature(KVM_FEATURE_ASYNC_PF_INT) && kvmapf) { u64 pa = slow_virt_to_phys(this_cpu_ptr(&apf_reason)); -#ifdef CONFIG_PREEMPTION - pa |= KVM_ASYNC_PF_SEND_ALWAYS; -#endif pa |= KVM_ASYNC_PF_ENABLED | KVM_ASYNC_PF_DELIVERY_AS_INT; if (kvm_para_has_feature(KVM_FEATURE_ASYNC_PF_VMEXIT)) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index cc1bf6cfc5e0..8222133bf5be 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -2648,7 +2648,10 @@ static int kvm_pv_enable_async_pf(struct kvm_vcpu *vcpu, u64 data) { gpa_t gpa = data & ~0x3f; - /* Bits 4:5 are reserved, Should be zero */ + /* + * Bits 4:5 are reserved and should be zero. Bit 1 + * (KVM_ASYNC_PF_SEND_ALWAYS) was deprecated and is being ignored. + */ if (data & 0x30) return 1; @@ -2664,7 +2667,6 @@ static int kvm_pv_enable_async_pf(struct kvm_vcpu *vcpu, u64 data) sizeof(u64))) return 1; - vcpu->arch.apf.send_user_only = !(data & KVM_ASYNC_PF_SEND_ALWAYS); vcpu->arch.apf.delivery_as_pf_vmexit = data & KVM_ASYNC_PF_DELIVERY_AS_PF_VMEXIT; kvm_async_pf_wakeup_all(vcpu); @@ -10433,8 +10435,11 @@ static bool kvm_can_deliver_async_pf(struct kvm_vcpu *vcpu) if (!vcpu->arch.apf.delivery_as_pf_vmexit && is_guest_mode(vcpu)) return false; - if (!kvm_pv_async_pf_enabled(vcpu) || - (vcpu->arch.apf.send_user_only && kvm_x86_ops.get_cpl(vcpu) == 0)) + /* + * 'Page not present' APF events are only delivered when CPU is in + * user mode and APF mechanism is enabled. + */ + if (!kvm_pv_async_pf_enabled(vcpu) || kvm_x86_ops.get_cpl(vcpu) == 0) return false; return true;
Concerns were expressed around APF events delivery when CPU is not in user mode (KVM_ASYNC_PF_SEND_ALWAYS), e.g. https://lore.kernel.org/kvm/ed71d0967113a35f670a9625a058b8e6e0b2f104.1583547991.git.luto@kernel.org/ 'Page ready' events are already free from '#PF abuse' but 'page not ready' notifications still go through #PF (to be changed in the future). Make the probability of running into issues when APF collides with regular #PF lower by deprecating KVM_ASYNC_PF_SEND_ALWAYS. The feature doesn't seem to be important enough for any particular workload to notice the difference. Suggested-by: Andy Lutomirski <luto@kernel.org> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/x86/include/asm/kvm_host.h | 1 - arch/x86/include/uapi/asm/kvm_para.h | 2 +- arch/x86/kernel/kvm.c | 3 --- arch/x86/kvm/x86.c | 13 +++++++++---- 4 files changed, 10 insertions(+), 9 deletions(-)