On 10/6/2024 11:56 AM, Borislav Petkov wrote:

On October 4, 2024 7:33:38 AM GMT+02:00, Manali Shukla manali.shukla@amd.com wrote:

...

Misbehaving guests can cause bus locks to degrade the performance of the system. The Bus Lock Threshold feature can be used to address this issue by providing capability to the hypervisor to limit guest's ability to generate bus lock, thereby preventing system slowdown due to performance penalities.

When the Bus Lock Threshold feature is enabled, the processor checks the bus lock threshold count before executing the buslock and decides whether to trigger bus lock exit or not.

The value of the bus lock threshold count '0' generates bus lock exits, and if the value is greater than '0', the bus lock is executed successfully and the bus lock threshold count is decremented.

Presence of the Bus Lock threshold feature is indicated via CPUID function 0x8000000A_EDX[29].

Signed-off-by: Manali Shukla manali.shukla@amd.com

arch/x86/include/asm/cpufeatures.h | 1 + 1 file changed, 1 insertion(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h index dd4682857c12..77fa8e743ccc 100644 --- a/arch/x86/include/asm/cpufeatures.h +++ b/arch/x86/include/asm/cpufeatures.h @@ -382,6 +382,7 @@ #define X86_FEATURE_V_SPEC_CTRL (15*32+20) /* "v_spec_ctrl" Virtual SPEC_CTRL */ #define X86_FEATURE_VNMI (15*32+25) /* "vnmi" Virtual NMI */ #define X86_FEATURE_SVME_ADDR_CHK (15*32+28) /* SVME addr check */ +#define X86_FEATURE_BUS_LOCK_THRESHOLD (15*32+29) /* "buslock" Bus lock threshold */

Why does this feature flag need to be visible in /proc/cpuinfo?

This was already discussed on [v1] of the same series but never concluded. As suggested in the discussion [v1], I have added "buslock" to be enumerated in "/proc/cpuinfo". I really don't have any strong opinions about adding or removing it from "/proc/cpuinfo". So, I would let maintainers decide how to go about it.

v1: https://lore.kernel.org/kvm/76355a11-a0ba-4a28-bf51-454facfd59e5@amd.com/T/#...

-Manali

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

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