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