Messages in this thread | | | Date | Wed, 17 Apr 2024 21:16:17 -0400 | Subject | Re: [PATCH 1/2] KVM: x86: Only set APICV_INHIBIT_REASON_ABSENT if APICv is enabled | From | 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@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 >>
| |