Re: [RFC PATCH v1 4/4] KVM: selftests: Add bus lock exit test

[Manali Shukla <manali.shukla@amd.com>]

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@amd.com>
>>
>> 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@amd.com>
>> Co-developed-by: Manali Shukla <manali.shukla@amd.com>
>> Signed-off-by: Manali Shukla <manali.shukla@amd.com>
>> ---
>>  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