| Message ID | 1631964600-73707-1-git-send-email-hao.xiang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: VMX: Check if bus lock vmexit was preempted | expand |
On 18/09/21 13:30, Hao Xiang wrote: > exit_reason.bus_lock_detected is not only set when bus lock VM exit > was preempted, in fact, this bit is always set if bus locks are > detected no matter what the exit_reason.basic is. > > So the bus_lock_vmexit handling in vmx_handle_exit should be duplicated > when exit_reason.basic is EXIT_REASON_BUS_LOCK(74). We can avoid it by > checking if bus lock vmexit was preempted in vmx_handle_exit. I don't understand, does this mean that bus_lock_detected=1 if basic=EXIT_REASON_BUS_LOCK? If so, can we instead replace the contents of handle_bus_lock_vmexit with /* Do nothing and let vmx_handle_exit exit to userspace. */ WARN_ON(!to_vmx(vcpu)->exit_reason.bus_lock_detected); return 0; ? That would be doable only if this is architectural behavior and not a processor erratum, of course. Thanks, Paolo > Signed-off-by: Hao Xiang <hao.xiang@linux.alibaba.com> > --- > arch/x86/kvm/vmx/vmx.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 0c2c0d5..5ddf1df 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -6054,7 +6054,8 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath) > * still need to exit to user space when bus lock detected to inform > * that there is a bus lock in guest. > */ > - if (to_vmx(vcpu)->exit_reason.bus_lock_detected) { > + if (to_vmx(vcpu)->exit_reason.bus_lock_detected && > + to_vmx(vcpu)->exit_reason.basic != EXIT_REASON_BUS_LOCK) { > if (ret > 0) > vcpu->run->exit_reason = KVM_EXIT_X86_BUS_LOCK; > >
On 9/22/2021 6:02 PM, Paolo Bonzini wrote: > On 18/09/21 13:30, Hao Xiang wrote: >> exit_reason.bus_lock_detected is not only set when bus lock VM exit >> was preempted, in fact, this bit is always set if bus locks are >> detected no matter what the exit_reason.basic is. >> >> So the bus_lock_vmexit handling in vmx_handle_exit should be duplicated >> when exit_reason.basic is EXIT_REASON_BUS_LOCK(74). We can avoid it by >> checking if bus lock vmexit was preempted in vmx_handle_exit. > > I don't understand, does this mean that bus_lock_detected=1 if > basic=EXIT_REASON_BUS_LOCK? If so, can we instead replace the contents > of handle_bus_lock_vmexit with > > /* Do nothing and let vmx_handle_exit exit to userspace. */ > WARN_ON(!to_vmx(vcpu)->exit_reason.bus_lock_detected); > return 0; > > ? > > That would be doable only if this is architectural behavior and not a > processor erratum, of course. EXIT_REASON.bus_lock_detected may or may not be set when exit reason == EXIT_REASON_BUS_LOCK. Intel will update ISE or SDM to state it. Maybe we can do below in handle_bus_lock_vmexit handler: if (!to_vmx(vcpu)->exit_reason.bus_lock_detected) to_vmx(vcpu)->exit_reason.bus_lock_detected = 1; But is manually changing the hardware reported value for software purpose a good thing? > Thanks, > > Paolo > >> Signed-off-by: Hao Xiang <hao.xiang@linux.alibaba.com> >> --- >> arch/x86/kvm/vmx/vmx.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c >> index 0c2c0d5..5ddf1df 100644 >> --- a/arch/x86/kvm/vmx/vmx.c >> +++ b/arch/x86/kvm/vmx/vmx.c >> @@ -6054,7 +6054,8 @@ static int vmx_handle_exit(struct kvm_vcpu >> *vcpu, fastpath_t exit_fastpath) >> * still need to exit to user space when bus lock detected to >> inform >> * that there is a bus lock in guest. >> */ >> - if (to_vmx(vcpu)->exit_reason.bus_lock_detected) { >> + if (to_vmx(vcpu)->exit_reason.bus_lock_detected && >> + to_vmx(vcpu)->exit_reason.basic != EXIT_REASON_BUS_LOCK) { >> if (ret > 0) >> vcpu->run->exit_reason = KVM_EXIT_X86_BUS_LOCK; >> >
On 22/09/21 12:32, Xiaoyao Li wrote: >> > > EXIT_REASON.bus_lock_detected may or may not be set when exit reason == > EXIT_REASON_BUS_LOCK. Intel will update ISE or SDM to state it. > > Maybe we can do below in handle_bus_lock_vmexit handler: > > if (!to_vmx(vcpu)->exit_reason.bus_lock_detected) > to_vmx(vcpu)->exit_reason.bus_lock_detected = 1; > > But is manually changing the hardware reported value for software > purpose a good thing? No. That said, Hao's patch is just making the code clearer; there's no behavioral change since the "if" will just redo the same assignments as handle_bus_lock_vmexit. Paolo
On Wed, Sep 22, 2021, Xiaoyao Li wrote: > On 9/22/2021 6:02 PM, Paolo Bonzini wrote: > > On 18/09/21 13:30, Hao Xiang wrote: > > > exit_reason.bus_lock_detected is not only set when bus lock VM exit > > > was preempted, in fact, this bit is always set if bus locks are > > > detected no matter what the exit_reason.basic is. > > > > > > So the bus_lock_vmexit handling in vmx_handle_exit should be duplicated > > > when exit_reason.basic is EXIT_REASON_BUS_LOCK(74). We can avoid it by > > > checking if bus lock vmexit was preempted in vmx_handle_exit. > > > > I don't understand, does this mean that bus_lock_detected=1 if > > basic=EXIT_REASON_BUS_LOCK? If so, can we instead replace the contents > > of handle_bus_lock_vmexit with > > > > /* Do nothing and let vmx_handle_exit exit to userspace. */ > > WARN_ON(!to_vmx(vcpu)->exit_reason.bus_lock_detected); > > return 0; > > > > ? > > > > That would be doable only if this is architectural behavior and not a > > processor erratum, of course. > > EXIT_REASON.bus_lock_detected may or may not be set when exit reason == > EXIT_REASON_BUS_LOCK. Intel will update ISE or SDM to state it. > > Maybe we can do below in handle_bus_lock_vmexit handler: > > if (!to_vmx(vcpu)->exit_reason.bus_lock_detected) > to_vmx(vcpu)->exit_reason.bus_lock_detected = 1; > > But is manually changing the hardware reported value for software purpose a > good thing? In this case, I'd say yes. Hardware having non-deterministic behavior is the not good thing, KVM would simply be correctly the not-technically-an-erratum erratum. Set it unconditionally and then handle everything in common path. This has the added advantage of having only one site that deals with KVM_RUN_X86_BUS_LOCK. diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 33f92febe3ce..aa9372452e49 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -5561,9 +5561,9 @@ static int handle_encls(struct kvm_vcpu *vcpu) static int handle_bus_lock_vmexit(struct kvm_vcpu *vcpu) { - vcpu->run->exit_reason = KVM_EXIT_X86_BUS_LOCK; - vcpu->run->flags |= KVM_RUN_X86_BUS_LOCK; - return 0; + /* The dedicated flag may or may not be set by hardware. /facepalm. */ + vcpu->exit_reason.bus_lock_detected = true; + return 1; } /* @@ -6050,9 +6050,8 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath) int ret = __vmx_handle_exit(vcpu, exit_fastpath); /* - * Even when current exit reason is handled by KVM internally, we - * still need to exit to user space when bus lock detected to inform - * that there is a bus lock in guest. + * Exit to user space when bus lock detected to inform that there is a + * bus lock in guest. */ if (to_vmx(vcpu)->exit_reason.bus_lock_detected) { if (ret > 0)
On 2021/9/22 20:40, Paolo Bonzini wrote: > On 22/09/21 12:32, Xiaoyao Li wrote: >>> >> >> EXIT_REASON.bus_lock_detected may or may not be set when exit reason >> == EXIT_REASON_BUS_LOCK. Intel will update ISE or SDM to state it. >> >> Maybe we can do below in handle_bus_lock_vmexit handler: >> >> if (!to_vmx(vcpu)->exit_reason.bus_lock_detected) >> to_vmx(vcpu)->exit_reason.bus_lock_detected = 1; >> >> But is manually changing the hardware reported value for software >> purpose a good thing? > > No. That said, Hao's patch is just making the code clearer; there's > no behavioral change since the "if" will just redo the same > assignments as handle_bus_lock_vmexit. > > Paolo I agree Paolo. EXIT_REASON.bus_lock_detected may or may not be set when exit_reason=EXIT_REASON_BUS_LOCK, It clould depend on hardware implementaion. No matter when intel states it clearly, I think it is better that we avoid repeated assignment by adding additional check condition in vmx_handle_exit. Of course , it is also ok that hand_bus_lock_vmexit do nothing , but the code is not clear, and the code logic will be inconsistent with spec description.
On 23/09/21 02:59, Hao Xiang wrote: > EXIT_REASON.bus_lock_detected may or may not be set when > exit_reason=EXIT_REASON_BUS_LOCK, It clould depend on hardware > implementaion. No matter when intel states it clearly, I think it is > better that we avoid repeated assignment by adding additional check > condition in vmx_handle_exit. Of course , it is also ok that > hand_bus_lock_vmexit do nothing , but the code is not clear, and the > code logic will be inconsistent with spec description. For 5.16 we'll go with something like Sean's sketch, that sets the bus_lock_detected bit on EXIT_REASON_BUS_LOCK. Paolo
On 2021/9/22 22:58, Sean Christopherson wrote: > On Wed, Sep 22, 2021, Xiaoyao Li wrote: >> On 9/22/2021 6:02 PM, Paolo Bonzini wrote: >>> On 18/09/21 13:30, Hao Xiang wrote: >>>> exit_reason.bus_lock_detected is not only set when bus lock VM exit >>>> was preempted, in fact, this bit is always set if bus locks are >>>> detected no matter what the exit_reason.basic is. >>>> >>>> So the bus_lock_vmexit handling in vmx_handle_exit should be duplicated >>>> when exit_reason.basic is EXIT_REASON_BUS_LOCK(74). We can avoid it by >>>> checking if bus lock vmexit was preempted in vmx_handle_exit. >>> I don't understand, does this mean that bus_lock_detected=1 if >>> basic=EXIT_REASON_BUS_LOCK? If so, can we instead replace the contents >>> of handle_bus_lock_vmexit with >>> >>> /* Do nothing and let vmx_handle_exit exit to userspace. */ >>> WARN_ON(!to_vmx(vcpu)->exit_reason.bus_lock_detected); >>> return 0; >>> >>> ? >>> >>> That would be doable only if this is architectural behavior and not a >>> processor erratum, of course. >> EXIT_REASON.bus_lock_detected may or may not be set when exit reason == >> EXIT_REASON_BUS_LOCK. Intel will update ISE or SDM to state it. >> >> Maybe we can do below in handle_bus_lock_vmexit handler: >> >> if (!to_vmx(vcpu)->exit_reason.bus_lock_detected) >> to_vmx(vcpu)->exit_reason.bus_lock_detected = 1; >> >> But is manually changing the hardware reported value for software purpose a >> good thing? > In this case, I'd say yes. Hardware having non-deterministic behavior is the not > good thing, KVM would simply be correctly the not-technically-an-erratum erratum. > > Set it unconditionally and then handle everything in common path. This has the > added advantage of having only one site that deals with KVM_RUN_X86_BUS_LOCK. > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 33f92febe3ce..aa9372452e49 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -5561,9 +5561,9 @@ static int handle_encls(struct kvm_vcpu *vcpu) > > static int handle_bus_lock_vmexit(struct kvm_vcpu *vcpu) > { > - vcpu->run->exit_reason = KVM_EXIT_X86_BUS_LOCK; > - vcpu->run->flags |= KVM_RUN_X86_BUS_LOCK; > - return 0; > + /* The dedicated flag may or may not be set by hardware. /facepalm. */ > + vcpu->exit_reason.bus_lock_detected = true; > + return 1; > } > > /* > @@ -6050,9 +6050,8 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath) > int ret = __vmx_handle_exit(vcpu, exit_fastpath); > > /* > - * Even when current exit reason is handled by KVM internally, we > - * still need to exit to user space when bus lock detected to inform > - * that there is a bus lock in guest. > + * Exit to user space when bus lock detected to inform that there is a > + * bus lock in guest. > */ > if (to_vmx(vcpu)->exit_reason.bus_lock_detected) { > if (ret > 0) I agree with your modifications. And I will re-submit the patch. Thanks.
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 0c2c0d5..5ddf1df 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -6054,7 +6054,8 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath) * still need to exit to user space when bus lock detected to inform * that there is a bus lock in guest. */ - if (to_vmx(vcpu)->exit_reason.bus_lock_detected) { + if (to_vmx(vcpu)->exit_reason.bus_lock_detected && + to_vmx(vcpu)->exit_reason.basic != EXIT_REASON_BUS_LOCK) { if (ret > 0) vcpu->run->exit_reason = KVM_EXIT_X86_BUS_LOCK;
exit_reason.bus_lock_detected is not only set when bus lock VM exit was preempted, in fact, this bit is always set if bus locks are detected no matter what the exit_reason.basic is. So the bus_lock_vmexit handling in vmx_handle_exit should be duplicated when exit_reason.basic is EXIT_REASON_BUS_LOCK(74). We can avoid it by checking if bus lock vmexit was preempted in vmx_handle_exit. Signed-off-by: Hao Xiang <hao.xiang@linux.alibaba.com> --- arch/x86/kvm/vmx/vmx.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)