| Message ID | 20200529153934.11694-9-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nSVM: event fixes and migration support | expand |
On 5/29/20 8:39 AM, Paolo Bonzini wrote: > Unmapping the nested VMCB in enter_svm_guest_mode is a bit of a wart, > since the map is not used elsewhere in the function. There are > just two calls, so move it there. The last sentence sounds bit incomplete. Also, does it make sense to mention the reason why unmapping is not required before we enter guest mode ? > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > arch/x86/kvm/svm/nested.c | 14 ++++++-------- > arch/x86/kvm/svm/svm.c | 3 ++- > arch/x86/kvm/svm/svm.h | 2 +- > 3 files changed, 9 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c > index 8756c9f463fd..8e98d5e420a5 100644 > --- a/arch/x86/kvm/svm/nested.c > +++ b/arch/x86/kvm/svm/nested.c > @@ -229,7 +229,7 @@ static bool nested_vmcb_checks(struct vmcb *vmcb) > } > > void enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa, > - struct vmcb *nested_vmcb, struct kvm_host_map *map) > + struct vmcb *nested_vmcb) > { > bool evaluate_pending_interrupts = > is_intercept(svm, INTERCEPT_VINTR) || > @@ -304,8 +304,6 @@ void enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa, > svm->vmcb->control.pause_filter_thresh = > nested_vmcb->control.pause_filter_thresh; > > - kvm_vcpu_unmap(&svm->vcpu, map, true); > - > /* Enter Guest-Mode */ > enter_guest_mode(&svm->vcpu); > > @@ -368,10 +366,7 @@ int nested_svm_vmrun(struct vcpu_svm *svm) > nested_vmcb->control.exit_code_hi = 0; > nested_vmcb->control.exit_info_1 = 0; > nested_vmcb->control.exit_info_2 = 0; > - > - kvm_vcpu_unmap(&svm->vcpu, &map, true); > - > - return ret; > + goto out; > } > > trace_kvm_nested_vmrun(svm->vmcb->save.rip, vmcb_gpa, > @@ -414,7 +409,7 @@ int nested_svm_vmrun(struct vcpu_svm *svm) > copy_vmcb_control_area(hsave, vmcb); > > svm->nested.nested_run_pending = 1; > - enter_svm_guest_mode(svm, vmcb_gpa, nested_vmcb, &map); > + enter_svm_guest_mode(svm, vmcb_gpa, nested_vmcb); > > if (!nested_svm_vmrun_msrpm(svm)) { > svm->vmcb->control.exit_code = SVM_EXIT_ERR; > @@ -425,6 +420,9 @@ int nested_svm_vmrun(struct vcpu_svm *svm) > nested_svm_vmexit(svm); > } > > +out: > + kvm_vcpu_unmap(&svm->vcpu, &map, true); > + > return ret; > } > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > index feb96a410f2d..76b3f553815e 100644 > --- a/arch/x86/kvm/svm/svm.c > +++ b/arch/x86/kvm/svm/svm.c > @@ -3814,7 +3814,8 @@ static int svm_pre_leave_smm(struct kvm_vcpu *vcpu, const char *smstate) > if (kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(vmcb), &map) == -EINVAL) > return 1; > nested_vmcb = map.hva; > - enter_svm_guest_mode(svm, vmcb, nested_vmcb, &map); > + enter_svm_guest_mode(svm, vmcb, nested_vmcb); > + kvm_vcpu_unmap(&svm->vcpu, &map, true); > } > return 0; > } > diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h > index 89fab75dd4f5..33e3f09d7a8e 100644 > --- a/arch/x86/kvm/svm/svm.h > +++ b/arch/x86/kvm/svm/svm.h > @@ -395,7 +395,7 @@ static inline bool nested_exit_on_nmi(struct vcpu_svm *svm) > } > > void enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa, > - struct vmcb *nested_vmcb, struct kvm_host_map *map); > + struct vmcb *nested_vmcb); > int nested_svm_vmrun(struct vcpu_svm *svm); > void nested_svm_vmloadsave(struct vmcb *from_vmcb, struct vmcb *to_vmcb); > int nested_svm_vmexit(struct vcpu_svm *svm);
On 29/05/20 20:10, Krish Sadhukhan wrote: >> Unmapping the nested VMCB in enter_svm_guest_mode is a bit of a wart, >> since the map is not used elsewhere in the function. There are >> just two calls, so move it there. > > The last sentence sounds bit incomplete. Good point---more precisely, "calls" should be "callers". "It" refers to "unmapping". > Also, does it make sense to mention the reason why unmapping is not > required before we enter guest mode ? Unmapping is a host operation and is not visible by the guest; is this what you mean? Paolo
On 5/29/20 12:04 PM, Paolo Bonzini wrote: > On 29/05/20 20:10, Krish Sadhukhan wrote: >>> Unmapping the nested VMCB in enter_svm_guest_mode is a bit of a wart, >>> since the map is not used elsewhere in the function. There are >>> just two calls, so move it there. >> The last sentence sounds bit incomplete. > Good point---more precisely, "calls" should be "callers". "It" refers > to "unmapping". > >> Also, does it make sense to mention the reason why unmapping is not >> required before we enter guest mode ? > Unmapping is a host operation and is not visible by the guest; is this > what you mean? Yes, I was thinking if we could mention it in the commit message... -Krish > > Paolo >
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c index 8756c9f463fd..8e98d5e420a5 100644 --- a/arch/x86/kvm/svm/nested.c +++ b/arch/x86/kvm/svm/nested.c @@ -229,7 +229,7 @@ static bool nested_vmcb_checks(struct vmcb *vmcb) } void enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa, - struct vmcb *nested_vmcb, struct kvm_host_map *map) + struct vmcb *nested_vmcb) { bool evaluate_pending_interrupts = is_intercept(svm, INTERCEPT_VINTR) || @@ -304,8 +304,6 @@ void enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa, svm->vmcb->control.pause_filter_thresh = nested_vmcb->control.pause_filter_thresh; - kvm_vcpu_unmap(&svm->vcpu, map, true); - /* Enter Guest-Mode */ enter_guest_mode(&svm->vcpu); @@ -368,10 +366,7 @@ int nested_svm_vmrun(struct vcpu_svm *svm) nested_vmcb->control.exit_code_hi = 0; nested_vmcb->control.exit_info_1 = 0; nested_vmcb->control.exit_info_2 = 0; - - kvm_vcpu_unmap(&svm->vcpu, &map, true); - - return ret; + goto out; } trace_kvm_nested_vmrun(svm->vmcb->save.rip, vmcb_gpa, @@ -414,7 +409,7 @@ int nested_svm_vmrun(struct vcpu_svm *svm) copy_vmcb_control_area(hsave, vmcb); svm->nested.nested_run_pending = 1; - enter_svm_guest_mode(svm, vmcb_gpa, nested_vmcb, &map); + enter_svm_guest_mode(svm, vmcb_gpa, nested_vmcb); if (!nested_svm_vmrun_msrpm(svm)) { svm->vmcb->control.exit_code = SVM_EXIT_ERR; @@ -425,6 +420,9 @@ int nested_svm_vmrun(struct vcpu_svm *svm) nested_svm_vmexit(svm); } +out: + kvm_vcpu_unmap(&svm->vcpu, &map, true); + return ret; } diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index feb96a410f2d..76b3f553815e 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -3814,7 +3814,8 @@ static int svm_pre_leave_smm(struct kvm_vcpu *vcpu, const char *smstate) if (kvm_vcpu_map(&svm->vcpu, gpa_to_gfn(vmcb), &map) == -EINVAL) return 1; nested_vmcb = map.hva; - enter_svm_guest_mode(svm, vmcb, nested_vmcb, &map); + enter_svm_guest_mode(svm, vmcb, nested_vmcb); + kvm_vcpu_unmap(&svm->vcpu, &map, true); } return 0; } diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h index 89fab75dd4f5..33e3f09d7a8e 100644 --- a/arch/x86/kvm/svm/svm.h +++ b/arch/x86/kvm/svm/svm.h @@ -395,7 +395,7 @@ static inline bool nested_exit_on_nmi(struct vcpu_svm *svm) } void enter_svm_guest_mode(struct vcpu_svm *svm, u64 vmcb_gpa, - struct vmcb *nested_vmcb, struct kvm_host_map *map); + struct vmcb *nested_vmcb); int nested_svm_vmrun(struct vcpu_svm *svm); void nested_svm_vmloadsave(struct vmcb *from_vmcb, struct vmcb *to_vmcb); int nested_svm_vmexit(struct vcpu_svm *svm);
Unmapping the nested VMCB in enter_svm_guest_mode is a bit of a wart, since the map is not used elsewhere in the function. There are just two calls, so move it there. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/kvm/svm/nested.c | 14 ++++++-------- arch/x86/kvm/svm/svm.c | 3 ++- arch/x86/kvm/svm/svm.h | 2 +- 3 files changed, 9 insertions(+), 10 deletions(-)