| Message ID | 20240417200849.971433-2-alejandro.j.jimenez@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | APICv-related fixes for inhibits and tracepoint | expand |
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. > + 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 >
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 >>
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. + */ + if (enable_apicv) + set_or_clear_apicv_inhibit(inhibits, + APICV_INHIBIT_REASON_ABSENT, true); if (!enable_apicv) set_or_clear_apicv_inhibit(inhibits,
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(-)