Re: [PATCH 41/60] kvm: x86: Make apic_map per plane

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Per-plane APIC map recalculation erroneously overwrites VM-global APICv inhibit state, enabling APICv unsafely.
--

commit ed523cb8e92284e8a72c3abcbafa7f4ecb34091f
Author: Paolo Bonzini <pbonzini@redhat.com>

    KVM: x86: Move apic_map to per-plane state
    
    This commit moves the local APIC map and related dirty tracking from KVM
    VM-wide state into the per-plane structure.

> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 1b763f164951..06a12b49fafa 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -403,36 +403,37 @@ enum {
>  	DIRTY
>  };
>  
> -static void kvm_recalculate_apic_map(struct kvm *kvm)
> +static void kvm_recalculate_apic_map(struct kvm_plane *plane)
>  {
>  	struct kvm_apic_map *new, *old = NULL;
> +	struct kvm *kvm = plane->kvm;

[ ... ]

> @@ -498,16 +499,16 @@ static void kvm_recalculate_apic_map(struct kvm *kvm)
>  	else
>  		kvm_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_APIC_ID_MODIFIED);
>  

[Severity: High]
Does this per-plane recalculation erroneously overwrite the VM-global APICv
inhibit state?

Since APICv inhibit flags (like APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED) are
global per-VM state stored in kvm->arch.apicv_inhibit_reasons, unconditionally
clearing them when a single plane doesn't have aliases might be unsafe.

If Plane 0 correctly inhibits APICv due to aliased IDs, and a subsequent
recalculation occurs on Plane 1 (which lacks aliases), would this
kvm_clear_apicv_inhibit() call globally clear the inhibit flag?

This could improperly re-enable hardware APIC virtualization for the entire VM
while Plane 0 is still in an invalid aliased state, leading to incorrect
interrupt delivery and potential hardware state corruption.

> -	old = rcu_dereference_protected(kvm->arch.apic_map,
> -			lockdep_is_held(&kvm->arch.apic_map_lock));
> -	rcu_assign_pointer(kvm->arch.apic_map, new);
> +	old = rcu_dereference_protected(plane->arch.apic_map,
> +			lockdep_is_held(&plane->arch.apic_map_lock));
> +	rcu_assign_pointer(plane->arch.apic_map, new);
>  	/*
> -	 * Write kvm->arch.apic_map before clearing apic->apic_map_dirty.
> +	 * Write kvm->arch.apic_map before clearing plane->apic_map_dirty.
>  	 * If another update has come in, leave it DIRTY.
>  	 */
> -	atomic_cmpxchg_release(&kvm->arch.apic_map_dirty,
> +	atomic_cmpxchg_release(&plane->arch.apic_map_dirty,
>  			       UPDATE_IN_PROGRESS, CLEAN);
> -	mutex_unlock(&kvm->arch.apic_map_lock);
> +	mutex_unlock(&plane->arch.apic_map_lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608144252.351443-1-joro@8bytes.org?part=41