Re: [PATCH 1/1] KVM: x86: lapic: update apic_base and APIC ID together

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Lock inversion (ABBA deadlock) between `cpus_read_lock()` and `apic_map_lock`.
--

--- Patch [1]: [PATCH 1/1] KVM: x86: lapic: update apic_base and APIC ID together ---
commit 01695608c13ffe0ad80fe486aaf7e61b96936d9e
Author: Dongli Zhang <dongli.zhang@oracle.com>

KVM: x86: lapic: update apic_base and APIC ID together

This commit aims to resolve a runtime apicv inhibition issue by atomically
updating apic_base and the APIC ID. It introduces conditional lock protection
using apic_map_lock within __kvm_apic_set_base() to serialize the updates
with APIC map recalculation.

> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2793,17 +2793,29 @@ static void __kvm_apic_set_base(struct kvm_vcpu *vcpu, u64 value)
>  {
>  	u64 old_value = vcpu->arch.apic_base;
>  	struct kvm_lapic *apic = vcpu->arch.apic;
> +	u64 changed = old_value ^ value;
> +	bool apicbase_enable_changed = changed & MSR_IA32_APICBASE_ENABLE;
> +	bool x2apic_enable_changed = changed & X2APIC_ENABLE;
> +	bool apic_mode_changed = apicbase_enable_changed || x2apic_enable_changed;
> +	bool need_lock = apic && apic_mode_changed;
> +
> +	/*
> +	 * Serialize apic_base and APIC ID updates with APIC map
> +	 * recalculation.
> +	 */
> +	if (need_lock)
> +		mutex_lock(&vcpu->kvm->arch.apic_map_lock);
>  
>  	vcpu->arch.apic_base = value;
>  
> [ ... ]
>
>  	/* update jump label if enable bit changes */
> -	if ((old_value ^ value) & MSR_IA32_APICBASE_ENABLE) {
> +	if (apicbase_enable_changed) {
>  		if (value & MSR_IA32_APICBASE_ENABLE) {
>  			kvm_apic_set_xapic_id(apic, vcpu->vcpu_id);
>  			static_branch_slow_dec_deferred(&apic_hw_disabled);
>  			/* Check if there are APF page ready requests pending */
>  			kvm_make_request(KVM_REQ_APF_READY, vcpu);
>  		} else {
>  			static_branch_inc(&apic_hw_disabled.key);

[Severity: High]
Does this introduce a lock inversion between apic_map_lock and
cpus_read_lock()?

By acquiring apic_map_lock at the beginning of __kvm_apic_set_base() and
calling static_branch_inc() here, we might be taking the global
cpus_read_lock() while holding a per-VM lock.

Because static_branch_inc() internally calls static_key_slow_inc(), which
unconditionally acquires cpus_read_lock(), this establishes a dependency of:
apic_map_lock -> cpus_read_lock()

However, CPU hotplug operations (like kvm_online_cpu()) take KVM locks under
cpus_write_lock(). Since KVM commonly acquires vcpu->mutex under kvm_lock,
and apic_map_lock is nested further inside vcpu->mutex, this appears to
create a circular dependency:

cpus_read_lock() -> kvm_lock -> vcpu->mutex -> apic_map_lock

Can a guest triggering this path by clearing the APIC enable bit via WRMSR,
while racing with a host CPU hotplug operation, cause an ABBA deadlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818234238.618158-1-dongli.zhang@oracle.com?part=1