Re: [PATCH v3 2/4] KVM: SVM: Enable Bus lock threshold exit

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sean,

Thanks for reviewing my patches.

On 10/15/2024 11:19 PM, Sean Christopherson wrote:
> On Fri, Oct 04, 2024, Manali Shukla wrote:
>> When a VMRUN instruction is executed, the bus lock threshold count is
>> loaded into an internal count register. Before the processor executes
>> a bus lock in the guest, it checks the value of this register:
>>
>>  - If the value is greater than '0', the processor successfully
>>    executes the bus lock and decrements the count.
>>
>>  - If the value is '0', the bus lock is not executed, and a #VMEXIT to
>>    the VMM is taken.
>>
>> The bus lock threshold #VMEXIT is reported to the VMM with the VMEXIT
>> code A5h, SVM_EXIT_BUS_LOCK.
> 
> I vote to split this into two patches: one to add the architectural collateral,
> with the above as the changelog, and a second to actually implement support in
> KVM.  Having the above background is useful, but it makes it quite hard to find
> information on the KVM design and implementation.
>  

Sure. I will split it into 2 patches.

>> This implementation is set up to intercept every guest bus lock.
> 
> "This implementation" is a variation on "This patch".  Drop it, and simply state
> what the patch is doing.
> 
>> initial value of the Bus Lock Threshold counter is '0' in the VMCB, so
>> the very first bus lock will exit, set the Bus Lock Threshold counter
>> to '1' (so that the offending instruction can make forward progress)
>> but then the value is at '0' again so the next bus lock will exit.
>>
>> The generated SVM_EXIT_BUS_LOCK in kvm will exit to user space by
> 
> s/kvm/KVM
> 
> And again, the tone is wrong.
> 
> Something is what I would aim for:
> 
>   Add support for KVM_CAP_X86_BUS_LOCK_EXIT on SVM CPUs with Bus Lock
>   Threshold, which is close enough to VMX's Bus Lock Detection VM-Exit to
>   allow reusing KVM_CAP_X86_BUS_LOCK_EXIT.
> 
>   The biggest difference between the two features is that Threshold is
>   fault-like, whereas Detection is trap-like.  To allow the guest to make
>   forward progress, Threshold provides a per-VMCB counter which is
>   decremented every time a bus lock occurs, and a VM-Exit is triggered if
>   and only if the counter is '0'.
> 
>   To provide Detection-like semantics, initialize the counter to '0', i.e.
>   exit on every bus lock, and when re-executing the guilty instruction, set
>   the counter to '1' to effectively step past the instruction.
> 
>   Note, in the unlikely scenario that re-executing the instruction doesn't
>   trigger a bus lock, e.g. because the guest has changed memory types or
>   patched the guilty instruction, the bus lock counter will be left at '1',
>   i.e. the guest will be able to do a bus lock on a different instruction.
>   In a perfect world, KVM would ensure the counter is '0' if the guest has
>   made forward progress, e.g. if RIP has changed.  But trying to close that
>   hole would incur non-trivial complexity, for marginal benefit; the intent
>   of KVM_CAP_X86_BUS_LOCK_EXIT is to allow userspace rate-limit bus locks,
>   not to allow for precise detection of problematic guest code.  And, it's
>   simply not feasible to fully close the hole, e.g. if an interrupt arrives
>   before the original instruction can re-execute, the guest could step past
>   a different bus lock.
> 
>> setting the KVM_RUN_BUS_LOCK flag in vcpu->run->flags, indicating to
>> the user space that a bus lock has been detected in the guest.
>>
>> Use the already available KVM capability KVM_CAP_X86_BUS_LOCK_EXIT to
>> enable the feature. This feature is disabled by default, it can be
>> enabled explicitly from user space.
>>
>> More details about the Bus Lock Threshold feature can be found in AMD
>> APM [1].
> 
> ...
> 
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index d5314cb7dff4..ca1c42201894 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
>> @@ -669,6 +669,11 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm,
>>  	vmcb02->control.iopm_base_pa = vmcb01->control.iopm_base_pa;
>>  	vmcb02->control.msrpm_base_pa = vmcb01->control.msrpm_base_pa;
>>  
>> +	/*
>> +	 * The bus lock threshold count should keep running across nested
>> +	 * transitions. Copy the buslock threshold count from vmcb01 to vmcb02.
> 
> No, it should be reset to '0'.  The bus lock can't have been on VMRUN, because KVM
> is emulating the VMRUN.  That leaves two possibilities: the bus lock happened in
> L1 on an instruction before VMRUN, or the bus lock happened in _an_ L2, before a
> nested VM-Exit to L1 occurred.
> 
> In the first case, the bus lock firmly happened on an instruction in the past.
> Even if vmcb01's counter is still '1', e.g. because the guilty instruction got
> patched, the vCPU has clearly made forward progress and so KVM should reset vmcb02's
> counter to '0'.
> 
> In the second case, KVM has no idea if L2 has made forward progress.  The only
> way to _try to_ detect if L2 has made forward progress would to be to track the
> counter on a per-vmcb12 basis, but even that is flawed because KVM doesn't have
> visibility into L1's management of L2.
>  
> I do think we may need to stash vmcb02's counter though.  E.g. if userspace rate-
> limits the vCPU, then it's entirely possible that L1's tick interrupt is pending
> by the time userspace re-runs the vCPU.  If KVM unconditionally clears the counter
> on VMRUN, then when L1 re-enters L2, the same instruction will trigger a VM-Exit
> and the entire cycle starts over.
> 
> Anything we do is going to be imperfect, but the best idea I can come up with is
> also relatively simple, especially in conjunction with my suggestion below.  If
> KVM tracks the RIP that triggered the bus lock, then on nested VM-Exit KVM can
> propagate that RIP into svm_nested_state as appropriate.  E.g.
> 
> 	if (vmcb02->control.bus_lock_counter &&
> 	    svm->bus_lock_rip == vmcb02->save.rip)
> 		svm->nested.bus_lock_rip = svm->bus_lock_rip;
> 	else
> 		svm->nested.bus_lock_rip = INVALID_GVA; /* or '0', as much as that bugs me */
> 
> and then on nested VMRUN
> 
> 	if (svm->nested.bus_lock_rip == vmcb02->save.rip) {
> 		vmcb02->control.bus_lock_counter = 1;
> 		svm->bus_lock_rip = svm->nested.bus_lock_rip;
> 	} else {
> 		vmcb02->control.bus_lock_counter = 0;
> 	}
> 
> 	svm->nested.bus_lock_rip = INVALID_GVA;
> 
> Again, it's imperfect, e.g. if L1 happens to run a different vCPU at the same
> RIP, then KVM will allow a bus lock for the wrong vCPU.  But the odds of that
> happening are absurdly low unless L1 is deliberately doing weird things, and so
> I don't think
> 
>> +	 */
>> +	vmcb02->control.bus_lock_counter = vmcb01->control.bus_lock_counter;
>>  	/* Done at vmrun: asid.  */
>>  
>>  	/* Also overwritten later if necessary.  */
>> @@ -1035,6 +1040,11 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
>>  
>>  	}
>>  
>> +	/*
>> +	 * The bus lock threshold count should keep running across nested
>> +	 * transitions. Copy the buslock threshold count from vmcb02 to vmcb01.
>> +	 */
>> +	vmcb01->control.bus_lock_counter = vmcb02->control.bus_lock_counter;
> 
> KVM should definitely reset the counter to '0' on a nested VM-Exit, because L1
> can't be interrupted by L2, i.e. there is zero chance that KVM needs to allow a
> bus lock in L1 to ensure L1 makes forward progress.
> 
>>  	nested_svm_copy_common_state(svm->nested.vmcb02.ptr, svm->vmcb01.ptr);
>>  
>>  	svm_switch_vmcb(svm, &svm->vmcb01);
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index 9df3e1e5ae81..0d0407f1ee6a 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -1372,6 +1372,9 @@ static void init_vmcb(struct kvm_vcpu *vcpu)
>>  		svm->vmcb->control.int_ctl |= V_GIF_ENABLE_MASK;
>>  	}
>>  
>> +	if (vcpu->kvm->arch.bus_lock_detection_enabled)
>> +		svm_set_intercept(svm, INTERCEPT_BUSLOCK);
>> +
>>  	if (sev_guest(vcpu->kvm))
>>  		sev_init_vmcb(svm);
>>  
>> @@ -3286,6 +3289,24 @@ static int invpcid_interception(struct kvm_vcpu *vcpu)
>>  	return kvm_handle_invpcid(vcpu, type, gva);
>>  }
>>  
>> +static int bus_lock_exit(struct kvm_vcpu *vcpu)
>> +{
>> +	struct vcpu_svm *svm = to_svm(vcpu);
>> +
>> +	vcpu->run->exit_reason = KVM_EXIT_X86_BUS_LOCK;
>> +	vcpu->run->flags |= KVM_RUN_X86_BUS_LOCK;
>> +
>> +	/*
>> +	 * Reload the counter with value greater than '0'.
> 
> The value quite obviously must be exactly '1', not simply greater than '0.  I also
> think this is the wrong place to set the counter.  Rather than set the counter at
> the time of exit, KVM should implement a vcpu->arch.complete_userspace_io callback
> and set the counter to '1' if and only if RIP (or LIP, but I have no objection to
> keeping things simple) is unchanged.  It's a bit of extra complexity, but it will
> make it super obvious why KVM is setting the counter to '1'.  And, if userspace
> wants to stuff state and move past the instruction, e.g. by emulating the guilty
> instruction, then KVM won't unnecessarily allow a bus lock in the guest.
> 
> And then the comment can be:
> 
> 	/*
> 	 * If userspace has NOT change RIP, then KVM's ABI is to let the guest
> 	 * execute the bus-locking instruction.  Set the bus lock counter to '1'
> 	 * to effectively step past the bus lock.
> 	 */
> 

Thank you for highlighting these scenarios (for nested guest and normal guests). 
I had not thought about them. I’m currently going through the comments
and trying to fully understand them. I’ll try out the suggested changes and
get back to you.

- Manali


>> +	 * The bus lock exit on SVM happens with RIP pointing to the guilty
>> +	 * instruction. So, reloading the value of bus_lock_counter to '0'
>> +	 * results into generating continuous bus lock exits.
>> +	 */
>> +	svm->vmcb->control.bus_lock_counter = 1;
>> +
>> +	return 0;
>> +}
>> +
>>  static int (*const svm_exit_handlers[])(struct kvm_vcpu *vcpu) = {
>>  	[SVM_EXIT_READ_CR0]			= cr_interception,
>>  	[SVM_EXIT_READ_CR3]			= cr_interception,
>> @@ -3353,6 +3374,7 @@ static int (*const svm_exit_handlers[])(struct kvm_vcpu *vcpu) = {
>>  	[SVM_EXIT_CR4_WRITE_TRAP]		= cr_trap,
>>  	[SVM_EXIT_CR8_WRITE_TRAP]		= cr_trap,
>>  	[SVM_EXIT_INVPCID]                      = invpcid_interception,
>> +	[SVM_EXIT_BUS_LOCK]			= bus_lock_exit,
>>  	[SVM_EXIT_NPF]				= npf_interception,
>>  	[SVM_EXIT_RSM]                          = rsm_interception,
>>  	[SVM_EXIT_AVIC_INCOMPLETE_IPI]		= avic_incomplete_ipi_interception,
>> @@ -5227,6 +5249,11 @@ static __init void svm_set_cpu_caps(void)
>>  		kvm_cpu_cap_set(X86_FEATURE_SVME_ADDR_CHK);
>>  	}
>>  
>> +	if (cpu_feature_enabled(X86_FEATURE_BUS_LOCK_THRESHOLD)) {
>> +		pr_info("Bus Lock Threashold supported\n");
>> +		kvm_caps.has_bus_lock_exit = true;
>> +	}
>> +
>>  	/* CPUID 0x80000008 */
>>  	if (boot_cpu_has(X86_FEATURE_LS_CFG_SSBD) ||
>>  	    boot_cpu_has(X86_FEATURE_AMD_SSBD))
>> -- 
>> 2.34.1
>>





[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux