* Suggest changing commit "KVM: vmx: Introduce handle_unexpected_vmexit and handle WAITPKG vmexit"
@ 2019-09-26 11:24 Liran Alon
2019-09-27 2:15 ` Tao Xu
0 siblings, 1 reply; 5+ messages in thread
From: Liran Alon @ 2019-09-26 11:24 UTC (permalink / raw)
To: kvm list, Paolo Bonzini, Tao Xu
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Suggest changing commit "KVM: vmx: Introduce handle_unexpected_vmexit and handle WAITPKG vmexit"
2019-09-26 11:24 Suggest changing commit "KVM: vmx: Introduce handle_unexpected_vmexit and handle WAITPKG vmexit" Liran Alon
@ 2019-09-27 2:15 ` Tao Xu
2019-09-27 14:55 ` Liran Alon
0 siblings, 1 reply; 5+ messages in thread
From: Tao Xu @ 2019-09-27 2:15 UTC (permalink / raw)
To: Liran Alon, kvm list, Paolo Bonzini, Sean Christopherson
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Suggest changing commit "KVM: vmx: Introduce handle_unexpected_vmexit and handle WAITPKG vmexit"
2019-09-27 2:15 ` Tao Xu
@ 2019-09-27 14:55 ` Liran Alon
2019-09-27 15:21 ` Paolo Bonzini
0 siblings, 1 reply; 5+ messages in thread
From: Liran Alon @ 2019-09-27 14:55 UTC (permalink / raw)
To: Tao Xu; +Cc: kvm list, Paolo Bonzini, Sean Christopherson
> On 27 Sep 2019, at 5:15, Tao Xu <tao3.xu@intel.com> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Suggest changing commit "KVM: vmx: Introduce handle_unexpected_vmexit and handle WAITPKG vmexit"
2019-09-27 14:55 ` Liran Alon
@ 2019-09-27 15:21 ` Paolo Bonzini
2019-09-29 0:59 ` Tao Xu
0 siblings, 1 reply; 5+ messages in thread
From: Paolo Bonzini @ 2019-09-27 15:21 UTC (permalink / raw)
To: Liran Alon, Tao Xu; +Cc: kvm list, Sean Christopherson
On 27/09/19 16:55, Liran Alon wrote:
> 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 agree that it's a bug compared to how other unhandled vmexits are
treated. I didn't want to rewrite kvm/next or have a revert, so I have
sent a pull request but this should be fixed. I'll wait for Liran's
patch or come up with one.
Paolo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Suggest changing commit "KVM: vmx: Introduce handle_unexpected_vmexit and handle WAITPKG vmexit"
2019-09-27 15:21 ` Paolo Bonzini
@ 2019-09-29 0:59 ` Tao Xu
0 siblings, 0 replies; 5+ messages in thread
From: Tao Xu @ 2019-09-29 0:59 UTC (permalink / raw)
To: Paolo Bonzini, Liran Alon; +Cc: kvm list, Sean Christopherson
On 9/27/2019 11:21 PM, Paolo Bonzini wrote:
> On 27/09/19 16:55, Liran Alon wrote:
>> 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 agree that it's a bug compared to how other unhandled vmexits are
> treated. I didn't want to rewrite kvm/next or have a revert, so I have
> sent a pull request but this should be fixed. I'll wait for Liran's
> patch or come up with one.
>
> Paolo
>
I got it. Thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-09-29 0:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-26 11:24 Suggest changing commit "KVM: vmx: Introduce handle_unexpected_vmexit and handle WAITPKG vmexit" Liran Alon
2019-09-27 2:15 ` Tao Xu
2019-09-27 14:55 ` Liran Alon
2019-09-27 15:21 ` Paolo Bonzini
2019-09-29 0:59 ` Tao Xu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.