Received-SPF: pass (google.com: domain of linux-kernel+bounces-149286-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 17 Apr 2024 15:37:17 -0700
In-Reply-To: <20240417200849.971433-2-alejandro.j.jimenez@oracle.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240417200849.971433-1-alejandro.j.jimenez@oracle.com> <20240417200849.971433-2-alejandro.j.jimenez@oracle.com>
Message-ID: <ZiBPHVKKnQPYK7Xy@google.com>
Subject: Re: [PATCH 1/2] KVM: x86: Only set APICV_INHIBIT_REASON_ABSENT if
 APICv is enabled
From: Sean Christopherson <seanjc@google.com>
To: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, linux-kernel@vger.kernel.org, 
	joao.m.martins@oracle.com, boris.ostrovsky@oracle.com, 
	suravee.suthikulpanit@amd.com, mlevitsk@redhat.com
Content-Type: text/plain; charset="us-ascii"

On Wed, Apr 17, 2024, Alejandro Jimenez wrote:
> Use the APICv enablement status to determine if APICV_INHIBIT_REASON_ABSENT
> needs to be set, instead of unconditionally setting the reason during
> initialization.
> 
> Specifically, in cases where AVIC is disabled via module parameter or lack
> of hardware support, unconditionally setting an inhibit reason due to the
> absence of an in-kernel local APIC can lead to a scenario where the reason
> incorrectly remains set after a local APIC has been created by either
> KVM_CREATE_IRQCHIP or the enabling of KVM_CAP_IRQCHIP_SPLIT. This is
> because the helpers in charge of removing the inhibit return early if
> enable_apicv is not true, and therefore the bit remains set.
> 
> This leads to confusion as to the cause why APICv is not active, since an
> incorrect reason will be reported by tracepoints and/or a debugging tool
> that examines the currently set inhibit reasons.
> 
> Fixes: ef8b4b720368 ("KVM: ensure APICv is considered inactive if there is no APIC")
> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> ---
>  arch/x86/kvm/x86.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 26288ca05364..eadd88fabadc 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9999,7 +9999,20 @@ static void kvm_apicv_init(struct kvm *kvm)
>  
>  	init_rwsem(&kvm->arch.apicv_update_lock);
>  
> -	set_or_clear_apicv_inhibit(inhibits, APICV_INHIBIT_REASON_ABSENT, true);
> +	/*
> +	 * Unconditionally inhibiting APICv due to the absence of in-kernel
> +	 * local APIC can lead to a scenario where APICV_INHIBIT_REASON_ABSENT
> +	 * remains set in the apicv_inhibit_reasons after a local APIC has been
> +	 * created by either KVM_CREATE_IRQCHIP or the enabling of
> +	 * KVM_CAP_IRQCHIP_SPLIT.
> +	 * Hardware support and module parameters governing APICv enablement
> +	 * have already been evaluated and the initial status is available in
> +	 * enable_apicv, so it can be used here to determine if an inhibit needs
> +	 * to be set.
> +	 */

Eh, this is good changelog material, but I don't think it's not necessary for
a comment.  Readers of this code really should be able to deduce that enable_apicv
can't be toggled on, i.e. DISABLE can't go away.

> +	if (enable_apicv)
> +		set_or_clear_apicv_inhibit(inhibits,
> +					   APICV_INHIBIT_REASON_ABSENT, true);
>  
>  	if (!enable_apicv)
>  		set_or_clear_apicv_inhibit(inhibits,

This can more concisely be:

	enum kvm_apicv_inhibit reason = enable_apicv ? APICV_INHIBIT_REASON_ABSENT :
						       APICV_INHIBIT_REASON_DISABLE;

	set_or_clear_apicv_inhibit(&kvm->arch.apicv_inhibit_reasons, reason, true);

	init_rwsem(&kvm->arch.apicv_update_lock);

which I think also helps the documentation side, e.g. it's shows the VM starts
with either ABSENT *or* DISABLE.

> -- 
> 2.39.3
>