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://git.kernel.org/pub/scm/virt/kvm/kvm.git/commit/?h=queue&id=bf653b78f9608d66db174eabcbda7454c8fde6d5)
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. 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
