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

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

Hi Sean,
Thank you for reviewing my patches.

On 8/17/2024 1:24 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
>> system. Non-WB(write-back) and misaligned locked RMW(read-modify-write)
>> instructions are referred to as "bus locks" and require system wide
>> synchronization among all processors to guarantee atomicity.  Bus locks
>> may incur significant performance penalties for all processors in the
>> system.
> 
> Copy+pasting the background into every changelog isn't helpful.  Instead, focus
> on what the feature actually does, and simply mention what bus locks are in
> passing.  If someone really doesn't know, it shouldn't be had for them to find
> the previous changelog.
> 

Sure. I will rewrite the commit messages based on the suggestions.

>> The Bus Lock Threshold feature proves beneficial for hypervisors seeking
>> to restrict guests' ability to initiate numerous bus locks, thereby
>> preventing system slowdowns that affect all tenants.
>>
>> Support for the buslock threshold is indicated via CPUID function
>> 0x8000000A_EDX[29].
>>
>> VMCB intercept bit
>> VMCB Offset	Bits	Function
>> 14h	        5	Intercept bus lock operations
>>                         (occurs after guest instruction finishes)
>>
>> Bus lock threshold
>> VMCB Offset	Bits	Function
>> 120h	        15:0	Bus lock counter
> 
> I can make a pretty educated guess as to how this works, but this is a pretty
> simple feature, i.e. there's no reason not to document how it works in the
> changelog.
>  

Sure.

>> Use the KVM capability KVM_CAP_X86_BUS_LOCK_EXIT to enable the feature.
>>
>> When the bus lock threshold counter reaches to zero, KVM will exit to
>> user space by setting KVM_RUN_BUS_LOCK in vcpu->run->flags in
>> bus_lock_exit handler, indicating that a bus lock has been detected in
>> the guest.
>>
>> 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
>>
>> [Manali:
>>   - Added exit reason string for SVM_EXIT_BUS_LOCK.
>>   - Moved enablement and disablement of bus lock intercept support.
>>     to svm_vcpu_after_set_cpuid().
>>   - Massage commit message.
>>   - misc cleanups.
>> ]
> 
> No need for this since you are listed as co-author.
> 

Ack.

>> 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>
>> ---
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index 7d396f5fa010..9f1d51384eac 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -191,6 +191,9 @@ module_param(pause_filter_count_shrink, ushort, 0444);
>>  static unsigned short pause_filter_count_max = KVM_SVM_DEFAULT_PLE_WINDOW_MAX;
>>  module_param(pause_filter_count_max, ushort, 0444);
>>  
>> +static unsigned short bus_lock_counter = KVM_SVM_DEFAULT_BUS_LOCK_COUNTER;
>> +module_param(bus_lock_counter, ushort, 0644);
> 
> This should be read-only, otherwise the behavior is non-deterministic, e.g. as
> proposed, awon't take effect until a vCPU happens to trigger a bus lock exit.
> 
> If we really want it to be writable, then a per-VM capability is likely a better
> solution.
> 
> Actually, we already have a capability, which means there's zero reason for this
> module param to exist.  Userspace already has to opt-in to turning on bus lock
> detection, i.e. userspace already has the opportunity to provide a different
> threshold.
> 
> That said, unless someone specifically needs a threshold other than '0', I vote
> to keep the uAPI as-is and simply exit on every bus lock.
>  

According to APM [1],
"The VMCB provides a Bus Lock Threshold enable bit and an unsigned 16-bit
Bus Lock Threshold count. On VMRUN, this value 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 executes the bus lock successfully and decrements the count. If the value is 0, the
bus lock is not executed and a #VMEXIT to the VMM is taken."

So, the bus_lock_counter value "0" always results in VMEXIT_BUSLOCK, so the default value of
the bus_lock_counter should be greater or equal to "1".

I can remove the module parameter and initialize the value of bus_lock_counter as "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

-Manali