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

From: Nikunj A Dadhania <nikunj@amd.com>

Virtual machines can exploit bus locks to degrade the performance of
the system. Bus locks can be caused by Non-WB(Write back) and
misaligned locked RMW (Read-modify-Write) instructions and require
systemwide synchronization among all processors which can result into
significant performance penalties.

To address this issue, the Bus Lock Threshold feature is introduced to
provide ability to hypervisor to restrict guests' capability of
initiating mulitple buslocks, thereby preventing system slowdowns.

Support for the buslock threshold is indicated via CPUID function
0x8000000A_EDX[29].

On the processors that support the Bus Lock Threshold feature, the
VMCB provides a Bus Lock Threshold enable bit and an unsigned 16-bit
Bus Lock threshold count.

VMCB intercept bit
VMCB Offset	Bits	Function
14h	        5	Intercept bus lock operations

Bus lock threshold count
VMCB Offset	Bits	Function
120h	        15:0	Bus lock counter

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.

This implementation is set up to intercept every guest bus lock. The
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
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].

[1]: AMD64 Architecture Programmer's Manual Pub. 24593, April 2024,
     Vol 2, 15.14.5 Bus Lock Threshold.
     https://bugzilla.kernel.org/attachment.cgi?id=306250

Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
Co-developed-by: Manali Shukla <manali.shukla@amd.com>
Signed-off-by: Manali Shukla <manali.shukla@amd.com>
---
 arch/x86/include/asm/svm.h      |  5 ++++-
 arch/x86/include/uapi/asm/svm.h |  2 ++
 arch/x86/kvm/svm/nested.c       | 10 ++++++++++
 arch/x86/kvm/svm/svm.c          | 27 +++++++++++++++++++++++++++
 4 files changed, 43 insertions(+), 1 deletion(-)

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.
 
> 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.
	 */

> +	 * 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
>

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
>>

On 10/15/2024 11:19 PM, Sean Christopherson wrote:
> On Fri, Oct 04, 2024, Manali Shukla wrote:
...
>>  
>> +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.
> 	 */
> 

The bus lock threshold intercept feature is available for SEV-ES and SEV-SNP
guests too. The rip where the bus lock exit occurred, is not available in
bus_lock_exit handler for SEV-ES and SEV-SNP guests, so the above-mentioned
solution won't work with SEV-ES and SEV-SNP guests.

I would propose to add the above-mentioned solution only for normal and SEV guests
and unconditionally reloading of bus_lock_counter to 1 in complete_userspace_io
for SEV-ES and SEV-SNP guests.

Any thoughts ?


>> +	 * 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
>>
 -Manali

On Sun, Nov 03, 2024, Manali Shukla wrote:
> On 10/15/2024 11:19 PM, Sean Christopherson wrote:
> > On Fri, Oct 04, 2024, Manali Shukla wrote:
> ...
> >>  
> >> +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.
> > 	 */
> > 
> 
> The bus lock threshold intercept feature is available for SEV-ES and SEV-SNP
> guests too. The rip where the bus lock exit occurred, is not available in
> bus_lock_exit handler for SEV-ES and SEV-SNP guests, so the above-mentioned
> solution won't work with SEV-ES and SEV-SNP guests.
> 
> I would propose to add the above-mentioned solution only for normal and SEV guests
> and unconditionally reloading of bus_lock_counter to 1 in complete_userspace_io
> for SEV-ES and SEV-SNP guests.

Yeah, that works.  Though I would condition the check on guest_state_protected.
Actually, and this is going to seem really stupid, but everything will Just Work
if you use kvm_get_linear_rip() and kvm_is_linear_rip(), because kvm_get_linear_rip()
returns '0' for vCPUs with protected state.  I.e. KVM will do a rather superfluous
cui() callback, but otherwise it's fine.  Silly, but in many ways preferable to
special casing ES and SNP guests.

On a related topic, can you add a refacotring prep patch to move linear_rip out
of kvm_pio_request and place it next to complete_userspace_io?  There's nothing
port I/O specific about that field, it just so happens to that port I/O is the
only case where KVM's ABI is to let userspace stuff state (to emulate RESET)
without first completing the I/O instruction.

