Messages in this thread | | | Subject | Re: [RESEND v4 2/2] KVM: VMX: Enable bus lock VM exit | From | Chenyi Qiang <> | Date | Mon, 26 Oct 2020 13:31:36 +0800 |
| |
On 10/21/2020 6:19 AM, Sean Christopherson wrote: > On Mon, Oct 12, 2020 at 11:35:42AM +0800, Chenyi Qiang wrote: >> @@ -6138,6 +6149,26 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath) >> return 0; >> } >> >> +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. >> + */ >> + if (to_vmx(vcpu)->exit_reason.bus_lock_detected) { >> + if (ret > 0) >> + vcpu->run->exit_reason = KVM_EXIT_BUS_LOCK; >> + else >> + vcpu->run->flags |= KVM_RUN_BUS_LOCK; > > This should always set flags.KVM_RUN_BUS_LOCK, e.g. so that userspace can > always check flags.KVM_RUN_BUS_LOCK instead of having to check both the flag > and the exit reason. As is, it's really bad because the flag is undefined, > which could teach userspace to do the wrong thing. >
Got it.
>> + return 0; >> + } >> + vcpu->run->flags &= ~KVM_RUN_BUS_LOCK; > > Hmm, I feel like explicitly clearing flags is should be unnecessary. By > that, I mean that's it's necessary in the current patch, bit I think we should > figure out how to make that not be the case. With the current approach, every > chunk of code that needs to set a flag also needs to clear it, which increases > the odds of missing a case and ending up with a flag in an undefined state. > > The easiest way I can think of is to add another prep patch that zeros > run->flags at the beginning of kvm_arch_vcpu_ioctl_run(), and changes > post_kvm_run_save() to do: > > if (is_smm(vcpu)) > kvm_run->flags |= KVM_RUN_X86_SMM; > > Then this patch can omit clearing KVM_RUN_BUS_LOCK, and doesn't have to touch > the SMM flag. >
Make sense, I will add the prep patch.
>> + return ret; >> +} >> + >> /* >> * Software based L1D cache flush which is used when microcode providing >> * the cache control MSR is not loaded.
| |