> On 27 Sep 2019, at 5:15, Tao Xu <tao3.xu@xxxxxxxxx> wrote: > > On 9/26/2019 7:24 PM, Liran Alon wrote: >> I just reviewed the patch "KVM: vmx: Introduce handle_unexpected_vmexit and handle WAITPKG vmexit” currently queued in kvm git tree >> (https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_virt_kvm_kvm.git_commit_-3Fh-3Dqueue-26id-3Dbf653b78f9608d66db174eabcbda7454c8fde6d5&d=DwIDaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=tOlWXew8EBjyAF7RjD8oKfiwr3Xt-mbLH9WcmUkGa5s&s=oqAj0v9cdPTzfE1MLuMz5FJDv_Y3rQgaXp0-2ksmwOo&e= ) >> It seems to me that we shouldn’t apply this patch in it’s current form. >> Instead of having a common handle_unexpected_vmexit() manually specified for specific VMExit reasons, >> we should rely on the functionality I have added to vmx_handle_exit() in case there is no valid handler for exit-reason. >> In this case (since commit 7396d337cfadc ("KVM: x86: Return to userspace with internal error on unexpected exit reason”), >> an internal-error will be raised to userspace as required. Instead of silently skipping emulated instruction. >> -Liran > > +Sean > > Hi Liran, > > After read your code, I understand your suggestion. But if we don't add exit reason for XSAVES/XRSTORS/UMWAIT/TPAUSE like this: > > @@ -5565,13 +5559,15 @@ static int (*kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = { > [...] > - [EXIT_REASON_XSAVES] = handle_xsaves, > - [EXIT_REASON_XRSTORS] = handle_xrstors, > + [EXIT_REASON_XSAVES] = NULL, > + [EXIT_REASON_XRSTORS] = NULL, > [...] > + [EXIT_REASON_UMWAIT] = NULL, > + [EXIT_REASON_TPAUSE] = NULL, > > It is confused when someone read these code. Why is it confusing? Any exit-reason not specified in kvm_vmx_exit_handlers[] is an exit-reason KVM doesn’t expect to be raised from hardware. Whether it’s because VMCS is configured to not raise that exit-reason or because it’s a new exit-reason only supported on newer CPUs. (Which is kinda the same. Because a new exit-reason should be raised only if hypervisor opt-in some VMCS feature). I think explicitly adding handlers for known exit-reasons to call handle_unexpected_vmexit() is confusing. (In addition, this misaligns VMX treatment of unexpected exit-reasons from SVM treatment of exactly the same. Which is currently aligned). -Liran > So how about I move your code chunk: > > vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", > exit_reason); > dump_vmcs(); > vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR; > vcpu->run->internal.suberror = > KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON; > vcpu->run->internal.ndata = 1; > vcpu->run->internal.data[0] = exit_reason; > > into handle_unexpected_vmexit(), then this function become: > > static int handle_unexpected_vmexit(struct kvm_vcpu *vcpu) > { > vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", > exit_reason); > dump_vmcs(); > vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR; > vcpu->run->internal.suberror = > KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON; > vcpu->run->internal.ndata = 1; > vcpu->run->internal.data[0] = to_vmx(cpu)->exit_reason; > return 0; > } > > Then vmx_handle_exit() also can call this function. > > How about this solution? > > Tao
