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

[Alejandro Jimenez <alejandro.j.jimenez@oracle.com>]

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

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