* [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() @ 2020-06-10 17:55 Vitaly Kuznetsov 2020-06-10 17:55 ` [PATCH 2/2] KVM: async_pf: Inject 'page ready' event only if 'page not present' was previously injected Vitaly Kuznetsov 2020-06-10 18:14 ` [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() Sean Christopherson 0 siblings, 2 replies; 11+ messages in thread From: Vitaly Kuznetsov @ 2020-06-10 17:55 UTC (permalink / raw) To: kvm, Paolo Bonzini Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Vivek Goyal, linux-kernel schedule_work() returns 'false' only when the work is already on the queue and this can't happen as kvm_setup_async_pf() always allocates a new one. Also, to avoid potential race, it makes sense to to schedule_work() at the very end after we've added it to the queue. While on it, do some minor cleanup. gfn_to_pfn_async() mentioned in a comment does not currently exist and, moreover, we can check kvm_is_error_hva() at the very beginning, before we try to allocate work so 'retry_sync' label can go away completely. Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- virt/kvm/async_pf.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c index f1e07fae84e9..ba080088da76 100644 --- a/virt/kvm/async_pf.c +++ b/virt/kvm/async_pf.c @@ -164,7 +164,9 @@ int kvm_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, if (vcpu->async_pf.queued >= ASYNC_PF_PER_VCPU) return 0; - /* setup delayed work */ + /* Arch specific code should not do async PF in this case */ + if (unlikely(kvm_is_error_hva(hva))) + return 0; /* * do alloc nowait since if we are going to sleep anyway we @@ -183,24 +185,15 @@ int kvm_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, mmget(work->mm); kvm_get_kvm(work->vcpu->kvm); - /* this can't really happen otherwise gfn_to_pfn_async - would succeed */ - if (unlikely(kvm_is_error_hva(work->addr))) - goto retry_sync; - INIT_WORK(&work->work, async_pf_execute); - if (!schedule_work(&work->work)) - goto retry_sync; list_add_tail(&work->queue, &vcpu->async_pf.queue); vcpu->async_pf.queued++; kvm_arch_async_page_not_present(vcpu, work); + + schedule_work(&work->work); + return 1; -retry_sync: - kvm_put_kvm(work->vcpu->kvm); - mmput(work->mm); - kmem_cache_free(async_pf_cache, work); - return 0; } int kvm_async_pf_wakeup_all(struct kvm_vcpu *vcpu) -- 2.25.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] KVM: async_pf: Inject 'page ready' event only if 'page not present' was previously injected 2020-06-10 17:55 [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() Vitaly Kuznetsov @ 2020-06-10 17:55 ` Vitaly Kuznetsov 2020-06-10 19:32 ` Vivek Goyal 2020-06-10 18:14 ` [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() Sean Christopherson 1 sibling, 1 reply; 11+ messages in thread From: Vitaly Kuznetsov @ 2020-06-10 17:55 UTC (permalink / raw) To: kvm, Paolo Bonzini Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Vivek Goyal, linux-kernel 'Page not present' event may or may not get injected depending on guest's state. If the event wasn't injected, there is no need to inject the corresponding 'page ready' event as the guest may get confused. E.g. Linux thinks that the corresponding 'page not present' event wasn't delivered *yet* and allocates a 'dummy entry' for it. This entry is never freed. Note, 'wakeup all' events have no corresponding 'page not present' event and always get injected. s390 seems to always be able to inject 'page not present', the change is effectively a nop. Suggested-by: Vivek Goyal <vgoyal@redhat.com> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/s390/include/asm/kvm_host.h | 2 +- arch/s390/kvm/kvm-s390.c | 4 +++- arch/x86/include/asm/kvm_host.h | 2 +- arch/x86/kvm/x86.c | 7 +++++-- include/linux/kvm_host.h | 1 + virt/kvm/async_pf.c | 2 +- 6 files changed, 12 insertions(+), 6 deletions(-) diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h index 3d554887794e..cee3cb6455a2 100644 --- a/arch/s390/include/asm/kvm_host.h +++ b/arch/s390/include/asm/kvm_host.h @@ -978,7 +978,7 @@ bool kvm_arch_can_dequeue_async_page_present(struct kvm_vcpu *vcpu); void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu, struct kvm_async_pf *work); -void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, +bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, struct kvm_async_pf *work); void kvm_arch_async_page_present(struct kvm_vcpu *vcpu, diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index 06bde4bad205..33fea4488ef3 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -3923,11 +3923,13 @@ static void __kvm_inject_pfault_token(struct kvm_vcpu *vcpu, bool start_token, } } -void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, +bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, struct kvm_async_pf *work) { trace_kvm_s390_pfault_init(vcpu, work->arch.pfault_token); __kvm_inject_pfault_token(vcpu, true, work->arch.pfault_token); + + return true; } void kvm_arch_async_page_present(struct kvm_vcpu *vcpu, diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 6e03c021956a..f54e7499fc6a 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1660,7 +1660,7 @@ void kvm_make_scan_ioapic_request(struct kvm *kvm); void kvm_make_scan_ioapic_request_mask(struct kvm *kvm, unsigned long *vcpu_bitmap); -void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, +bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, struct kvm_async_pf *work); void kvm_arch_async_page_present(struct kvm_vcpu *vcpu, struct kvm_async_pf *work); diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 13d0b0fa1a7c..75e4c68e9586 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -10513,7 +10513,7 @@ bool kvm_can_do_async_pf(struct kvm_vcpu *vcpu) return kvm_arch_interrupt_allowed(vcpu); } -void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, +bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, struct kvm_async_pf *work) { struct x86_exception fault; @@ -10530,6 +10530,7 @@ void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, fault.address = work->arch.token; fault.async_page_fault = true; kvm_inject_page_fault(vcpu, &fault); + return true; } else { /* * It is not possible to deliver a paravirtualized asynchronous @@ -10540,6 +10541,7 @@ void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, * fault is retried, hopefully the page will be ready in the host. */ kvm_make_request(KVM_REQ_APF_HALT, vcpu); + return false; } } @@ -10557,7 +10559,8 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu, kvm_del_async_pf_gfn(vcpu, work->arch.gfn); trace_kvm_async_pf_ready(work->arch.token, work->cr2_or_gpa); - if (kvm_pv_async_pf_enabled(vcpu) && + if ((work->wakeup_all || work->notpresent_injected) && + kvm_pv_async_pf_enabled(vcpu) && !apf_put_user_ready(vcpu, work->arch.token)) { vcpu->arch.apf.pageready_pending = true; kvm_apic_set_irq(vcpu, &irq, NULL); diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 802b9e2306f0..2456dc5338f8 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -206,6 +206,7 @@ struct kvm_async_pf { unsigned long addr; struct kvm_arch_async_pf arch; bool wakeup_all; + bool notpresent_injected; }; void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu); diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c index ba080088da76..a36828fbf40a 100644 --- a/virt/kvm/async_pf.c +++ b/virt/kvm/async_pf.c @@ -189,7 +189,7 @@ int kvm_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, list_add_tail(&work->queue, &vcpu->async_pf.queue); vcpu->async_pf.queued++; - kvm_arch_async_page_not_present(vcpu, work); + work->notpresent_injected = kvm_arch_async_page_not_present(vcpu, work); schedule_work(&work->work); -- 2.25.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] KVM: async_pf: Inject 'page ready' event only if 'page not present' was previously injected 2020-06-10 17:55 ` [PATCH 2/2] KVM: async_pf: Inject 'page ready' event only if 'page not present' was previously injected Vitaly Kuznetsov @ 2020-06-10 19:32 ` Vivek Goyal 2020-06-10 19:47 ` Sean Christopherson 2020-06-11 8:14 ` Vitaly Kuznetsov 0 siblings, 2 replies; 11+ messages in thread From: Vivek Goyal @ 2020-06-10 19:32 UTC (permalink / raw) To: Vitaly Kuznetsov Cc: kvm, Paolo Bonzini, Sean Christopherson, Wanpeng Li, Jim Mattson, linux-kernel On Wed, Jun 10, 2020 at 07:55:32PM +0200, Vitaly Kuznetsov wrote: > 'Page not present' event may or may not get injected depending on > guest's state. If the event wasn't injected, there is no need to > inject the corresponding 'page ready' event as the guest may get > confused. E.g. Linux thinks that the corresponding 'page not present' > event wasn't delivered *yet* and allocates a 'dummy entry' for it. > This entry is never freed. > > Note, 'wakeup all' events have no corresponding 'page not present' > event and always get injected. > > s390 seems to always be able to inject 'page not present', the > change is effectively a nop. > > Suggested-by: Vivek Goyal <vgoyal@redhat.com> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > arch/s390/include/asm/kvm_host.h | 2 +- > arch/s390/kvm/kvm-s390.c | 4 +++- > arch/x86/include/asm/kvm_host.h | 2 +- > arch/x86/kvm/x86.c | 7 +++++-- > include/linux/kvm_host.h | 1 + > virt/kvm/async_pf.c | 2 +- > 6 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index 3d554887794e..cee3cb6455a2 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -978,7 +978,7 @@ bool kvm_arch_can_dequeue_async_page_present(struct kvm_vcpu *vcpu); > void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu, > struct kvm_async_pf *work); > > -void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, > +bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, > struct kvm_async_pf *work); Hi Vitaly, A minor nit. Using return code to figure out if exception was injected or not is little odd. How about we pass a pointer instead as parameter and kvm_arch_async_page_not_present() sets it to true if page not present exception was injected. This probably will be easier to read. If for some reason you don't like above, atleats it warrants a comment explaining what do 0 and 1 mean. Otherwise both the patches look good to me. I tested and I can confirm that now page ready events are not being delivered to guest if page not present was not injected. Thanks Vivek > > void kvm_arch_async_page_present(struct kvm_vcpu *vcpu, > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 06bde4bad205..33fea4488ef3 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -3923,11 +3923,13 @@ static void __kvm_inject_pfault_token(struct kvm_vcpu *vcpu, bool start_token, > } > } > > -void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, > +bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, > struct kvm_async_pf *work) > { > trace_kvm_s390_pfault_init(vcpu, work->arch.pfault_token); > __kvm_inject_pfault_token(vcpu, true, work->arch.pfault_token); > + > + return true; > } > > void kvm_arch_async_page_present(struct kvm_vcpu *vcpu, > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 6e03c021956a..f54e7499fc6a 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -1660,7 +1660,7 @@ void kvm_make_scan_ioapic_request(struct kvm *kvm); > void kvm_make_scan_ioapic_request_mask(struct kvm *kvm, > unsigned long *vcpu_bitmap); > > -void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, > +bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, > struct kvm_async_pf *work); > void kvm_arch_async_page_present(struct kvm_vcpu *vcpu, > struct kvm_async_pf *work); > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 13d0b0fa1a7c..75e4c68e9586 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -10513,7 +10513,7 @@ bool kvm_can_do_async_pf(struct kvm_vcpu *vcpu) > return kvm_arch_interrupt_allowed(vcpu); > } > > -void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, > +bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, > struct kvm_async_pf *work) > { > struct x86_exception fault; > @@ -10530,6 +10530,7 @@ void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, > fault.address = work->arch.token; > fault.async_page_fault = true; > kvm_inject_page_fault(vcpu, &fault); > + return true; > } else { > /* > * It is not possible to deliver a paravirtualized asynchronous > @@ -10540,6 +10541,7 @@ void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, > * fault is retried, hopefully the page will be ready in the host. > */ > kvm_make_request(KVM_REQ_APF_HALT, vcpu); > + return false; > } > } > > @@ -10557,7 +10559,8 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu, > kvm_del_async_pf_gfn(vcpu, work->arch.gfn); > trace_kvm_async_pf_ready(work->arch.token, work->cr2_or_gpa); > > - if (kvm_pv_async_pf_enabled(vcpu) && > + if ((work->wakeup_all || work->notpresent_injected) && > + kvm_pv_async_pf_enabled(vcpu) && > !apf_put_user_ready(vcpu, work->arch.token)) { > vcpu->arch.apf.pageready_pending = true; > kvm_apic_set_irq(vcpu, &irq, NULL); > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 802b9e2306f0..2456dc5338f8 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -206,6 +206,7 @@ struct kvm_async_pf { > unsigned long addr; > struct kvm_arch_async_pf arch; > bool wakeup_all; > + bool notpresent_injected; > }; > > void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu); > diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c > index ba080088da76..a36828fbf40a 100644 > --- a/virt/kvm/async_pf.c > +++ b/virt/kvm/async_pf.c > @@ -189,7 +189,7 @@ int kvm_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, > > list_add_tail(&work->queue, &vcpu->async_pf.queue); > vcpu->async_pf.queued++; > - kvm_arch_async_page_not_present(vcpu, work); > + work->notpresent_injected = kvm_arch_async_page_not_present(vcpu, work); > > schedule_work(&work->work); > > -- > 2.25.4 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] KVM: async_pf: Inject 'page ready' event only if 'page not present' was previously injected 2020-06-10 19:32 ` Vivek Goyal @ 2020-06-10 19:47 ` Sean Christopherson 2020-06-10 19:57 ` Vivek Goyal 2020-06-11 8:14 ` Vitaly Kuznetsov 1 sibling, 1 reply; 11+ messages in thread From: Sean Christopherson @ 2020-06-10 19:47 UTC (permalink / raw) To: Vivek Goyal Cc: Vitaly Kuznetsov, kvm, Paolo Bonzini, Wanpeng Li, Jim Mattson, linux-kernel On Wed, Jun 10, 2020 at 03:32:11PM -0400, Vivek Goyal wrote: > On Wed, Jun 10, 2020 at 07:55:32PM +0200, Vitaly Kuznetsov wrote: > > 'Page not present' event may or may not get injected depending on > > guest's state. If the event wasn't injected, there is no need to > > inject the corresponding 'page ready' event as the guest may get > > confused. E.g. Linux thinks that the corresponding 'page not present' > > event wasn't delivered *yet* and allocates a 'dummy entry' for it. > > This entry is never freed. > > > > Note, 'wakeup all' events have no corresponding 'page not present' > > event and always get injected. > > > > s390 seems to always be able to inject 'page not present', the > > change is effectively a nop. > > > > Suggested-by: Vivek Goyal <vgoyal@redhat.com> > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > --- > > arch/s390/include/asm/kvm_host.h | 2 +- > > arch/s390/kvm/kvm-s390.c | 4 +++- > > arch/x86/include/asm/kvm_host.h | 2 +- > > arch/x86/kvm/x86.c | 7 +++++-- > > include/linux/kvm_host.h | 1 + > > virt/kvm/async_pf.c | 2 +- > > 6 files changed, 12 insertions(+), 6 deletions(-) > > > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > > index 3d554887794e..cee3cb6455a2 100644 > > --- a/arch/s390/include/asm/kvm_host.h > > +++ b/arch/s390/include/asm/kvm_host.h > > @@ -978,7 +978,7 @@ bool kvm_arch_can_dequeue_async_page_present(struct kvm_vcpu *vcpu); > > void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu, > > struct kvm_async_pf *work); > > > > -void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, > > +bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, > > struct kvm_async_pf *work); > > Hi Vitaly, > > A minor nit. Using return code to figure out if exception was injected > or not is little odd. How about we pass a pointer instead as parameter > and kvm_arch_async_page_not_present() sets it to true if page not > present exception was injected. This probably will be easier to > read. > > If for some reason you don't like above, atleats it warrants a comment > explaining what do 0 and 1 mean. > > Otherwise both the patches look good to me. I tested and I can confirm > that now page ready events are not being delivered to guest if page > not present was not injected. Why does kvm_arch_async_page_not_present() need to "return" anything? It has access to @work, e.g. simply replace "return true" with "work->notpresent_injected = true". ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] KVM: async_pf: Inject 'page ready' event only if 'page not present' was previously injected 2020-06-10 19:47 ` Sean Christopherson @ 2020-06-10 19:57 ` Vivek Goyal 2020-06-10 23:53 ` Paolo Bonzini 0 siblings, 1 reply; 11+ messages in thread From: Vivek Goyal @ 2020-06-10 19:57 UTC (permalink / raw) To: Sean Christopherson Cc: Vitaly Kuznetsov, kvm, Paolo Bonzini, Wanpeng Li, Jim Mattson, linux-kernel On Wed, Jun 10, 2020 at 12:47:38PM -0700, Sean Christopherson wrote: > On Wed, Jun 10, 2020 at 03:32:11PM -0400, Vivek Goyal wrote: > > On Wed, Jun 10, 2020 at 07:55:32PM +0200, Vitaly Kuznetsov wrote: > > > 'Page not present' event may or may not get injected depending on > > > guest's state. If the event wasn't injected, there is no need to > > > inject the corresponding 'page ready' event as the guest may get > > > confused. E.g. Linux thinks that the corresponding 'page not present' > > > event wasn't delivered *yet* and allocates a 'dummy entry' for it. > > > This entry is never freed. > > > > > > Note, 'wakeup all' events have no corresponding 'page not present' > > > event and always get injected. > > > > > > s390 seems to always be able to inject 'page not present', the > > > change is effectively a nop. > > > > > > Suggested-by: Vivek Goyal <vgoyal@redhat.com> > > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > > --- > > > arch/s390/include/asm/kvm_host.h | 2 +- > > > arch/s390/kvm/kvm-s390.c | 4 +++- > > > arch/x86/include/asm/kvm_host.h | 2 +- > > > arch/x86/kvm/x86.c | 7 +++++-- > > > include/linux/kvm_host.h | 1 + > > > virt/kvm/async_pf.c | 2 +- > > > 6 files changed, 12 insertions(+), 6 deletions(-) > > > > > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > > > index 3d554887794e..cee3cb6455a2 100644 > > > --- a/arch/s390/include/asm/kvm_host.h > > > +++ b/arch/s390/include/asm/kvm_host.h > > > @@ -978,7 +978,7 @@ bool kvm_arch_can_dequeue_async_page_present(struct kvm_vcpu *vcpu); > > > void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu, > > > struct kvm_async_pf *work); > > > > > > -void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, > > > +bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, > > > struct kvm_async_pf *work); > > > > Hi Vitaly, > > > > A minor nit. Using return code to figure out if exception was injected > > or not is little odd. How about we pass a pointer instead as parameter > > and kvm_arch_async_page_not_present() sets it to true if page not > > present exception was injected. This probably will be easier to > > read. > > > > If for some reason you don't like above, atleats it warrants a comment > > explaining what do 0 and 1 mean. > > > > Otherwise both the patches look good to me. I tested and I can confirm > > that now page ready events are not being delivered to guest if page > > not present was not injected. > > Why does kvm_arch_async_page_not_present() need to "return" anything? It > has access to @work, e.g. simply replace "return true" with > "work->notpresent_injected = true". We could do it and I thought about it. But modifying work->notpresent_injected inside kvm_arch_async_page_not_present() again feels very unintuitive. I personally find it better that initialization of work->notpresent_injected is very explicit at the site where this structure has been allocated and being initialized. (Instead of a a callee function silently initializing a filed of this structure). Thanks Vivek ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] KVM: async_pf: Inject 'page ready' event only if 'page not present' was previously injected 2020-06-10 19:57 ` Vivek Goyal @ 2020-06-10 23:53 ` Paolo Bonzini 0 siblings, 0 replies; 11+ messages in thread From: Paolo Bonzini @ 2020-06-10 23:53 UTC (permalink / raw) To: Vivek Goyal, Sean Christopherson Cc: Vitaly Kuznetsov, kvm, Wanpeng Li, Jim Mattson, linux-kernel On 10/06/20 21:57, Vivek Goyal wrote: > I personally find it better that initialization of > work->notpresent_injected is very explicit at the site where this > structure has been allocated and being initialized. (Instead of a > a callee function silently initializing a filed of this structure). I agree. Paolo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] KVM: async_pf: Inject 'page ready' event only if 'page not present' was previously injected 2020-06-10 19:32 ` Vivek Goyal 2020-06-10 19:47 ` Sean Christopherson @ 2020-06-11 8:14 ` Vitaly Kuznetsov 1 sibling, 0 replies; 11+ messages in thread From: Vitaly Kuznetsov @ 2020-06-11 8:14 UTC (permalink / raw) To: Vivek Goyal Cc: kvm, Paolo Bonzini, Sean Christopherson, Wanpeng Li, Jim Mattson, linux-kernel Vivek Goyal <vgoyal@redhat.com> writes: > On Wed, Jun 10, 2020 at 07:55:32PM +0200, Vitaly Kuznetsov wrote: >> 'Page not present' event may or may not get injected depending on >> guest's state. If the event wasn't injected, there is no need to >> inject the corresponding 'page ready' event as the guest may get >> confused. E.g. Linux thinks that the corresponding 'page not present' >> event wasn't delivered *yet* and allocates a 'dummy entry' for it. >> This entry is never freed. >> >> Note, 'wakeup all' events have no corresponding 'page not present' >> event and always get injected. >> >> s390 seems to always be able to inject 'page not present', the >> change is effectively a nop. >> >> Suggested-by: Vivek Goyal <vgoyal@redhat.com> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> >> --- >> arch/s390/include/asm/kvm_host.h | 2 +- >> arch/s390/kvm/kvm-s390.c | 4 +++- >> arch/x86/include/asm/kvm_host.h | 2 +- >> arch/x86/kvm/x86.c | 7 +++++-- >> include/linux/kvm_host.h | 1 + >> virt/kvm/async_pf.c | 2 +- >> 6 files changed, 12 insertions(+), 6 deletions(-) >> >> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >> index 3d554887794e..cee3cb6455a2 100644 >> --- a/arch/s390/include/asm/kvm_host.h >> +++ b/arch/s390/include/asm/kvm_host.h >> @@ -978,7 +978,7 @@ bool kvm_arch_can_dequeue_async_page_present(struct kvm_vcpu *vcpu); >> void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu, >> struct kvm_async_pf *work); >> >> -void kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, >> +bool kvm_arch_async_page_not_present(struct kvm_vcpu *vcpu, >> struct kvm_async_pf *work); > > Hi Vitaly, > > A minor nit. Using return code to figure out if exception was injected > or not is little odd. How about we pass a pointer instead as parameter > and kvm_arch_async_page_not_present() sets it to true if page not > present exception was injected. This probably will be easier to > read. > > If for some reason you don't like above, atleats it warrants a comment > explaining what do 0 and 1 mean. > I think it's the 'kvm_arch_async_page_not_present' name which is a bit misleading now, if we rename it to something like kvm_arch_inject_apf_not_present() then it becomes a bit more clear what's going on. We may as well write the code as if (kvm_arch_inject_apf_not_present()) work->notpresent_injected = true; or change the return type to int so it'll be if (!kvm_arch_inject_apf_not_present()) work->notpresent_injected = true; > Otherwise both the patches look good to me. I tested and I can confirm > that now page ready events are not being delivered to guest if page > not present was not injected. Thank you for testing! -- Vitaly ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() 2020-06-10 17:55 [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() Vitaly Kuznetsov 2020-06-10 17:55 ` [PATCH 2/2] KVM: async_pf: Inject 'page ready' event only if 'page not present' was previously injected Vitaly Kuznetsov @ 2020-06-10 18:14 ` Sean Christopherson 2020-06-11 0:07 ` Paolo Bonzini 2020-06-11 8:31 ` Vitaly Kuznetsov 1 sibling, 2 replies; 11+ messages in thread From: Sean Christopherson @ 2020-06-10 18:14 UTC (permalink / raw) To: Vitaly Kuznetsov Cc: kvm, Paolo Bonzini, Wanpeng Li, Jim Mattson, Vivek Goyal, linux-kernel On Wed, Jun 10, 2020 at 07:55:31PM +0200, Vitaly Kuznetsov wrote: > schedule_work() returns 'false' only when the work is already on the queue > and this can't happen as kvm_setup_async_pf() always allocates a new one. > Also, to avoid potential race, it makes sense to to schedule_work() at the > very end after we've added it to the queue. > > While on it, do some minor cleanup. gfn_to_pfn_async() mentioned in a > comment does not currently exist and, moreover, we can check > kvm_is_error_hva() at the very beginning, before we try to allocate work so > 'retry_sync' label can go away completely. > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > virt/kvm/async_pf.c | 19 ++++++------------- > 1 file changed, 6 insertions(+), 13 deletions(-) > > diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c > index f1e07fae84e9..ba080088da76 100644 > --- a/virt/kvm/async_pf.c > +++ b/virt/kvm/async_pf.c > @@ -164,7 +164,9 @@ int kvm_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, > if (vcpu->async_pf.queued >= ASYNC_PF_PER_VCPU) > return 0; > > - /* setup delayed work */ > + /* Arch specific code should not do async PF in this case */ > + if (unlikely(kvm_is_error_hva(hva))) This feels like it should be changed to a WARN_ON_ONCE in a follow-up. With the WARN, the comment could probably be dropped. I'd also be in favor of changing the return type to a boolean. I think you alluded to it earlier, the current semantics are quite confusing as they invert the normal "return 0 on success". For this patch: Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() 2020-06-10 18:14 ` [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() Sean Christopherson @ 2020-06-11 0:07 ` Paolo Bonzini 2020-06-11 8:31 ` Vitaly Kuznetsov 1 sibling, 0 replies; 11+ messages in thread From: Paolo Bonzini @ 2020-06-11 0:07 UTC (permalink / raw) To: Sean Christopherson, Vitaly Kuznetsov Cc: kvm, Wanpeng Li, Jim Mattson, Vivek Goyal, linux-kernel On 10/06/20 20:14, Sean Christopherson wrote: >> - /* setup delayed work */ >> + /* Arch specific code should not do async PF in this case */ >> + if (unlikely(kvm_is_error_hva(hva))) > This feels like it should be changed to a WARN_ON_ONCE in a follow-up. > With the WARN, the comment could probably be dropped. I think a race is possible in principle where the memslots are changed (for example) between s390's page fault handler and the gfn_to_hva call in kvm_arch_setup_async_pf. Queued both, thanks! Paolo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() 2020-06-10 18:14 ` [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() Sean Christopherson 2020-06-11 0:07 ` Paolo Bonzini @ 2020-06-11 8:31 ` Vitaly Kuznetsov 2020-06-11 15:35 ` Sean Christopherson 1 sibling, 1 reply; 11+ messages in thread From: Vitaly Kuznetsov @ 2020-06-11 8:31 UTC (permalink / raw) To: Sean Christopherson Cc: kvm, Paolo Bonzini, Wanpeng Li, Jim Mattson, Vivek Goyal, linux-kernel Sean Christopherson <sean.j.christopherson@intel.com> writes: > > I'd also be in favor of changing the return type to a boolean. I think > you alluded to it earlier, the current semantics are quite confusing as they > invert the normal "return 0 on success". Yes, will do a follow-up. KVM/x86 code has an intertwined mix of: - normal 'int' functions ('0 on success') - bool functions ('true'/'1' on success) - 'int' exit handlers ('1'/'0' on success depending if exit to userspace was required) - ... I think we can try to standardize this to: - 'int' when error is propagated outside of KVM (userspace, other kernel subsystem,...) - 'bool' when the function is internal to KVM and the result is binary ('is_exit_required()', 'was_pf_injected()', 'will_have_another_beer()', ...) - 'enum' for the rest. And, if there's a good reason for making an exception, require a comment. (leaving aside everything returning a pointer, of course as these are self-explanatory -- unless it's 'void *' :-)) > > For this patch: > > Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com> > Thank you! -- Vitaly ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() 2020-06-11 8:31 ` Vitaly Kuznetsov @ 2020-06-11 15:35 ` Sean Christopherson 0 siblings, 0 replies; 11+ messages in thread From: Sean Christopherson @ 2020-06-11 15:35 UTC (permalink / raw) To: Vitaly Kuznetsov Cc: kvm, Paolo Bonzini, Wanpeng Li, Jim Mattson, Vivek Goyal, linux-kernel On Thu, Jun 11, 2020 at 10:31:08AM +0200, Vitaly Kuznetsov wrote: > Sean Christopherson <sean.j.christopherson@intel.com> writes: > > > > > I'd also be in favor of changing the return type to a boolean. I think > > you alluded to it earlier, the current semantics are quite confusing as they > > invert the normal "return 0 on success". > > Yes, will do a follow-up. > > KVM/x86 code has an intertwined mix of: > - normal 'int' functions ('0 on success') > - bool functions ('true'/'1' on success) > - 'int' exit handlers ('1'/'0' on success depending if exit to userspace > was required) > - ... > > I think we can try to standardize this to: > - 'int' when error is propagated outside of KVM (userspace, other kernel > subsystem,...) > - 'bool' when the function is internal to KVM and the result is binary > ('is_exit_required()', 'was_pf_injected()', 'will_have_another_beer()', > ...) > - 'enum' for the rest. > And, if there's a good reason for making an exception, require a > comment. (leaving aside everything returning a pointer, of course as > these are self-explanatory -- unless it's 'void *' :-)) Agreed for 'bool' case, but 'int' versus 'enum' is less straightforward as there are a huge number of functions that _may_ propagate an error outside of KVM, including all of the exit handlers. As Paolo point out[*], it'd be quite easy to end up with a mixture of enum/#define and 0/1 code, which would be an even bigger mess than what we have today. There are undoubtedly cases that could be converted to an enum, but they're probably few and far between as it requires total encapsulation, e.g. the emulator. [*] https://lkml.kernel.org/r/3d827e8b-a04e-0a93-4bb4-e0e9d59036da@redhat.com ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-06-11 15:36 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-06-10 17:55 [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() Vitaly Kuznetsov 2020-06-10 17:55 ` [PATCH 2/2] KVM: async_pf: Inject 'page ready' event only if 'page not present' was previously injected Vitaly Kuznetsov 2020-06-10 19:32 ` Vivek Goyal 2020-06-10 19:47 ` Sean Christopherson 2020-06-10 19:57 ` Vivek Goyal 2020-06-10 23:53 ` Paolo Bonzini 2020-06-11 8:14 ` Vitaly Kuznetsov 2020-06-10 18:14 ` [PATCH 1/2] KVM: async_pf: Cleanup kvm_setup_async_pf() Sean Christopherson 2020-06-11 0:07 ` Paolo Bonzini 2020-06-11 8:31 ` Vitaly Kuznetsov 2020-06-11 15:35 ` Sean Christopherson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox