Re: [PATCH v3 4/5] KVM: SVM: Compute DEBUGCTL reserved bits dynamically

[Shivansh Dhiman <shivansh.dhiman@amd.com>]

On 09-07-26 14:14, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] The dynamic reserved bit check for DEBUGCTLMSR_BUS_LOCK_DETECT is dead code because KVM explicitly clears the feature from SVM's supported CPUID capabilities.
> - [High] Hardware silently ignores the guest's Bus Lock Detect configuration on VMRUN unless the guest also enables LBRs, due to AMD SVM requirements for loading DEBUGCTLMSR.
> - [High] Bus Lock Detect support is fundamentally broken because DEBUGCTLMSR state management is strictly gated by unrelated LBR Virtualization parameters and host capabilities.
> --
> 
> --- Patch [4]: [PATCH v3 4/5] KVM: SVM: Compute DEBUGCTL reserved bits dynamically ---
> commit 8b16c8234c21a39b440d6a7688ffcc624702cdf8
> Author: Shivansh Dhiman <shivansh.dhiman@amd.com>
> 
> KVM: SVM: Compute DEBUGCTL reserved bits dynamically
> 
> This patch replaces the static DEBUGCTL_RESERVED_BITS macro with a
> per-vCPU helper that derives reserved bits from the guest's CPUID. This
> allows DEBUGCTLMSR_BUS_LOCK_DETECT when the guest supports it.
> 
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
> 
> [ ... ]
> 
>> @@ -1231,7 +1231,7 @@ void svm_copy_vmrun_state(struct vmcb_save_area *to_save,
>>
>>       if (kvm_cpu_cap_has(X86_FEATURE_LBRV)) {
>>               svm_copy_lbrs(to_save, from_save);
>> -             to_save->dbgctl &= ~DEBUGCTL_RESERVED_BITS;
>> +             to_save->dbgctl &= ~svm_get_debugctl_reserved_bits(vcpu);
> 
> [Severity: High]
> Does Bus Lock Detect incorrectly depend on LBR Virtualization being supported
> and enabled?

This dependency is architectural, and has to be honored.

> 
> Here, the guest's dbgctl is only copied if the host supports X86_FEATURE_LBRV.
> 
> Similarly, in svm_set_msr(), writes to MSR_IA32_DEBUGCTLMSR are unconditionally
> rejected if the lbrv module parameter is disabled:
> 
> svm_set_msr() {
>         ...
>         case MSR_IA32_DEBUGCTLMSR:
>                 if (!lbrv) {
>                         kvm_pr_unimpl_wrmsr(vcpu, ecx, data);
>                         break;
>                 }
>         ...
> }
> 
> And earlier in nested_vmcb02_prepare_save(), L2's dbgctl is replaced with
> L1's dbgctl if L1 does not explicitly enable LBRV for L2:
> 
> nested_vmcb02_prepare_save() {
>         ...
>         if (nested_vmcb12_has_lbrv(vcpu)) {
>                 ...
>         } else {
>                 svm_copy_lbrs(&vmcb02->save, &vmcb01->save);
>         }
>         ...
> }
> 
> Will this break Bus Lock Detect for guests when LBR features are unavailable
> or disabled?

[...]

> 
>>       }
>>  }
> 
>> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
>> --- a/arch/x86/kvm/svm/svm.h
>> +++ b/arch/x86/kvm/svm/svm.h
> 
> [ ... ]
> 
>> @@ -783,7 +783,15 @@ BUILD_SVM_MSR_BITMAP_HELPERS(bool, test, test)
>>  BUILD_SVM_MSR_BITMAP_HELPERS(void, clear, __clear)
>>  BUILD_SVM_MSR_BITMAP_HELPERS(void, set, __set)
>>
>> -#define DEBUGCTL_RESERVED_BITS (~DEBUGCTLMSR_LBR)
>> +static inline u64 svm_get_debugctl_reserved_bits(struct kvm_vcpu *vcpu)
>> +{
>> +     u64 debugctl = DEBUGCTLMSR_LBR;
>> +
>> +     if (guest_cpu_cap_has(vcpu, X86_FEATURE_BUS_LOCK_DETECT))
>> +             debugctl |= DEBUGCTLMSR_BUS_LOCK_DETECT;
> 
> [Severity: High]
> Is it possible for a guest to actually have this feature under SVM?
> 
> In svm_set_cpu_caps(), KVM unconditionally clears the feature:
> 
> svm_set_cpu_caps() {
>         ...
>         kvm_cpu_cap_clear(X86_FEATURE_BUS_LOCK_DETECT);
>         ...
> }
> 
> Because of this, guest_cpu_cap_has() will always evaluate to false,
> rendering this new logic dead code. Guest writes to enable the feature
> will inject a #GP.
> 
> [Severity: High]
> If the dead-code issue above is fixed and KVM accepts the write to
> DEBUGCTLMSR_BUS_LOCK_DETECT, will the hardware silently ignore the guest's
> configuration on VMRUN unless the guest also enables LBRs?
> 
> AMD SVM only loads DEBUGCTLMSR from the VMCB save area during VMRUN if LBR
> Virtualization is enabled. In svm_update_lbrv(), KVM disables V_LBR if the
> LBR bit is not set:
> 
> svm_update_lbrv() {
>         ...
>         if (svm->vmcb->save.dbgctl & DEBUGCTLMSR_LBR)
>                 enable_lbrv = true;
>         ...
>         else if (!enable_lbrv && current_enable_lbrv)
>                 __svm_disable_lbrv(vcpu);
>         ...
> }
> 
> Could this create a state desynchronization where KVM accepts the feature
> but the CPU ignores it?
> 

All these issues stem from AI reviewing the patch in isolation. The Bus Lock Detect
feature in SVM is enabled in 5th patch of this series, so, the hunks in this patch
are not dead code.

>> +
>> +     return ~debugctl;
>> +}
> 
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260709082953.69434-1-shivansh.dhiman@amd.com?part=4