lkml.org 
[lkml]   [2024]   [Apr]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/2] KVM: x86: Only set APICV_INHIBIT_REASON_ABSENT if APICv is enabled
From


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

\
 
 \ /
  Last update: 2024-04-18 03:16    [W:0.072 / U:0.096 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site