Re: [RESEND v4 2/2] KVM: VMX: Enable bus lock VM exit

[Chenyi Qiang]

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.