From: Sean Christopherson <seanjc@google.com>
To: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com,
linux-kernel@vger.kernel.org, joao.m.martins@oracle.com,
boris.ostrovsky@oracle.com, suravee.suthikulpanit@amd.com,
mlevitsk@redhat.com
Subject: Re: [PATCH v2 1/2] KVM: x86: Only set APICV_INHIBIT_REASON_ABSENT if APICv is enabled
Date: Thu, 2 May 2024 16:21:39 -0700 [thread overview]
Message-ID: <ZjQgA0ml4-mRJC-e@google.com> (raw)
In-Reply-To: <20240418021823.1275276-2-alejandro.j.jimenez@oracle.com>
On Thu, Apr 18, 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")
> Co-developed-by: Sean Christopherson <seanjc@google.com>
Heh, no need, I just provided review feedback.
> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> ---
> arch/x86/kvm/x86.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 26288ca05364..09052ff5a9a0 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9995,15 +9995,14 @@ static void set_or_clear_apicv_inhibit(unsigned long *inhibits,
>
> static void kvm_apicv_init(struct kvm *kvm)
> {
> - unsigned long *inhibits = &kvm->arch.apicv_inhibit_reasons;
> + enum kvm_apicv_inhibit reason = enable_apicv ?
> + APICV_INHIBIT_REASON_ABSENT :
> + APICV_INHIBIT_REASON_DISABLE;
Just let this poke out, the 80 char limit is a soft limit, where "soft" is fairly
"hard" in KVM", but there are still legitimate situations where running past 80
is yields more readable code, and IMO this is one of them.
> - init_rwsem(&kvm->arch.apicv_update_lock);
> -
> - set_or_clear_apicv_inhibit(inhibits, APICV_INHIBIT_REASON_ABSENT, true);
> + set_or_clear_apicv_inhibit(&kvm->arch.apicv_inhibit_reasons, reason,
> + true);
Same here.
No need for a v3, I'll fixup when applying.
>
> - if (!enable_apicv)
> - set_or_clear_apicv_inhibit(inhibits,
> - APICV_INHIBIT_REASON_DISABLE, true);
> + init_rwsem(&kvm->arch.apicv_update_lock);
> }
>
> static void kvm_sched_yield(struct kvm_vcpu *vcpu, unsigned long dest_id)
> --
> 2.39.3
>
next prev parent reply other threads:[~2024-05-02 23:21 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-18 2:18 [PATCH v2 0/2] APICv-related fixes for inhibits and tracepoint Alejandro Jimenez
2024-04-18 2:18 ` [PATCH v2 1/2] KVM: x86: Only set APICV_INHIBIT_REASON_ABSENT if APICv is enabled Alejandro Jimenez
2024-05-02 23:21 ` Sean Christopherson [this message]
2024-04-18 2:18 ` [PATCH v2 2/2] KVM: x86: Remove VT-d mention in posted interrupt tracepoint Alejandro Jimenez
2024-05-03 21:32 ` [PATCH v2 0/2] APICv-related fixes for inhibits and tracepoint Sean Christopherson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZjQgA0ml4-mRJC-e@google.com \
--to=seanjc@google.com \
--cc=alejandro.j.jimenez@oracle.com \
--cc=boris.ostrovsky@oracle.com \
--cc=joao.m.martins@oracle.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mlevitsk@redhat.com \
--cc=pbonzini@redhat.com \
--cc=suravee.suthikulpanit@amd.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).