I.e.

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 8e8ca6dab2b2..8617b15096a6 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -406,7 +406,6 @@ struct kvm_rmap_head {
 };
 
 struct kvm_pio_request {
-       unsigned long linear_rip;
        unsigned long count;
        int in;
        int port;
@@ -884,6 +883,7 @@ struct kvm_vcpu_arch {
        bool emulate_regs_need_sync_to_vcpu;
        bool emulate_regs_need_sync_from_vcpu;
        int (*complete_userspace_io)(struct kvm_vcpu *vcpu);
+       unsigned long cui_linear_rip;
 
        gpa_t time;
        struct pvclock_vcpu_time_info hv_clock;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 425a301911a6..7704d3901481 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9308,7 +9308,7 @@ static int complete_fast_pio_out(struct kvm_vcpu *vcpu)
 {
        vcpu->arch.pio.count = 0;
 
-       if (unlikely(!kvm_is_linear_rip(vcpu, vcpu->arch.pio.linear_rip)))
+       if (unlikely(!kvm_is_linear_rip(vcpu, vcpu->arch.cui_linear_rip)))
                return 1;
 
        return kvm_skip_emulated_instruction(vcpu);
@@ -9333,7 +9333,7 @@ static int kvm_fast_pio_out(struct kvm_vcpu *vcpu, int size,
                        complete_fast_pio_out_port_0x7e;
                kvm_skip_emulated_instruction(vcpu);
        } else {
-               vcpu->arch.pio.linear_rip = kvm_get_linear_rip(vcpu);
+               vcpu->arch.cui_linear_rip = kvm_get_linear_rip(vcpu);
                vcpu->arch.complete_userspace_io = complete_fast_pio_out;
        }
        return 0;
@@ -9346,7 +9346,7 @@ static int complete_fast_pio_in(struct kvm_vcpu *vcpu)
        /* We should only ever be called with arch.pio.count equal to 1 */
        BUG_ON(vcpu->arch.pio.count != 1);
 
-       if (unlikely(!kvm_is_linear_rip(vcpu, vcpu->arch.pio.linear_rip))) {
+       if (unlikely(!kvm_is_linear_rip(vcpu, vcpu->arch.cui_linear_rip))) {
                vcpu->arch.pio.count = 0;
                return 1;
        }
@@ -9375,7 +9375,7 @@ static int kvm_fast_pio_in(struct kvm_vcpu *vcpu, int size,
                return ret;
        }
 
-       vcpu->arch.pio.linear_rip = kvm_get_linear_rip(vcpu);
+       vcpu->arch.cui_linear_rip = kvm_get_linear_rip(vcpu);
        vcpu->arch.complete_userspace_io = complete_fast_pio_in;
 
        return 0;

On 11/5/2024 7:52 AM, Sean Christopherson wrote:
> On Sun, Nov 03, 2024, Manali Shukla wrote:
>> On 10/15/2024 11:19 PM, Sean Christopherson wrote:
>>> On Fri, Oct 04, 2024, Manali Shukla wrote:
>> ...
>>>>  
>>>> +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.
>>> 	 */
>>>
>>
>> The bus lock threshold intercept feature is available for SEV-ES and SEV-SNP
>> guests too. The rip where the bus lock exit occurred, is not available in
>> bus_lock_exit handler for SEV-ES and SEV-SNP guests, so the above-mentioned
>> solution won't work with SEV-ES and SEV-SNP guests.
>>
>> I would propose to add the above-mentioned solution only for normal and SEV guests
>> and unconditionally reloading of bus_lock_counter to 1 in complete_userspace_io
>> for SEV-ES and SEV-SNP guests.
> 
> Yeah, that works.  Though I would condition the check on guest_state_protected.
> Actually, and this is going to seem really stupid, but everything will Just Work
> if you use kvm_get_linear_rip() and kvm_is_linear_rip(), because kvm_get_linear_rip()
> returns '0' for vCPUs with protected state.  I.e. KVM will do a rather superfluous
> cui() callback, but otherwise it's fine.  Silly, but in many ways preferable to
> special casing ES and SNP guests.

Ack.

> 
> On a related topic, can you add a refacotring prep patch to move linear_rip out
> of kvm_pio_request and place it next to complete_userspace_io?  There's nothing
> port I/O specific about that field, it just so happens to that port I/O is the
> only case where KVM's ABI is to let userspace stuff state (to emulate RESET)
> without first completing the I/O instruction.
> 
Sure. I will add this refactoring prep patch with v4.

- Manali


> I.e.
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 8e8ca6dab2b2..8617b15096a6 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -406,7 +406,6 @@ struct kvm_rmap_head {
>  };
>  
>  struct kvm_pio_request {
> -       unsigned long linear_rip;
>         unsigned long count;
>         int in;
>         int port;
> @@ -884,6 +883,7 @@ struct kvm_vcpu_arch {
>         bool emulate_regs_need_sync_to_vcpu;
>         bool emulate_regs_need_sync_from_vcpu;
>         int (*complete_userspace_io)(struct kvm_vcpu *vcpu);
> +       unsigned long cui_linear_rip;
>  
>         gpa_t time;
>         struct pvclock_vcpu_time_info hv_clock;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 425a301911a6..7704d3901481 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9308,7 +9308,7 @@ static int complete_fast_pio_out(struct kvm_vcpu *vcpu)
>  {
>         vcpu->arch.pio.count = 0;
>  
> -       if (unlikely(!kvm_is_linear_rip(vcpu, vcpu->arch.pio.linear_rip)))
> +       if (unlikely(!kvm_is_linear_rip(vcpu, vcpu->arch.cui_linear_rip)))
>                 return 1;
>  
>         return kvm_skip_emulated_instruction(vcpu);
> @@ -9333,7 +9333,7 @@ static int kvm_fast_pio_out(struct kvm_vcpu *vcpu, int size,
>                         complete_fast_pio_out_port_0x7e;
>                 kvm_skip_emulated_instruction(vcpu);
>         } else {
> -               vcpu->arch.pio.linear_rip = kvm_get_linear_rip(vcpu);
> +               vcpu->arch.cui_linear_rip = kvm_get_linear_rip(vcpu);
>                 vcpu->arch.complete_userspace_io = complete_fast_pio_out;
>         }
>         return 0;
> @@ -9346,7 +9346,7 @@ static int complete_fast_pio_in(struct kvm_vcpu *vcpu)
>         /* We should only ever be called with arch.pio.count equal to 1 */
>         BUG_ON(vcpu->arch.pio.count != 1);
>  
> -       if (unlikely(!kvm_is_linear_rip(vcpu, vcpu->arch.pio.linear_rip))) {
> +       if (unlikely(!kvm_is_linear_rip(vcpu, vcpu->arch.cui_linear_rip))) {
>                 vcpu->arch.pio.count = 0;
>                 return 1;
>         }
> @@ -9375,7 +9375,7 @@ static int kvm_fast_pio_in(struct kvm_vcpu *vcpu, int size,
>                 return ret;
>         }
>  
> -       vcpu->arch.pio.linear_rip = kvm_get_linear_rip(vcpu);
> +       vcpu->arch.cui_linear_rip = kvm_get_linear_rip(vcpu);
>         vcpu->arch.complete_userspace_io = complete_fast_pio_in;
>  
>         return 0;