Hi Sean,
Thank you for reviewing my patches.
On 4/23/2025 8:59 PM, Sean Christopherson wrote:
> On Wed, Apr 23, 2025, Manali Shukla wrote:
>> On 4/16/2025 11:30 AM, Xiaoyao Li wrote:
>>>> + if (cpu_feature_enabled(X86_FEATURE_BUS_LOCK_THRESHOLD)) {
>>>> + pr_info("Bus Lock Threshold supported\n");
>>>
>>> It will be printed every time kvm-amd.ko module gets loaded.
>>>
>>> I think it's for your development and debug purpose. Comparing to the
>>> existing features in svm_set_cpu_caps(), nothing makes it special for
>>> BUS_LOCK_THRESHOLD to require a kernel message. So I think we can just
>>> remove it.
>>
>> I didn't add this for development and debug purpose. I added this pr_info()
>> to make it easy to find whether BUS Lock threshold is supported or not from
>> dmesg. I can remove it if you think it is not required.
>
> Please remove it. The user typically doesn't care.
>
>>>> + kvm_caps.has_bus_lock_exit = true;
>>>
>>> Besides, this patch doesn't ensure the bisectability. It allows userspace
>>> to enable KVM_BUS_LOCK_DETECTION_EXIT and set intercept of
>>> INTERCEPT_BUSLOCK but without providing the handler.
>>>
>>> So either move next patch before it or just merge them.
>>>
>>
>> Oh.., my bad, I will move the next patch before this one in v5.
>
> No, do exactly as I suggested in v3.
Sure. I will remove it from v5.
>
> : 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.
>
> I want this (and any other arch collateral I'm missing) in a separate patch so
> that the background on what the hardware feature does is captured. But I see no
> reason to split KVM's implementation into multiple patches.
>
> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> index 2b59b9951c90..d1819c564b1c 100644
> --- a/arch/x86/include/asm/svm.h
> +++ b/arch/x86/include/asm/svm.h
> @@ -116,6 +116,7 @@ enum {
> INTERCEPT_INVPCID,
> INTERCEPT_MCOMMIT,
> INTERCEPT_TLBSYNC,
> + INTERCEPT_BUSLOCK,
> };
>
>
> @@ -158,7 +159,9 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
> u64 avic_physical_id; /* Offset 0xf8 */
> u8 reserved_7[8];
> u64 vmsa_pa; /* Used for an SEV-ES guest */
> - u8 reserved_8[720];
> + u8 reserved_8[16];
> + u16 bus_lock_counter; /* Offset 0x120 */
> + u8 reserved_9[702];
> /*
> * Offset 0x3e0, 32 bytes reserved
> * for use by hypervisor/software.
> diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h
> index 1814b413fd57..abf6aed88cee 100644
> --- a/arch/x86/include/uapi/asm/svm.h
> +++ b/arch/x86/include/uapi/asm/svm.h
> @@ -95,6 +95,7 @@
> #define SVM_EXIT_CR14_WRITE_TRAP 0x09e
> #define SVM_EXIT_CR15_WRITE_TRAP 0x09f
> #define SVM_EXIT_INVPCID 0x0a2
> +#define SVM_EXIT_BUS_LOCK 0x0a5
> #define SVM_EXIT_NPF 0x400
> #define SVM_EXIT_AVIC_INCOMPLETE_IPI 0x401
> #define SVM_EXIT_AVIC_UNACCELERATED_ACCESS 0x402
> @@ -224,6 +225,7 @@
> { SVM_EXIT_CR4_WRITE_TRAP, "write_cr4_trap" }, \
> { SVM_EXIT_CR8_WRITE_TRAP, "write_cr8_trap" }, \
> { SVM_EXIT_INVPCID, "invpcid" }, \
> + { SVM_EXIT_BUS_LOCK, "buslock" }, \
> { SVM_EXIT_NPF, "npf" }, \
> { SVM_EXIT_AVIC_INCOMPLETE_IPI, "avic_incomplete_ipi" }, \
> { SVM_EXIT_AVIC_UNACCELERATED_ACCESS, "avic_unaccelerated_access" }, \
>
Ack.
-Manali
