Hi Sean,
Thank you for reviewing my changes.
On 8/17/2024 1:51 AM, Sean Christopherson wrote:
> On Tue, Jul 09, 2024, Manali Shukla wrote:
>> From: Nikunj A Dadhania <nikunj@xxxxxxx>
>>
>> Malicious guests can cause bus locks to degrade the performance of
>> a system. The Bus Lock Threshold feature is beneficial for
>> hypervisors aiming to restrict the ability of the guests to perform
>> excessive bus locks and slow down the system for all the tenants.
>>
>> Add a test case to verify the Bus Lock Threshold feature for SVM.
>>
>> [Manali:
>> - The KVM_CAP_X86_BUS_LOCK_EXIT capability is not enabled while
>> vcpus are created, changed the VM and vCPU creation logic to
>> resolve the mentioned issue.
>> - Added nested guest test case for bus lock exit.
>> - massage commit message.
>> - misc cleanups. ]
>
> Again, 99% of the changelog is boilerplate that does nothing to help me
> understand what the test actually does.
>
Sure. I will rewrite the commit messages for all the patches.
>>
>> Signed-off-by: Nikunj A Dadhania <nikunj@xxxxxxx>
>> Co-developed-by: Manali Shukla <manali.shukla@xxxxxxx>
>> Signed-off-by: Manali Shukla <manali.shukla@xxxxxxx>
>> ---
>> tools/testing/selftests/kvm/Makefile | 1 +
>> .../selftests/kvm/x86_64/svm_buslock_test.c | 114 ++++++++++++++++++
>> 2 files changed, 115 insertions(+)
>> create mode 100644 tools/testing/selftests/kvm/x86_64/svm_buslock_test.c
>>
>> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
>> index ce8ff8e8ce3a..711ec195e386 100644
>> --- a/tools/testing/selftests/kvm/Makefile
>> +++ b/tools/testing/selftests/kvm/Makefile
>> @@ -94,6 +94,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/smaller_maxphyaddr_emulation_test
>> TEST_GEN_PROGS_x86_64 += x86_64/smm_test
>> TEST_GEN_PROGS_x86_64 += x86_64/state_test
>> TEST_GEN_PROGS_x86_64 += x86_64/vmx_preemption_timer_test
>> +TEST_GEN_PROGS_x86_64 += x86_64/svm_buslock_test
>> TEST_GEN_PROGS_x86_64 += x86_64/svm_vmcall_test
>> TEST_GEN_PROGS_x86_64 += x86_64/svm_int_ctl_test
>> TEST_GEN_PROGS_x86_64 += x86_64/svm_nested_shutdown_test
>> diff --git a/tools/testing/selftests/kvm/x86_64/svm_buslock_test.c b/tools/testing/selftests/kvm/x86_64/svm_buslock_test.c
>> new file mode 100644
>> index 000000000000..dcb595999046
>> --- /dev/null
>> +++ b/tools/testing/selftests/kvm/x86_64/svm_buslock_test.c
>
> I would *very* strongly prefer to have a bus lock test that is comment to VMX
> and SVM. For L1, there's no unique behavior. And for L2, assuming we don't
> support nested bus lock enabling, the only vendor specific bits are launching
> L2.
>
> I.e. writing this so it works on both VMX and SVM should be quite straightforward.
>
Sure I will try to write a common test for SVM and VMX.
>> @@ -0,0 +1,114 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * svm_buslock_test
>> + *
>> + * Copyright (C) 2024 Advanced Micro Devices, Inc.
>> + *
>> + * SVM testing: Buslock exit
>
> Keep the Copyright, ditch everything else.
Sure.
>
>> + */
>> +
>> +#include "test_util.h"
>> +#include "kvm_util.h"
>> +#include "processor.h"
>> +#include "svm_util.h"
>> +
>> +#define NO_ITERATIONS 100
>
> Heh, NR_ITERATIONS.
Ack.
>
>> +#define __cacheline_aligned __aligned(128)
>
> Eh, I would just split a page, that's about as future proof as we can get in
> terms of cache line sizes.
>
Sure.
>> +
>> +struct buslock_test {
>> + unsigned char pad[126];
>> + atomic_long_t val;
>> +} __packed;
>> +
>> +struct buslock_test test __cacheline_aligned;
>> +
>> +static __always_inline void buslock_atomic_add(int i, atomic_long_t *v)
>> +{
>> + asm volatile(LOCK_PREFIX "addl %1,%0"
>> + : "+m" (v->counter)
>> + : "ir" (i) : "memory");
>> +}
>> +
>> +static void buslock_add(void)
>> +{
>> + /*
>> + * Increment a cache unaligned variable atomically.
>> + * This should generate a bus lock exit.
>
> So... this test doesn't actually verify that a bus lock exit occurs. The userspace
> side will eat an exit if one occurs, but there's literally not a single TEST_ASSERT()
> in here.
Agreed, How about doing following?
+ for (;;) {
+ struct ucall uc;
+
+ vcpu_run(vcpu);
+
+ if (run->exit_reason == KVM_EXIT_IO) {
+ switch (get_ucall(vcpu, &uc)) {
+ case UCALL_ABORT:
+ REPORT_GUEST_ASSERT(uc);
+ /* NOT REACHED */
+ case UCALL_SYNC:
+ break;
+ case UCALL_DONE:
+ goto done;
+ default:
+ TEST_FAIL("Unknown ucall 0x%lx.", uc.cmd);
+ }
+ }
+
+ TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_X86_BUS_LOCK);
+ TEST_ASSERT_EQ(run->flags, KVM_RUN_X86_BUS_LOCK);
+ run->flags &= ~KVM_RUN_X86_BUS_LOCK;
+ run->exit_reason = 0;
+ }
- Manali
