Re: [PATCH 1/2] KVM: x86: Only set APICV_INHIBIT_REASON_ABSENT if APICv is enabled

[Alejandro Jimenez]

On 4/17/24 18:37, Sean Christopherson wrote:
> 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@xxxxxxxxxx>
> > ---
> >   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.

ACK, I'll remove the comment block.

> > +	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.

I initially had combined the checks (using a less elegant if/else), but didn't want
to convey that these two inhibits were mutually exclusive. But as you point out
that is exactly what REASON_DISABLE is with respect to all the other inhibits.

I'll send v2 with the changes.

Thank you,
Alejandro


> > --
> > 2.39.3