[PATCH v2 0/2] APICv-related fixes for inhibits and tracepoint

[PATCH v2 0/2] APICv-related fixes for inhibits and tracepoint

[Alejandro Jimenez]

v2: Add Sean's changes to [PATCH 1/2].

--

Patch 1 fixes an issue when avic=0 (current default) where
APICV_INHIBIT_REASON_ABSENT remains set even after an in-kernel local APIC has
been created. e.g. tracing the inhibition tracepoint shows:

<...>-196432  [247] ..... 70380.628931: kvm_apicv_inhibit_changed: set reason=2, inhibits=0x4
<...>-196432  [247] ..... 70380.628941: kvm_apicv_inhibit_changed: set reason=0, inhibits=0x5

and the reason=2 inhibit is not removed after the local APIC is created.

Patch 2 modifies the wording in the pi_irte_update tracepoint to make it clear
that it is used by the posted interrupt implementation of both vendors. I have
reservations about modifying the tracepoint output and breaking user scripts,
but according to recent discussions tracepoints are not strictly a stable ABI,
so I'd consider this minor change to avoid confusion around this area.

Thank you,
Alejandro


Alejandro Jimenez (2):
  KVM: x86: Only set APICV_INHIBIT_REASON_ABSENT if APICv is enabled
  KVM: x86: Remove VT-d mention in posted interrupt tracepoint

 arch/x86/kvm/trace.h |  4 ++--
 arch/x86/kvm/x86.c   | 13 ++++++-------
 2 files changed, 8 insertions(+), 9 deletions(-)


base-commit: 2d181d84af38146748042a6974c577fc46c3f1c3
-- 
2.39.3

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

[Alejandro Jimenez]

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

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

[Sean Christopherson]

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
> 

[PATCH v2 2/2] KVM: x86: Remove VT-d mention in posted interrupt tracepoint

[Alejandro Jimenez]

The kvm_pi_irte_update tracepoint is called from both SVM and VMX vendor
code, and while the "posted interrupt" naming is also adopted by SVM in
several places, VT-d specifically refers to Intel's "Virtualization
Technology for Directed I/O".

Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
---
 arch/x86/kvm/trace.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index c6b4b1728006..9d0b02ef307e 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -1074,7 +1074,7 @@ TRACE_EVENT(kvm_smm_transition,
 );
 
 /*
- * Tracepoint for VT-d posted-interrupts.
+ * Tracepoint for VT-d posted-interrupts and AMD-Vi Guest Virtual APIC.
  */
 TRACE_EVENT(kvm_pi_irte_update,
 	TP_PROTO(unsigned int host_irq, unsigned int vcpu_id,
@@ -1100,7 +1100,7 @@ TRACE_EVENT(kvm_pi_irte_update,
 		__entry->set		= set;
 	),
 
-	TP_printk("VT-d PI is %s for irq %u, vcpu %u, gsi: 0x%x, "
+	TP_printk("PI is %s for irq %u, vcpu %u, gsi: 0x%x, "
 		  "gvec: 0x%x, pi_desc_addr: 0x%llx",
 		  __entry->set ? "enabled and being updated" : "disabled",
 		  __entry->host_irq,
-- 
2.39.3

Re: [PATCH v2 0/2] APICv-related fixes for inhibits and tracepoint

[Sean Christopherson]

On Thu, 18 Apr 2024 02:18:21 +0000, Alejandro Jimenez wrote:
> v2: Add Sean's changes to [PATCH 1/2].
> 
> --
> 
> Patch 1 fixes an issue when avic=0 (current default) where
> APICV_INHIBIT_REASON_ABSENT remains set even after an in-kernel local APIC has
> been created. e.g. tracing the inhibition tracepoint shows:
> 
> [...]

Applied to kvm-x86 misc, with the previously mentioned fixups, thanks!

[1/2] KVM: x86: Only set APICV_INHIBIT_REASON_ABSENT if APICv is enabled
      https://github.com/kvm-x86/linux/commit/6982b34c21cb
[2/2] KVM: x86: Remove VT-d mention in posted interrupt tracepoint
      https://github.com/kvm-x86/linux/commit/51937f2aae18

--
https://github.com/kvm-x86/linux/tree/next