[PATCH v2 0/5] KVM: x86: hyper-v: Conditionally allow SynIC with APICv/AVIC

[PATCH v2 0/5] KVM: x86: hyper-v: Conditionally allow SynIC with APICv/AVIC

[Vitaly Kuznetsov]

Changes since v1 (Sean):
- Use common 'enable_apicv' variable for both APICv and AVIC instead of 
 adding a new hook to 'struct kvm_x86_ops'.
- Drop unneded CONFIG_X86_LOCAL_APIC checks from VMX/SVM code along the
 way.

Original description:

APICV_INHIBIT_REASON_HYPERV is currently unconditionally forced upon
SynIC activation as SynIC's AutoEOI is incompatible with APICv/AVIC. It is,
however, possible to track whether the feature was actually used by the
guest and only inhibit APICv/AVIC when needed.

The feature can be tested with QEMU's 'hv-passthrough' debug mode.

Note, 'avic' kvm-amd module parameter is '0' by default and thus needs to
be explicitly enabled.

Vitaly Kuznetsov (5):
  KVM: SVM: Drop unneeded CONFIG_X86_LOCAL_APIC check for AVIC
  KVM: VMX: Drop unneeded CONFIG_X86_LOCAL_APIC check from
    cpu_has_vmx_posted_intr()
  KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC
  KVM: x86: Invert APICv/AVIC enablement check
  KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in
    use

 arch/x86/include/asm/kvm_host.h |  5 ++++-
 arch/x86/kvm/hyperv.c           | 27 +++++++++++++++++++++------
 arch/x86/kvm/svm/avic.c         | 16 +++++-----------
 arch/x86/kvm/svm/svm.c          | 24 +++++++++++++-----------
 arch/x86/kvm/svm/svm.h          |  2 --
 arch/x86/kvm/vmx/capabilities.h |  4 +---
 arch/x86/kvm/vmx/vmx.c          |  2 --
 arch/x86/kvm/x86.c              |  9 ++++++---
 8 files changed, 50 insertions(+), 39 deletions(-)

-- 
2.31.1

[PATCH v2 1/5] KVM: SVM: Drop unneeded CONFIG_X86_LOCAL_APIC check for AVIC

[Vitaly Kuznetsov]

AVIC dependency on CONFIG_X86_LOCAL_APIC is dead code since
commit e42eef4ba388 ("KVM: add X86_LOCAL_APIC dependency").

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/svm/avic.c | 2 --
 arch/x86/kvm/svm/svm.c  | 4 +---
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 712b4e0de481..1c1bf911e02b 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -29,9 +29,7 @@
 
 /* enable / disable AVIC */
 int avic;
-#ifdef CONFIG_X86_LOCAL_APIC
 module_param(avic, int, S_IRUGO);
-#endif
 
 #define SVM_AVIC_DOORBELL	0xc001011b
 
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index dfa351e605de..8c3918a11826 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1010,9 +1010,7 @@ static __init int svm_hardware_setup(void)
 	}
 
 	if (avic) {
-		if (!npt_enabled ||
-		    !boot_cpu_has(X86_FEATURE_AVIC) ||
-		    !IS_ENABLED(CONFIG_X86_LOCAL_APIC)) {
+		if (!npt_enabled || !boot_cpu_has(X86_FEATURE_AVIC)) {
 			avic = false;
 		} else {
 			pr_info("AVIC enabled\n");
-- 
2.31.1

Re: [PATCH v2 1/5] KVM: SVM: Drop unneeded CONFIG_X86_LOCAL_APIC check for AVIC

[Sean Christopherson]

On Tue, May 18, 2021, Vitaly Kuznetsov wrote:
> AVIC dependency on CONFIG_X86_LOCAL_APIC is dead code since
> commit e42eef4ba388 ("KVM: add X86_LOCAL_APIC dependency").
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH v2 1/5] KVM: SVM: Drop unneeded CONFIG_X86_LOCAL_APIC check for AVIC

[Maxim Levitsky]

On Tue, 2021-05-18 at 16:43 +0200, Vitaly Kuznetsov wrote:
> AVIC dependency on CONFIG_X86_LOCAL_APIC is dead code since
> commit e42eef4ba388 ("KVM: add X86_LOCAL_APIC dependency").

Indeed!
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  arch/x86/kvm/svm/avic.c | 2 --
>  arch/x86/kvm/svm/svm.c  | 4 +---
>  2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 712b4e0de481..1c1bf911e02b 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -29,9 +29,7 @@
>  
>  /* enable / disable AVIC */
>  int avic;
> -#ifdef CONFIG_X86_LOCAL_APIC
>  module_param(avic, int, S_IRUGO);
> -#endif
>  
>  #define SVM_AVIC_DOORBELL	0xc001011b
>  
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index dfa351e605de..8c3918a11826 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1010,9 +1010,7 @@ static __init int svm_hardware_setup(void)
>  	}
>  
>  	if (avic) {
> -		if (!npt_enabled ||
> -		    !boot_cpu_has(X86_FEATURE_AVIC) ||
> -		    !IS_ENABLED(CONFIG_X86_LOCAL_APIC)) {
> +		if (!npt_enabled || !boot_cpu_has(X86_FEATURE_AVIC)) {
>  			avic = false;
>  		} else {
>  			pr_info("AVIC enabled\n");

[PATCH v2 2/5] KVM: VMX: Drop unneeded CONFIG_X86_LOCAL_APIC check from cpu_has_vmx_posted_intr()

[Vitaly Kuznetsov]

CONFIG_X86_LOCAL_APIC is always on when CONFIG_KVM (on x86) since
commit e42eef4ba388 ("KVM: add X86_LOCAL_APIC dependency").

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/vmx/capabilities.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
index 8dee8a5fbc17..aa0e7872fcc9 100644
--- a/arch/x86/kvm/vmx/capabilities.h
+++ b/arch/x86/kvm/vmx/capabilities.h
@@ -90,8 +90,7 @@ static inline bool cpu_has_vmx_preemption_timer(void)
 
 static inline bool cpu_has_vmx_posted_intr(void)
 {
-	return IS_ENABLED(CONFIG_X86_LOCAL_APIC) &&
-		vmcs_config.pin_based_exec_ctrl & PIN_BASED_POSTED_INTR;
+	return vmcs_config.pin_based_exec_ctrl & PIN_BASED_POSTED_INTR;
 }
 
 static inline bool cpu_has_load_ia32_efer(void)
-- 
2.31.1

Re: [PATCH v2 2/5] KVM: VMX: Drop unneeded CONFIG_X86_LOCAL_APIC check from cpu_has_vmx_posted_intr()

[Sean Christopherson]

On Tue, May 18, 2021, Vitaly Kuznetsov wrote:
> CONFIG_X86_LOCAL_APIC is always on when CONFIG_KVM (on x86) since
> commit e42eef4ba388 ("KVM: add X86_LOCAL_APIC dependency").
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH v2 2/5] KVM: VMX: Drop unneeded CONFIG_X86_LOCAL_APIC check from cpu_has_vmx_posted_intr()

[Maxim Levitsky]

On Tue, 2021-05-18 at 16:43 +0200, Vitaly Kuznetsov wrote:
> CONFIG_X86_LOCAL_APIC is always on when CONFIG_KVM (on x86) since
> commit e42eef4ba388 ("KVM: add X86_LOCAL_APIC dependency").
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  arch/x86/kvm/vmx/capabilities.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
> index 8dee8a5fbc17..aa0e7872fcc9 100644
> --- a/arch/x86/kvm/vmx/capabilities.h
> +++ b/arch/x86/kvm/vmx/capabilities.h
> @@ -90,8 +90,7 @@ static inline bool cpu_has_vmx_preemption_timer(void)
>  
>  static inline bool cpu_has_vmx_posted_intr(void)
>  {
> -	return IS_ENABLED(CONFIG_X86_LOCAL_APIC) &&
> -		vmcs_config.pin_based_exec_ctrl & PIN_BASED_POSTED_INTR;
> +	return vmcs_config.pin_based_exec_ctrl & PIN_BASED_POSTED_INTR;
>  }
>  
>  static inline bool cpu_has_load_ia32_efer(void)
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
	Maxim Levitsky

[PATCH v2 3/5] KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC

[Vitaly Kuznetsov]

Unify VMX and SVM code by moving APICv/AVIC enablement tracking to common
'enable_apicv' variable. Note: unlike APICv, AVIC is disabled by default.

No functional change intended.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/svm/avic.c         | 14 +++++---------
 arch/x86/kvm/svm/svm.c          | 23 ++++++++++++++---------
 arch/x86/kvm/svm/svm.h          |  2 --
 arch/x86/kvm/vmx/capabilities.h |  1 -
 arch/x86/kvm/vmx/vmx.c          |  1 -
 arch/x86/kvm/x86.c              |  3 +++
 7 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 55efbacfc244..a2197fcf0e7c 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1422,6 +1422,7 @@ struct kvm_arch_async_pf {
 extern u32 __read_mostly kvm_nr_uret_msrs;
 extern u64 __read_mostly host_efer;
 extern bool __read_mostly allow_smaller_maxphyaddr;
+extern bool __read_mostly enable_apicv;
 extern struct kvm_x86_ops kvm_x86_ops;
 
 #define KVM_X86_OP(func) \
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 1c1bf911e02b..05cd0b128b02 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -27,10 +27,6 @@
 #include "irq.h"
 #include "svm.h"
 
-/* enable / disable AVIC */
-int avic;
-module_param(avic, int, S_IRUGO);
-
 #define SVM_AVIC_DOORBELL	0xc001011b
 
 #define AVIC_HPA_MASK	~((0xFFFULL << 52) | 0xFFF)
@@ -124,7 +120,7 @@ void avic_vm_destroy(struct kvm *kvm)
 	unsigned long flags;
 	struct kvm_svm *kvm_svm = to_kvm_svm(kvm);
 
-	if (!avic)
+	if (!enable_apicv)
 		return;
 
 	if (kvm_svm->avic_logical_id_table_page)
@@ -147,7 +143,7 @@ int avic_vm_init(struct kvm *kvm)
 	struct page *l_page;
 	u32 vm_id;
 
-	if (!avic)
+	if (!enable_apicv)
 		return 0;
 
 	/* Allocating physical APIC ID table (4KB) */
@@ -569,7 +565,7 @@ int avic_init_vcpu(struct vcpu_svm *svm)
 	int ret;
 	struct kvm_vcpu *vcpu = &svm->vcpu;
 
-	if (!avic || !irqchip_in_kernel(vcpu->kvm))
+	if (!enable_apicv || !irqchip_in_kernel(vcpu->kvm))
 		return 0;
 
 	ret = avic_init_backing_page(vcpu);
@@ -593,7 +589,7 @@ void avic_post_state_restore(struct kvm_vcpu *vcpu)
 
 void svm_toggle_avic_for_irq_window(struct kvm_vcpu *vcpu, bool activate)
 {
-	if (!avic || !lapic_in_kernel(vcpu))
+	if (!enable_apicv || !lapic_in_kernel(vcpu))
 		return;
 
 	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
@@ -653,7 +649,7 @@ void svm_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
 	struct vmcb *vmcb = svm->vmcb;
 	bool activated = kvm_vcpu_apicv_active(vcpu);
 
-	if (!avic)
+	if (!enable_apicv)
 		return;
 
 	if (activated) {
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 8c3918a11826..0d6ec34d1e4b 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -185,6 +185,10 @@ module_param(vls, int, 0444);
 static int vgif = true;
 module_param(vgif, int, 0444);
 
+/* enable / disable AVIC */
+static int avic;
+module_param(avic, int, 0444);
+
 bool __read_mostly dump_invalid_vmcb;
 module_param(dump_invalid_vmcb, bool, 0644);
 
@@ -1009,14 +1013,15 @@ static __init int svm_hardware_setup(void)
 			nrips = false;
 	}
 
-	if (avic) {
-		if (!npt_enabled || !boot_cpu_has(X86_FEATURE_AVIC)) {
-			avic = false;
-		} else {
-			pr_info("AVIC enabled\n");
+	if (!npt_enabled || !boot_cpu_has(X86_FEATURE_AVIC))
+		avic = false;
 
-			amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
-		}
+	/* 'enable_apicv' is common between VMX/SVM but the defaults differ */
+	enable_apicv = avic;
+	if (enable_apicv) {
+		pr_info("AVIC enabled\n");
+
+		amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
 	}
 
 	if (vls) {
@@ -4427,13 +4432,13 @@ static int svm_vm_init(struct kvm *kvm)
 	if (!pause_filter_count || !pause_filter_thresh)
 		kvm->arch.pause_in_guest = true;
 
-	if (avic) {
+	if (enable_apicv) {
 		int ret = avic_vm_init(kvm);
 		if (ret)
 			return ret;
 	}
 
-	kvm_apicv_init(kvm, avic);
+	kvm_apicv_init(kvm, enable_apicv);
 	return 0;
 }
 
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index e44567ceb865..a514b490db4a 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -479,8 +479,6 @@ extern struct kvm_x86_nested_ops svm_nested_ops;
 
 #define VMCB_AVIC_APIC_BAR_MASK		0xFFFFFFFFFF000ULL
 
-extern int avic;
-
 static inline void avic_update_vapic_bar(struct vcpu_svm *svm, u64 data)
 {
 	svm->vmcb->control.avic_vapic_bar = data & VMCB_AVIC_APIC_BAR_MASK;
diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
index aa0e7872fcc9..4705ad55abb5 100644
--- a/arch/x86/kvm/vmx/capabilities.h
+++ b/arch/x86/kvm/vmx/capabilities.h
@@ -12,7 +12,6 @@ extern bool __read_mostly enable_ept;
 extern bool __read_mostly enable_unrestricted_guest;
 extern bool __read_mostly enable_ept_ad_bits;
 extern bool __read_mostly enable_pml;
-extern bool __read_mostly enable_apicv;
 extern int __read_mostly pt_mode;
 
 #define PT_MODE_SYSTEM		0
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 4bceb5ca3a89..5e9ba10e9c2d 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -101,7 +101,6 @@ module_param(emulate_invalid_guest_state, bool, S_IRUGO);
 static bool __read_mostly fasteoi = 1;
 module_param(fasteoi, bool, S_IRUGO);
 
-bool __read_mostly enable_apicv = 1;
 module_param(enable_apicv, bool, S_IRUGO);
 
 /*
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 9b6bca616929..23fdbba6b394 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -209,6 +209,9 @@ EXPORT_SYMBOL_GPL(host_efer);
 bool __read_mostly allow_smaller_maxphyaddr = 0;
 EXPORT_SYMBOL_GPL(allow_smaller_maxphyaddr);
 
+bool __read_mostly enable_apicv = true;
+EXPORT_SYMBOL_GPL(enable_apicv);
+
 u64 __read_mostly host_xss;
 EXPORT_SYMBOL_GPL(host_xss);
 u64 __read_mostly supported_xss;
-- 
2.31.1

Re: [PATCH v2 3/5] KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC

[Sean Christopherson]

On Tue, May 18, 2021, Vitaly Kuznetsov wrote:
> Unify VMX and SVM code by moving APICv/AVIC enablement tracking to common
> 'enable_apicv' variable. Note: unlike APICv, AVIC is disabled by default.
> 
> No functional change intended.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---

...

> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 8c3918a11826..0d6ec34d1e4b 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -185,6 +185,10 @@ module_param(vls, int, 0444);
>  static int vgif = true;
>  module_param(vgif, int, 0444);
>  
> +/* enable / disable AVIC */
> +static int avic;
> +module_param(avic, int, 0444);

We should opportunistically make avic a "bool".

> +
>  bool __read_mostly dump_invalid_vmcb;
>  module_param(dump_invalid_vmcb, bool, 0644);
>  
> @@ -1009,14 +1013,15 @@ static __init int svm_hardware_setup(void)
>  			nrips = false;
>  	}
>  
> -	if (avic) {
> -		if (!npt_enabled || !boot_cpu_has(X86_FEATURE_AVIC)) {
> -			avic = false;
> -		} else {
> -			pr_info("AVIC enabled\n");
> +	if (!npt_enabled || !boot_cpu_has(X86_FEATURE_AVIC))
> +		avic = false;
>  
> -			amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
> -		}
> +	/* 'enable_apicv' is common between VMX/SVM but the defaults differ */

-1 for not throwing Jim under the bus :-)

Nits aside,

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH v2 3/5] KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> On Tue, May 18, 2021, Vitaly Kuznetsov wrote:
...
>> +	/* 'enable_apicv' is common between VMX/SVM but the defaults differ */
>
> -1 for not throwing Jim under the bus :-)

I bought the 'messenger' argument :-)

>
> Nits aside,
>
> Reviewed-by: Sean Christopherson <seanjc@google.com>
>

Thanks!

-- 
Vitaly

Re: [PATCH v2 3/5] KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC

[Paolo Bonzini]

On 18/05/21 22:39, Sean Christopherson wrote:
>> +/* enable / disable AVIC */
>> +static int avic;
>> +module_param(avic, int, 0444);
> We should opportunistically make avic a "bool".
> 

And also:

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 11714c22c9f1..48cb498ff070 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -185,9 +185,12 @@ module_param(vls, int, 0444);
  static int vgif = true;
  module_param(vgif, int, 0444);
  
-/* enable / disable AVIC */
-static int avic;
-module_param(avic, int, 0444);
+/*
+ * enable / disable AVIC.  Because the defaults differ for APICv
+ * support between VMX and SVM we cannot use module_param_named.
+ */
+static bool avic;
+module_param(avic, bool, 0444);
  
  bool __read_mostly dump_invalid_vmcb;
  module_param(dump_invalid_vmcb, bool, 0644);
@@ -1013,11 +1016,7 @@ static __init int svm_hardware_setup(void)
  			nrips = false;
  	}
  
-	if (!npt_enabled || !boot_cpu_has(X86_FEATURE_AVIC))
-		avic = false;
-
-	/* 'enable_apicv' is common between VMX/SVM but the defaults differ */
-	enable_apicv = avic;
+	enable_apicv = avic && npt_enabled && boot_cpu_has(X86_FEATURE_AVIC);
  	if (enable_apicv) {
  		pr_info("AVIC enabled\n");
  

The "if" can come back when AVIC is enabled by default.

Paolo

Re: [PATCH v2 3/5] KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC

[Sean Christopherson]

On Mon, May 24, 2021, Paolo Bonzini wrote:
> On 18/05/21 22:39, Sean Christopherson wrote:
> > > +/* enable / disable AVIC */
> > > +static int avic;
> > > +module_param(avic, int, 0444);
> > We should opportunistically make avic a "bool".
> > 
> 
> And also:
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 11714c22c9f1..48cb498ff070 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -185,9 +185,12 @@ module_param(vls, int, 0444);
>  static int vgif = true;
>  module_param(vgif, int, 0444);
> -/* enable / disable AVIC */
> -static int avic;
> -module_param(avic, int, 0444);
> +/*
> + * enable / disable AVIC.  Because the defaults differ for APICv
> + * support between VMX and SVM we cannot use module_param_named.
> + */
> +static bool avic;
> +module_param(avic, bool, 0444);
>  bool __read_mostly dump_invalid_vmcb;
>  module_param(dump_invalid_vmcb, bool, 0644);
> @@ -1013,11 +1016,7 @@ static __init int svm_hardware_setup(void)
>  			nrips = false;
>  	}
> -	if (!npt_enabled || !boot_cpu_has(X86_FEATURE_AVIC))
> -		avic = false;
> -
> -	/* 'enable_apicv' is common between VMX/SVM but the defaults differ */
> -	enable_apicv = avic;
> +	enable_apicv = avic && npt_enabled && boot_cpu_has(X86_FEATURE_AVIC);
>  	if (enable_apicv) {
>  		pr_info("AVIC enabled\n");
> 
> The "if" can come back when AVIC is enabled by default.

But "avic" is connected to the module param, even if it's off by default its
effective value should be reflected in sysfs.  E.g. the user may incorrectly
think AVIC is in use if they set avic=1 but the CPU doesn't support AVIC.
Forcing the user to check /proc/cpuinfo or look for "AVIC enabled" in dmesg is
kludgy at best.

Re: [PATCH v2 3/5] KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC

[Paolo Bonzini]

On 24/05/21 18:52, Sean Christopherson wrote:
> On Mon, May 24, 2021, Paolo Bonzini wrote:
>> On 18/05/21 22:39, Sean Christopherson wrote:
>>>> +/* enable / disable AVIC */
>>>> +static int avic;
>>>> +module_param(avic, int, 0444);
>>> We should opportunistically make avic a "bool".
>>>
>>
>> And also:
>>
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index 11714c22c9f1..48cb498ff070 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -185,9 +185,12 @@ module_param(vls, int, 0444);
>>   static int vgif = true;
>>   module_param(vgif, int, 0444);
>> -/* enable / disable AVIC */
>> -static int avic;
>> -module_param(avic, int, 0444);
>> +/*
>> + * enable / disable AVIC.  Because the defaults differ for APICv
>> + * support between VMX and SVM we cannot use module_param_named.
>> + */
>> +static bool avic;
>> +module_param(avic, bool, 0444);
>>   bool __read_mostly dump_invalid_vmcb;
>>   module_param(dump_invalid_vmcb, bool, 0644);
>> @@ -1013,11 +1016,7 @@ static __init int svm_hardware_setup(void)
>>   			nrips = false;
>>   	}
>> -	if (!npt_enabled || !boot_cpu_has(X86_FEATURE_AVIC))
>> -		avic = false;
>> -
>> -	/* 'enable_apicv' is common between VMX/SVM but the defaults differ */
>> -	enable_apicv = avic;
>> +	enable_apicv = avic && npt_enabled && boot_cpu_has(X86_FEATURE_AVIC);
>>   	if (enable_apicv) {
>>   		pr_info("AVIC enabled\n");
>>
>> The "if" can come back when AVIC is enabled by default.
> 
> But "avic" is connected to the module param, even if it's off by default its
> effective value should be reflected in sysfs.  E.g. the user may incorrectly
> think AVIC is in use if they set avic=1 but the CPU doesn't support AVIC.
> Forcing the user to check /proc/cpuinfo or look for "AVIC enabled" in dmesg is
> kludgy at best.

Indeed -- I even tested the above, but only before realizing that 
module_param_named would change the default.  So for now this needs to
be "enable_apicv = avic = ...", and later it can become just

	enable_apicv &= npt_enabled && boot_cpu_has(X86_FEATURE_AVIC);

Paolo

Re: [PATCH v2 3/5] KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC

[Maxim Levitsky]

On Tue, 2021-05-18 at 16:43 +0200, Vitaly Kuznetsov wrote:
> Unify VMX and SVM code by moving APICv/AVIC enablement tracking to common
> 'enable_apicv' variable. Note: unlike APICv, AVIC is disabled by default.
> 
> No functional change intended.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  1 +
>  arch/x86/kvm/svm/avic.c         | 14 +++++---------
>  arch/x86/kvm/svm/svm.c          | 23 ++++++++++++++---------
>  arch/x86/kvm/svm/svm.h          |  2 --
>  arch/x86/kvm/vmx/capabilities.h |  1 -
>  arch/x86/kvm/vmx/vmx.c          |  1 -
>  arch/x86/kvm/x86.c              |  3 +++
>  7 files changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 55efbacfc244..a2197fcf0e7c 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1422,6 +1422,7 @@ struct kvm_arch_async_pf {
>  extern u32 __read_mostly kvm_nr_uret_msrs;
>  extern u64 __read_mostly host_efer;
>  extern bool __read_mostly allow_smaller_maxphyaddr;
> +extern bool __read_mostly enable_apicv;
>  extern struct kvm_x86_ops kvm_x86_ops;
>  
>  #define KVM_X86_OP(func) \
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 1c1bf911e02b..05cd0b128b02 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -27,10 +27,6 @@
>  #include "irq.h"
>  #include "svm.h"
>  
> -/* enable / disable AVIC */
> -int avic;
> -module_param(avic, int, S_IRUGO);
> -
>  #define SVM_AVIC_DOORBELL	0xc001011b
>  
>  #define AVIC_HPA_MASK	~((0xFFFULL << 52) | 0xFFF)
> @@ -124,7 +120,7 @@ void avic_vm_destroy(struct kvm *kvm)
>  	unsigned long flags;
>  	struct kvm_svm *kvm_svm = to_kvm_svm(kvm);
>  
> -	if (!avic)
> +	if (!enable_apicv)
>  		return;
>  
>  	if (kvm_svm->avic_logical_id_table_page)
> @@ -147,7 +143,7 @@ int avic_vm_init(struct kvm *kvm)
>  	struct page *l_page;
>  	u32 vm_id;
>  
> -	if (!avic)
> +	if (!enable_apicv)
>  		return 0;
>  
>  	/* Allocating physical APIC ID table (4KB) */
> @@ -569,7 +565,7 @@ int avic_init_vcpu(struct vcpu_svm *svm)
>  	int ret;
>  	struct kvm_vcpu *vcpu = &svm->vcpu;
>  
> -	if (!avic || !irqchip_in_kernel(vcpu->kvm))
> +	if (!enable_apicv || !irqchip_in_kernel(vcpu->kvm))
>  		return 0;
>  
>  	ret = avic_init_backing_page(vcpu);
> @@ -593,7 +589,7 @@ void avic_post_state_restore(struct kvm_vcpu *vcpu)
>  
>  void svm_toggle_avic_for_irq_window(struct kvm_vcpu *vcpu, bool activate)
>  {
> -	if (!avic || !lapic_in_kernel(vcpu))
> +	if (!enable_apicv || !lapic_in_kernel(vcpu))
>  		return;
>  
>  	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
> @@ -653,7 +649,7 @@ void svm_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
>  	struct vmcb *vmcb = svm->vmcb;
>  	bool activated = kvm_vcpu_apicv_active(vcpu);
>  
> -	if (!avic)
> +	if (!enable_apicv)
>  		return;
>  
>  	if (activated) {
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 8c3918a11826..0d6ec34d1e4b 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -185,6 +185,10 @@ module_param(vls, int, 0444);
>  static int vgif = true;
>  module_param(vgif, int, 0444);
>  
> +/* enable / disable AVIC */
> +static int avic;
> +module_param(avic, int, 0444);
> +
>  bool __read_mostly dump_invalid_vmcb;
>  module_param(dump_invalid_vmcb, bool, 0644);
>  
> @@ -1009,14 +1013,15 @@ static __init int svm_hardware_setup(void)
>  			nrips = false;
>  	}
>  
> -	if (avic) {
> -		if (!npt_enabled || !boot_cpu_has(X86_FEATURE_AVIC)) {
> -			avic = false;
> -		} else {
> -			pr_info("AVIC enabled\n");
> +	if (!npt_enabled || !boot_cpu_has(X86_FEATURE_AVIC))
> +		avic = false;
>  
> -			amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
> -		}
> +	/* 'enable_apicv' is common between VMX/SVM but the defaults differ */
> +	enable_apicv = avic;
> +	if (enable_apicv) {
> +		pr_info("AVIC enabled\n");
> +
> +		amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
>  	}
>  
>  	if (vls) {
> @@ -4427,13 +4432,13 @@ static int svm_vm_init(struct kvm *kvm)
>  	if (!pause_filter_count || !pause_filter_thresh)
>  		kvm->arch.pause_in_guest = true;
>  
> -	if (avic) {
> +	if (enable_apicv) {
>  		int ret = avic_vm_init(kvm);
>  		if (ret)
>  			return ret;
>  	}
>  
> -	kvm_apicv_init(kvm, avic);
> +	kvm_apicv_init(kvm, enable_apicv);
>  	return 0;
>  }
>  
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index e44567ceb865..a514b490db4a 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -479,8 +479,6 @@ extern struct kvm_x86_nested_ops svm_nested_ops;
>  
>  #define VMCB_AVIC_APIC_BAR_MASK		0xFFFFFFFFFF000ULL
>  
> -extern int avic;
> -
>  static inline void avic_update_vapic_bar(struct vcpu_svm *svm, u64 data)
>  {
>  	svm->vmcb->control.avic_vapic_bar = data & VMCB_AVIC_APIC_BAR_MASK;
> diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
> index aa0e7872fcc9..4705ad55abb5 100644
> --- a/arch/x86/kvm/vmx/capabilities.h
> +++ b/arch/x86/kvm/vmx/capabilities.h
> @@ -12,7 +12,6 @@ extern bool __read_mostly enable_ept;
>  extern bool __read_mostly enable_unrestricted_guest;
>  extern bool __read_mostly enable_ept_ad_bits;
>  extern bool __read_mostly enable_pml;
> -extern bool __read_mostly enable_apicv;
>  extern int __read_mostly pt_mode;
>  
>  #define PT_MODE_SYSTEM		0
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 4bceb5ca3a89..5e9ba10e9c2d 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -101,7 +101,6 @@ module_param(emulate_invalid_guest_state, bool, S_IRUGO);
>  static bool __read_mostly fasteoi = 1;
>  module_param(fasteoi, bool, S_IRUGO);
>  
> -bool __read_mostly enable_apicv = 1;
>  module_param(enable_apicv, bool, S_IRUGO);
>  
>  /*
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 9b6bca616929..23fdbba6b394 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -209,6 +209,9 @@ EXPORT_SYMBOL_GPL(host_efer);
>  bool __read_mostly allow_smaller_maxphyaddr = 0;
>  EXPORT_SYMBOL_GPL(allow_smaller_maxphyaddr);
>  
> +bool __read_mostly enable_apicv = true;

Nitpick: I don't like this asymmetry.

VMX and the common code uses the enable_apicv module param and variable,
while SVM uses avic, which sets the enable_apicv variable.
 
I'll prefer both VMX and SVM have their own private variable
for their avic/apicv module param, 
which should set a common variable later.
 

Best regards,
	Maxim Levitsky


> +EXPORT_SYMBOL_GPL(enable_apicv);
> +
>  u64 __read_mostly host_xss;
>  EXPORT_SYMBOL_GPL(host_xss);
>  u64 __read_mostly supported_xss;

Re: [PATCH v2 3/5] KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC

[Sean Christopherson]

On Wed, May 26, 2021, Maxim Levitsky wrote:
> On Tue, 2021-05-18 at 16:43 +0200, Vitaly Kuznetsov wrote:
> > Unify VMX and SVM code by moving APICv/AVIC enablement tracking to common
> > 'enable_apicv' variable. Note: unlike APICv, AVIC is disabled by default.
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 9b6bca616929..23fdbba6b394 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -209,6 +209,9 @@ EXPORT_SYMBOL_GPL(host_efer);
> >  bool __read_mostly allow_smaller_maxphyaddr = 0;
> >  EXPORT_SYMBOL_GPL(allow_smaller_maxphyaddr);
> >  
> > +bool __read_mostly enable_apicv = true;
> 
> Nitpick: I don't like this asymmetry.
>
> VMX and the common code uses the enable_apicv module param and variable,
> while SVM uses avic, which sets the enable_apicv variable.
>  
> I'll prefer both VMX and SVM have their own private variable for their
> avic/apicv module param, which should set a common variable later.

I don't love the intermediate "avic" either, but there isn't a good alternative.
Forcing VMX to also use an intermediate doesn't make much sense, we'd be penalizing
ourselves in the form of unnecessary complexity just because AVIC needs to be
disabled by default for reasons KVM can't fix.

As for the asymmetry, I actually like it because it makes "avic" stand out and
highlights that there is weirdness with enabling AVIC.

Re: [PATCH v2 3/5] KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC

[Maxim Levitsky]

On Wed, 2021-05-26 at 15:07 +0000, Sean Christopherson wrote:
> On Wed, May 26, 2021, Maxim Levitsky wrote:
> > On Tue, 2021-05-18 at 16:43 +0200, Vitaly Kuznetsov wrote:
> > > Unify VMX and SVM code by moving APICv/AVIC enablement tracking to common
> > > 'enable_apicv' variable. Note: unlike APICv, AVIC is disabled by default.
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 9b6bca616929..23fdbba6b394 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -209,6 +209,9 @@ EXPORT_SYMBOL_GPL(host_efer);
> > >  bool __read_mostly allow_smaller_maxphyaddr = 0;
> > >  EXPORT_SYMBOL_GPL(allow_smaller_maxphyaddr);
> > >  
> > > +bool __read_mostly enable_apicv = true;
> > 
> > Nitpick: I don't like this asymmetry.
> > 
> > VMX and the common code uses the enable_apicv module param and variable,
> > while SVM uses avic, which sets the enable_apicv variable.
> >  
> > I'll prefer both VMX and SVM have their own private variable for their
> > avic/apicv module param, which should set a common variable later.
> 
> I don't love the intermediate "avic" either, but there isn't a good alternative.
> Forcing VMX to also use an intermediate doesn't make much sense, we'd be penalizing
> ourselves in the form of unnecessary complexity just because AVIC needs to be
> disabled by default for reasons KVM can't fix.
This is also something we should eventually reconsider. 
These days, the AVIC works quite well and disables itself when needed.
When do you think it will be the time to enable it by default?

> 
> As for the asymmetry, I actually like it because it makes "avic" stand out and
> highlights that there is weirdness with enabling AVIC.
You mean that it is disabled by default?

Anyway I don't have that strong opinion on this,
so let it be like this.

Best regards,
	Maxim Levitsky


> 

Re: [PATCH v2 3/5] KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC

[Paolo Bonzini]

On 26/05/21 17:52, Maxim Levitsky wrote:
>> I don't love the intermediate "avic" either, but there isn't a good alternative.
>> Forcing VMX to also use an intermediate doesn't make much sense, we'd be penalizing
>> ourselves in the form of unnecessary complexity just because AVIC needs to be
>> disabled by default for reasons KVM can't fix.
> This is also something we should eventually reconsider.
> These days, the AVIC works quite well and disables itself when needed.
> When do you think it will be the time to enable it by default?

We can probably enable it, but most guests won't use it because of x2apic.

Paolo

[PATCH v2 4/5] KVM: x86: Invert APICv/AVIC enablement check

[Vitaly Kuznetsov]

Now that APICv/AVIC enablement is kept in common 'enable_apicv' variable,
there's no need to call kvm_apicv_init() from vendor specific code.

No functional change intended.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/include/asm/kvm_host.h | 1 -
 arch/x86/kvm/svm/svm.c          | 1 -
 arch/x86/kvm/vmx/vmx.c          | 1 -
 arch/x86/kvm/x86.c              | 6 +++---
 4 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index a2197fcf0e7c..bf5807d35339 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1662,7 +1662,6 @@ gpa_t kvm_mmu_gva_to_gpa_system(struct kvm_vcpu *vcpu, gva_t gva,
 				struct x86_exception *exception);
 
 bool kvm_apicv_activated(struct kvm *kvm);
-void kvm_apicv_init(struct kvm *kvm, bool enable);
 void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu);
 void kvm_request_apicv_update(struct kvm *kvm, bool activate,
 			      unsigned long bit);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 0d6ec34d1e4b..84f58e8b2f49 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4438,7 +4438,6 @@ static int svm_vm_init(struct kvm *kvm)
 			return ret;
 	}
 
-	kvm_apicv_init(kvm, enable_apicv);
 	return 0;
 }
 
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 5e9ba10e9c2d..697dd54c7df8 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7000,7 +7000,6 @@ static int vmx_vm_init(struct kvm *kvm)
 			break;
 		}
 	}
-	kvm_apicv_init(kvm, enable_apicv);
 	return 0;
 }
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 23fdbba6b394..22a1e2b438c3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8345,16 +8345,15 @@ bool kvm_apicv_activated(struct kvm *kvm)
 }
 EXPORT_SYMBOL_GPL(kvm_apicv_activated);
 
-void kvm_apicv_init(struct kvm *kvm, bool enable)
+static void kvm_apicv_init(struct kvm *kvm)
 {
-	if (enable)
+	if (enable_apicv)
 		clear_bit(APICV_INHIBIT_REASON_DISABLE,
 			  &kvm->arch.apicv_inhibit_reasons);
 	else
 		set_bit(APICV_INHIBIT_REASON_DISABLE,
 			&kvm->arch.apicv_inhibit_reasons);
 }
-EXPORT_SYMBOL_GPL(kvm_apicv_init);
 
 static void kvm_sched_yield(struct kvm_vcpu *vcpu, unsigned long dest_id)
 {
@@ -10739,6 +10738,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	INIT_DELAYED_WORK(&kvm->arch.kvmclock_update_work, kvmclock_update_fn);
 	INIT_DELAYED_WORK(&kvm->arch.kvmclock_sync_work, kvmclock_sync_fn);
 
+	kvm_apicv_init(kvm);
 	kvm_hv_init_vm(kvm);
 	kvm_page_track_init(kvm);
 	kvm_mmu_init_vm(kvm);
-- 
2.31.1

Re: [PATCH v2 4/5] KVM: x86: Invert APICv/AVIC enablement check

[Sean Christopherson]

On Tue, May 18, 2021, Vitaly Kuznetsov wrote:
> Now that APICv/AVIC enablement is kept in common 'enable_apicv' variable,
> there's no need to call kvm_apicv_init() from vendor specific code.
> 
> No functional change intended.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH v2 4/5] KVM: x86: Invert APICv/AVIC enablement check

[Maxim Levitsky]

On Tue, 2021-05-18 at 16:43 +0200, Vitaly Kuznetsov wrote:
> Now that APICv/AVIC enablement is kept in common 'enable_apicv' variable,
> there's no need to call kvm_apicv_init() from vendor specific code.
> 
> No functional change intended.

Minor nitpick: I don't see any invert here, but rather
a unification of SVM/VMX virtual apic enablement code.
Maybe update the subject a bit?

For the code:

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h | 1 -
>  arch/x86/kvm/svm/svm.c          | 1 -
>  arch/x86/kvm/vmx/vmx.c          | 1 -
>  arch/x86/kvm/x86.c              | 6 +++---
>  4 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index a2197fcf0e7c..bf5807d35339 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1662,7 +1662,6 @@ gpa_t kvm_mmu_gva_to_gpa_system(struct kvm_vcpu *vcpu, gva_t gva,
>  				struct x86_exception *exception);
>  
>  bool kvm_apicv_activated(struct kvm *kvm);
> -void kvm_apicv_init(struct kvm *kvm, bool enable);
>  void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu);
>  void kvm_request_apicv_update(struct kvm *kvm, bool activate,
>  			      unsigned long bit);
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 0d6ec34d1e4b..84f58e8b2f49 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4438,7 +4438,6 @@ static int svm_vm_init(struct kvm *kvm)
>  			return ret;
>  	}
>  
> -	kvm_apicv_init(kvm, enable_apicv);
>  	return 0;
>  }
>  
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 5e9ba10e9c2d..697dd54c7df8 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7000,7 +7000,6 @@ static int vmx_vm_init(struct kvm *kvm)
>  			break;
>  		}
>  	}
> -	kvm_apicv_init(kvm, enable_apicv);
>  	return 0;
>  }
>  
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 23fdbba6b394..22a1e2b438c3 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8345,16 +8345,15 @@ bool kvm_apicv_activated(struct kvm *kvm)
>  }
>  EXPORT_SYMBOL_GPL(kvm_apicv_activated);
>  
> -void kvm_apicv_init(struct kvm *kvm, bool enable)
> +static void kvm_apicv_init(struct kvm *kvm)
>  {
> -	if (enable)
> +	if (enable_apicv)
>  		clear_bit(APICV_INHIBIT_REASON_DISABLE,
>  			  &kvm->arch.apicv_inhibit_reasons);
>  	else
>  		set_bit(APICV_INHIBIT_REASON_DISABLE,
>  			&kvm->arch.apicv_inhibit_reasons);
>  }
> -EXPORT_SYMBOL_GPL(kvm_apicv_init);
>  
>  static void kvm_sched_yield(struct kvm_vcpu *vcpu, unsigned long dest_id)
>  {
> @@ -10739,6 +10738,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>  	INIT_DELAYED_WORK(&kvm->arch.kvmclock_update_work, kvmclock_update_fn);
>  	INIT_DELAYED_WORK(&kvm->arch.kvmclock_sync_work, kvmclock_sync_fn);
>  
> +	kvm_apicv_init(kvm);
>  	kvm_hv_init_vm(kvm);
>  	kvm_page_track_init(kvm);
>  	kvm_mmu_init_vm(kvm);

Re: [PATCH v2 4/5] KVM: x86: Invert APICv/AVIC enablement check

[Vitaly Kuznetsov]

Maxim Levitsky <mlevitsk@redhat.com> writes:

> On Tue, 2021-05-18 at 16:43 +0200, Vitaly Kuznetsov wrote:
>> Now that APICv/AVIC enablement is kept in common 'enable_apicv' variable,
>> there's no need to call kvm_apicv_init() from vendor specific code.
>> 
>> No functional change intended.
>
> Minor nitpick: I don't see any invert here, but rather
> a unification of SVM/VMX virtual apic enablement code.
> Maybe update the subject a bit?

It is a bit umbiguous in v2, I agree (v1 used hooks in vendor-specific
code so instead of calling to vendor-neutral kvm_apicv_init() from
vendor-specific svm_vm_init()/vmx_vm_init(), we were calling
vendor-specific hooks from vendor-neutral kvm_apicv_init(), thus
'invert'. We can update the subject to something like

"KVM: x86: Drop vendor specific functions for APICv/AVIC enablement"

or something like that.

>
> For the code:
>
> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
>

Thanks!

> Best regards,
> 	Maxim Levitsky
>
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  arch/x86/include/asm/kvm_host.h | 1 -
>>  arch/x86/kvm/svm/svm.c          | 1 -
>>  arch/x86/kvm/vmx/vmx.c          | 1 -
>>  arch/x86/kvm/x86.c              | 6 +++---
>>  4 files changed, 3 insertions(+), 6 deletions(-)
>> 
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index a2197fcf0e7c..bf5807d35339 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -1662,7 +1662,6 @@ gpa_t kvm_mmu_gva_to_gpa_system(struct kvm_vcpu *vcpu, gva_t gva,
>>  				struct x86_exception *exception);
>>  
>>  bool kvm_apicv_activated(struct kvm *kvm);
>> -void kvm_apicv_init(struct kvm *kvm, bool enable);
>>  void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu);
>>  void kvm_request_apicv_update(struct kvm *kvm, bool activate,
>>  			      unsigned long bit);
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index 0d6ec34d1e4b..84f58e8b2f49 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -4438,7 +4438,6 @@ static int svm_vm_init(struct kvm *kvm)
>>  			return ret;
>>  	}
>>  
>> -	kvm_apicv_init(kvm, enable_apicv);
>>  	return 0;
>>  }
>>  
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index 5e9ba10e9c2d..697dd54c7df8 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -7000,7 +7000,6 @@ static int vmx_vm_init(struct kvm *kvm)
>>  			break;
>>  		}
>>  	}
>> -	kvm_apicv_init(kvm, enable_apicv);
>>  	return 0;
>>  }
>>  
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 23fdbba6b394..22a1e2b438c3 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -8345,16 +8345,15 @@ bool kvm_apicv_activated(struct kvm *kvm)
>>  }
>>  EXPORT_SYMBOL_GPL(kvm_apicv_activated);
>>  
>> -void kvm_apicv_init(struct kvm *kvm, bool enable)
>> +static void kvm_apicv_init(struct kvm *kvm)
>>  {
>> -	if (enable)
>> +	if (enable_apicv)
>>  		clear_bit(APICV_INHIBIT_REASON_DISABLE,
>>  			  &kvm->arch.apicv_inhibit_reasons);
>>  	else
>>  		set_bit(APICV_INHIBIT_REASON_DISABLE,
>>  			&kvm->arch.apicv_inhibit_reasons);
>>  }
>> -EXPORT_SYMBOL_GPL(kvm_apicv_init);
>>  
>>  static void kvm_sched_yield(struct kvm_vcpu *vcpu, unsigned long dest_id)
>>  {
>> @@ -10739,6 +10738,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>>  	INIT_DELAYED_WORK(&kvm->arch.kvmclock_update_work, kvmclock_update_fn);
>>  	INIT_DELAYED_WORK(&kvm->arch.kvmclock_sync_work, kvmclock_sync_fn);
>>  
>> +	kvm_apicv_init(kvm);
>>  	kvm_hv_init_vm(kvm);
>>  	kvm_page_track_init(kvm);
>>  	kvm_mmu_init_vm(kvm);
>
>

-- 
Vitaly

Re: [PATCH v2 4/5] KVM: x86: Invert APICv/AVIC enablement check

[Maxim Levitsky]

On Wed, 2021-05-26 at 12:40 +0200, Vitaly Kuznetsov wrote:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
> 
> > On Tue, 2021-05-18 at 16:43 +0200, Vitaly Kuznetsov wrote:
> > > Now that APICv/AVIC enablement is kept in common 'enable_apicv' variable,
> > > there's no need to call kvm_apicv_init() from vendor specific code.
> > > 
> > > No functional change intended.
> > 
> > Minor nitpick: I don't see any invert here, but rather
> > a unification of SVM/VMX virtual apic enablement code.
> > Maybe update the subject a bit?
> 
> It is a bit umbiguous in v2, I agree (v1 used hooks in vendor-specific
> code so instead of calling to vendor-neutral kvm_apicv_init() from
> vendor-specific svm_vm_init()/vmx_vm_init(), we were calling
> vendor-specific hooks from vendor-neutral kvm_apicv_init(), thus
> 'invert'. We can update the subject to something like
> 
> "KVM: x86: Drop vendor specific functions for APICv/AVIC enablement"

Yep, this subject line looks great.
Thanks!

Best regards,
	Maxim Levitsky

> 
> or something like that.
> 
> > For the code:
> > 
> > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> > 
> 
> Thanks!
> 
> > Best regards,
> > 	Maxim Levitsky
> > 
> > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > > ---
> > >  arch/x86/include/asm/kvm_host.h | 1 -
> > >  arch/x86/kvm/svm/svm.c          | 1 -
> > >  arch/x86/kvm/vmx/vmx.c          | 1 -
> > >  arch/x86/kvm/x86.c              | 6 +++---
> > >  4 files changed, 3 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > index a2197fcf0e7c..bf5807d35339 100644
> > > --- a/arch/x86/include/asm/kvm_host.h
> > > +++ b/arch/x86/include/asm/kvm_host.h
> > > @@ -1662,7 +1662,6 @@ gpa_t kvm_mmu_gva_to_gpa_system(struct kvm_vcpu *vcpu, gva_t gva,
> > >  				struct x86_exception *exception);
> > >  
> > >  bool kvm_apicv_activated(struct kvm *kvm);
> > > -void kvm_apicv_init(struct kvm *kvm, bool enable);
> > >  void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu);
> > >  void kvm_request_apicv_update(struct kvm *kvm, bool activate,
> > >  			      unsigned long bit);
> > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > > index 0d6ec34d1e4b..84f58e8b2f49 100644
> > > --- a/arch/x86/kvm/svm/svm.c
> > > +++ b/arch/x86/kvm/svm/svm.c
> > > @@ -4438,7 +4438,6 @@ static int svm_vm_init(struct kvm *kvm)
> > >  			return ret;
> > >  	}
> > >  
> > > -	kvm_apicv_init(kvm, enable_apicv);
> > >  	return 0;
> > >  }
> > >  
> > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > > index 5e9ba10e9c2d..697dd54c7df8 100644
> > > --- a/arch/x86/kvm/vmx/vmx.c
> > > +++ b/arch/x86/kvm/vmx/vmx.c
> > > @@ -7000,7 +7000,6 @@ static int vmx_vm_init(struct kvm *kvm)
> > >  			break;
> > >  		}
> > >  	}
> > > -	kvm_apicv_init(kvm, enable_apicv);
> > >  	return 0;
> > >  }
> > >  
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 23fdbba6b394..22a1e2b438c3 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -8345,16 +8345,15 @@ bool kvm_apicv_activated(struct kvm *kvm)
> > >  }
> > >  EXPORT_SYMBOL_GPL(kvm_apicv_activated);
> > >  
> > > -void kvm_apicv_init(struct kvm *kvm, bool enable)
> > > +static void kvm_apicv_init(struct kvm *kvm)
> > >  {
> > > -	if (enable)
> > > +	if (enable_apicv)
> > >  		clear_bit(APICV_INHIBIT_REASON_DISABLE,
> > >  			  &kvm->arch.apicv_inhibit_reasons);
> > >  	else
> > >  		set_bit(APICV_INHIBIT_REASON_DISABLE,
> > >  			&kvm->arch.apicv_inhibit_reasons);
> > >  }
> > > -EXPORT_SYMBOL_GPL(kvm_apicv_init);
> > >  
> > >  static void kvm_sched_yield(struct kvm_vcpu *vcpu, unsigned long dest_id)
> > >  {
> > > @@ -10739,6 +10738,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> > >  	INIT_DELAYED_WORK(&kvm->arch.kvmclock_update_work, kvmclock_update_fn);
> > >  	INIT_DELAYED_WORK(&kvm->arch.kvmclock_sync_work, kvmclock_sync_fn);
> > >  
> > > +	kvm_apicv_init(kvm);
> > >  	kvm_hv_init_vm(kvm);
> > >  	kvm_page_track_init(kvm);
> > >  	kvm_mmu_init_vm(kvm);

[PATCH v2 5/5] KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in use

[Vitaly Kuznetsov]

APICV_INHIBIT_REASON_HYPERV is currently unconditionally forced upon
SynIC activation as SynIC's AutoEOI is incompatible with APICv/AVIC. It is,
however, possible to track whether the feature was actually used by the
guest and only inhibit APICv/AVIC when needed.

TLFS suggests a dedicated 'HV_DEPRECATING_AEOI_RECOMMENDED' flag to let
Windows know that AutoEOI feature should be avoided. While it's up to
KVM userspace to set the flag, KVM can help a bit by exposing global
APICv/AVIC enablement: in case APICv/AVIC usage is impossible, AutoEOI
is still preferred.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  3 +++
 arch/x86/kvm/hyperv.c           | 27 +++++++++++++++++++++------
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index bf5807d35339..5e03ab4c0e4f 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -936,6 +936,9 @@ struct kvm_hv {
 	/* How many vCPUs have VP index != vCPU index */
 	atomic_t num_mismatched_vp_indexes;
 
+	/* How many SynICs use 'AutoEOI' feature */
+	atomic_t synic_auto_eoi_used;
+
 	struct hv_partition_assist_pg *hv_pa_pg;
 	struct kvm_hv_syndbg hv_syndbg;
 };
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index f98370a39936..89e7d5b99279 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -87,6 +87,10 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic,
 static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
 				int vector)
 {
+	struct kvm_vcpu *vcpu = hv_synic_to_vcpu(synic);
+	struct kvm_hv *hv = to_kvm_hv(vcpu->kvm);
+	int auto_eoi_old, auto_eoi_new;
+
 	if (vector < HV_SYNIC_FIRST_VALID_VECTOR)
 		return;
 
@@ -95,10 +99,25 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
 	else
 		__clear_bit(vector, synic->vec_bitmap);
 
+	auto_eoi_old = bitmap_weight(synic->auto_eoi_bitmap, 256);
+
 	if (synic_has_vector_auto_eoi(synic, vector))
 		__set_bit(vector, synic->auto_eoi_bitmap);
 	else
 		__clear_bit(vector, synic->auto_eoi_bitmap);
+
+	auto_eoi_new = bitmap_weight(synic->auto_eoi_bitmap, 256);
+
+	/* Hyper-V SynIC auto EOI SINTs are not compatible with APICV */
+	if (!auto_eoi_old && auto_eoi_new) {
+		if (atomic_inc_return(&hv->synic_auto_eoi_used) == 1)
+			kvm_request_apicv_update(vcpu->kvm, false,
+						 APICV_INHIBIT_REASON_HYPERV);
+	} else if (!auto_eoi_new && auto_eoi_old) {
+		if (atomic_dec_return(&hv->synic_auto_eoi_used) == 0)
+			kvm_request_apicv_update(vcpu->kvm, true,
+						 APICV_INHIBIT_REASON_HYPERV);
+	}
 }
 
 static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
@@ -931,12 +950,6 @@ int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages)
 
 	synic = to_hv_synic(vcpu);
 
-	/*
-	 * Hyper-V SynIC auto EOI SINT's are
-	 * not compatible with APICV, so request
-	 * to deactivate APICV permanently.
-	 */
-	kvm_request_apicv_update(vcpu->kvm, false, APICV_INHIBIT_REASON_HYPERV);
 	synic->active = true;
 	synic->dont_zero_synic_pages = dont_zero_synic_pages;
 	synic->control = HV_SYNIC_CONTROL_ENABLE;
@@ -2198,6 +2211,8 @@ int kvm_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
 				ent->eax |= HV_X64_ENLIGHTENED_VMCS_RECOMMENDED;
 			if (!cpu_smt_possible())
 				ent->eax |= HV_X64_NO_NONARCH_CORESHARING;
+			if (enable_apicv)
+				ent->eax |= HV_DEPRECATING_AEOI_RECOMMENDED;
 			/*
 			 * Default number of spinlock retry attempts, matches
 			 * HyperV 2016.
-- 
2.31.1

Re: [PATCH v2 5/5] KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in use

[Paolo Bonzini]

On 18/05/21 16:43, Vitaly Kuznetsov wrote:
> APICV_INHIBIT_REASON_HYPERV is currently unconditionally forced upon
> SynIC activation as SynIC's AutoEOI is incompatible with APICv/AVIC. It is,
> however, possible to track whether the feature was actually used by the
> guest and only inhibit APICv/AVIC when needed.
> 
> TLFS suggests a dedicated 'HV_DEPRECATING_AEOI_RECOMMENDED' flag to let
> Windows know that AutoEOI feature should be avoided. While it's up to
> KVM userspace to set the flag, KVM can help a bit by exposing global
> APICv/AVIC enablement: in case APICv/AVIC usage is impossible, AutoEOI
> is still preferred.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Should it also disable APICv unconditionally if 
HV_DEPRECATING_AEOI_RECOMMENDED is not in the guest CPUID?  That should 
avoid ping-pong between enabled and disabled APICv even in pathological 
cases that we cannot think about.

Paolo

> ---
>   arch/x86/include/asm/kvm_host.h |  3 +++
>   arch/x86/kvm/hyperv.c           | 27 +++++++++++++++++++++------
>   2 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index bf5807d35339..5e03ab4c0e4f 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -936,6 +936,9 @@ struct kvm_hv {
>   	/* How many vCPUs have VP index != vCPU index */
>   	atomic_t num_mismatched_vp_indexes;
>   
> +	/* How many SynICs use 'AutoEOI' feature */
> +	atomic_t synic_auto_eoi_used;
> +
>   	struct hv_partition_assist_pg *hv_pa_pg;
>   	struct kvm_hv_syndbg hv_syndbg;
>   };
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index f98370a39936..89e7d5b99279 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -87,6 +87,10 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic,
>   static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
>   				int vector)
>   {
> +	struct kvm_vcpu *vcpu = hv_synic_to_vcpu(synic);
> +	struct kvm_hv *hv = to_kvm_hv(vcpu->kvm);
> +	int auto_eoi_old, auto_eoi_new;
> +
>   	if (vector < HV_SYNIC_FIRST_VALID_VECTOR)
>   		return;
>   
> @@ -95,10 +99,25 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
>   	else
>   		__clear_bit(vector, synic->vec_bitmap);
>   
> +	auto_eoi_old = bitmap_weight(synic->auto_eoi_bitmap, 256);
> +
>   	if (synic_has_vector_auto_eoi(synic, vector))
>   		__set_bit(vector, synic->auto_eoi_bitmap);
>   	else
>   		__clear_bit(vector, synic->auto_eoi_bitmap);
> +
> +	auto_eoi_new = bitmap_weight(synic->auto_eoi_bitmap, 256);
> +
> +	/* Hyper-V SynIC auto EOI SINTs are not compatible with APICV */
> +	if (!auto_eoi_old && auto_eoi_new) {
> +		if (atomic_inc_return(&hv->synic_auto_eoi_used) == 1)
> +			kvm_request_apicv_update(vcpu->kvm, false,
> +						 APICV_INHIBIT_REASON_HYPERV);
> +	} else if (!auto_eoi_new && auto_eoi_old) {
> +		if (atomic_dec_return(&hv->synic_auto_eoi_used) == 0)
> +			kvm_request_apicv_update(vcpu->kvm, true,
> +						 APICV_INHIBIT_REASON_HYPERV);
> +	}
>   }
>   
>   static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
> @@ -931,12 +950,6 @@ int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages)
>   
>   	synic = to_hv_synic(vcpu);
>   
> -	/*
> -	 * Hyper-V SynIC auto EOI SINT's are
> -	 * not compatible with APICV, so request
> -	 * to deactivate APICV permanently.
> -	 */
> -	kvm_request_apicv_update(vcpu->kvm, false, APICV_INHIBIT_REASON_HYPERV);
>   	synic->active = true;
>   	synic->dont_zero_synic_pages = dont_zero_synic_pages;
>   	synic->control = HV_SYNIC_CONTROL_ENABLE;
> @@ -2198,6 +2211,8 @@ int kvm_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
>   				ent->eax |= HV_X64_ENLIGHTENED_VMCS_RECOMMENDED;
>   			if (!cpu_smt_possible())
>   				ent->eax |= HV_X64_NO_NONARCH_CORESHARING;
> +			if (enable_apicv)
> +				ent->eax |= HV_DEPRECATING_AEOI_RECOMMENDED;
>   			/*
>   			 * Default number of spinlock retry attempts, matches
>   			 * HyperV 2016.
> 

Re: [PATCH v2 5/5] KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in use

[Vitaly Kuznetsov]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 18/05/21 16:43, Vitaly Kuznetsov wrote:
>> APICV_INHIBIT_REASON_HYPERV is currently unconditionally forced upon
>> SynIC activation as SynIC's AutoEOI is incompatible with APICv/AVIC. It is,
>> however, possible to track whether the feature was actually used by the
>> guest and only inhibit APICv/AVIC when needed.
>> 
>> TLFS suggests a dedicated 'HV_DEPRECATING_AEOI_RECOMMENDED' flag to let
>> Windows know that AutoEOI feature should be avoided. While it's up to
>> KVM userspace to set the flag, KVM can help a bit by exposing global
>> APICv/AVIC enablement: in case APICv/AVIC usage is impossible, AutoEOI
>> is still preferred.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>
> Should it also disable APICv unconditionally if 
> HV_DEPRECATING_AEOI_RECOMMENDED is not in the guest CPUID?  That should 
> avoid ping-pong between enabled and disabled APICv even in pathological 
> cases that we cannot think about.

When you run Hyper-V on KVM it doesn't use SynIC (let alone AutoEOI) but
we still inhibit APICv unconditionally. The patch as-is improves this
without any userspace changes required and I see it as a benefit. Going
forward, we will definitely add something like 'hv-synic-noaeoi' to QEMU
to make non-nesting setups benefit too but it'll take a while for this
option to propagate to real world configurations (sigh).

-- 
Vitaly

Re: [PATCH v2 5/5] KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in use

[Paolo Bonzini]

On 25/05/21 08:23, Vitaly Kuznetsov wrote:
>> Should it also disable APICv unconditionally if
>> HV_DEPRECATING_AEOI_RECOMMENDED is not in the guest CPUID?  That should
>> avoid ping-pong between enabled and disabled APICv even in pathological
>> cases that we cannot think about.
> When you run Hyper-V on KVM it doesn't use SynIC (let alone AutoEOI) but
> we still inhibit APICv unconditionally. The patch as-is improves this
> without any userspace changes required and I see it as a benefit. Going
> forward, we will definitely add something like 'hv-synic-noaeoi' to QEMU
> to make non-nesting setups benefit too but it'll take a while for this
> option to propagate to real world configurations (sigh).

Ok, if enable<->disable APICv becomes an issue we can also consider 
disabling APICv forever if AutoEOI is ever used.

Paolo

Re: [PATCH v2 5/5] KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in use

[Maxim Levitsky]

On Mon, 2021-05-24 at 18:21 +0200, Paolo Bonzini wrote:
> On 18/05/21 16:43, Vitaly Kuznetsov wrote:
> > APICV_INHIBIT_REASON_HYPERV is currently unconditionally forced upon
> > SynIC activation as SynIC's AutoEOI is incompatible with APICv/AVIC. It is,
> > however, possible to track whether the feature was actually used by the
> > guest and only inhibit APICv/AVIC when needed.
> > 
> > TLFS suggests a dedicated 'HV_DEPRECATING_AEOI_RECOMMENDED' flag to let
> > Windows know that AutoEOI feature should be avoided. While it's up to
> > KVM userspace to set the flag, KVM can help a bit by exposing global
> > APICv/AVIC enablement: in case APICv/AVIC usage is impossible, AutoEOI
> > is still preferred.
> > 
> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> 
> Should it also disable APICv unconditionally if 
> HV_DEPRECATING_AEOI_RECOMMENDED is not in the guest CPUID?  That should 
> avoid ping-pong between enabled and disabled APICv even in pathological 
> cases that we cannot think about.

Probably not worth it, as the guest might still not use it.
We already disable/enable AVIC at the rate of a few iterations
per second when PIC sends its interrupts via ExtINT,
and we need an interrupt window.

This is sadly something that windows still does use and
it seems to work.

Best regards,
	Maxim Levitsky

> 
> Paolo
> 
> > ---
> >   arch/x86/include/asm/kvm_host.h |  3 +++
> >   arch/x86/kvm/hyperv.c           | 27 +++++++++++++++++++++------
> >   2 files changed, 24 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index bf5807d35339..5e03ab4c0e4f 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -936,6 +936,9 @@ struct kvm_hv {
> >   	/* How many vCPUs have VP index != vCPU index */
> >   	atomic_t num_mismatched_vp_indexes;
> >   
> > +	/* How many SynICs use 'AutoEOI' feature */
> > +	atomic_t synic_auto_eoi_used;
> > +
> >   	struct hv_partition_assist_pg *hv_pa_pg;
> >   	struct kvm_hv_syndbg hv_syndbg;
> >   };
> > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > index f98370a39936..89e7d5b99279 100644
> > --- a/arch/x86/kvm/hyperv.c
> > +++ b/arch/x86/kvm/hyperv.c
> > @@ -87,6 +87,10 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic,
> >   static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
> >   				int vector)
> >   {
> > +	struct kvm_vcpu *vcpu = hv_synic_to_vcpu(synic);
> > +	struct kvm_hv *hv = to_kvm_hv(vcpu->kvm);
> > +	int auto_eoi_old, auto_eoi_new;
> > +
> >   	if (vector < HV_SYNIC_FIRST_VALID_VECTOR)
> >   		return;
> >   
> > @@ -95,10 +99,25 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
> >   	else
> >   		__clear_bit(vector, synic->vec_bitmap);
> >   
> > +	auto_eoi_old = bitmap_weight(synic->auto_eoi_bitmap, 256);
> > +
> >   	if (synic_has_vector_auto_eoi(synic, vector))
> >   		__set_bit(vector, synic->auto_eoi_bitmap);
> >   	else
> >   		__clear_bit(vector, synic->auto_eoi_bitmap);
> > +
> > +	auto_eoi_new = bitmap_weight(synic->auto_eoi_bitmap, 256);
> > +
> > +	/* Hyper-V SynIC auto EOI SINTs are not compatible with APICV */
> > +	if (!auto_eoi_old && auto_eoi_new) {
> > +		if (atomic_inc_return(&hv->synic_auto_eoi_used) == 1)
> > +			kvm_request_apicv_update(vcpu->kvm, false,
> > +						 APICV_INHIBIT_REASON_HYPERV);
> > +	} else if (!auto_eoi_new && auto_eoi_old) {
> > +		if (atomic_dec_return(&hv->synic_auto_eoi_used) == 0)
> > +			kvm_request_apicv_update(vcpu->kvm, true,
> > +						 APICV_INHIBIT_REASON_HYPERV);
> > +	}
> >   }
> >   
> >   static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
> > @@ -931,12 +950,6 @@ int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages)
> >   
> >   	synic = to_hv_synic(vcpu);
> >   
> > -	/*
> > -	 * Hyper-V SynIC auto EOI SINT's are
> > -	 * not compatible with APICV, so request
> > -	 * to deactivate APICV permanently.
> > -	 */
> > -	kvm_request_apicv_update(vcpu->kvm, false, APICV_INHIBIT_REASON_HYPERV);
> >   	synic->active = true;
> >   	synic->dont_zero_synic_pages = dont_zero_synic_pages;
> >   	synic->control = HV_SYNIC_CONTROL_ENABLE;
> > @@ -2198,6 +2211,8 @@ int kvm_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
> >   				ent->eax |= HV_X64_ENLIGHTENED_VMCS_RECOMMENDED;
> >   			if (!cpu_smt_possible())
> >   				ent->eax |= HV_X64_NO_NONARCH_CORESHARING;
> > +			if (enable_apicv)
> > +				ent->eax |= HV_DEPRECATING_AEOI_RECOMMENDED;
> >   			/*
> >   			 * Default number of spinlock retry attempts, matches
> >   			 * HyperV 2016.
> > 

Re: [PATCH v2 5/5] KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in use

[Maxim Levitsky]

On Tue, 2021-05-18 at 16:43 +0200, Vitaly Kuznetsov wrote:
> APICV_INHIBIT_REASON_HYPERV is currently unconditionally forced upon
> SynIC activation as SynIC's AutoEOI is incompatible with APICv/AVIC. It is,
> however, possible to track whether the feature was actually used by the
> guest and only inhibit APICv/AVIC when needed.
> 
> TLFS suggests a dedicated 'HV_DEPRECATING_AEOI_RECOMMENDED' flag to let
> Windows know that AutoEOI feature should be avoided. While it's up to
> KVM userspace to set the flag, KVM can help a bit by exposing global
> APICv/AVIC enablement: in case APICv/AVIC usage is impossible, AutoEOI
> is still preferred.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  3 +++
>  arch/x86/kvm/hyperv.c           | 27 +++++++++++++++++++++------
>  2 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index bf5807d35339..5e03ab4c0e4f 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -936,6 +936,9 @@ struct kvm_hv {
>  	/* How many vCPUs have VP index != vCPU index */
>  	atomic_t num_mismatched_vp_indexes;
>  
> +	/* How many SynICs use 'AutoEOI' feature */
> +	atomic_t synic_auto_eoi_used;
> +
>  	struct hv_partition_assist_pg *hv_pa_pg;
>  	struct kvm_hv_syndbg hv_syndbg;
>  };
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index f98370a39936..89e7d5b99279 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -87,6 +87,10 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic,
>  static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
>  				int vector)
>  {
> +	struct kvm_vcpu *vcpu = hv_synic_to_vcpu(synic);
> +	struct kvm_hv *hv = to_kvm_hv(vcpu->kvm);
> +	int auto_eoi_old, auto_eoi_new;
> +
>  	if (vector < HV_SYNIC_FIRST_VALID_VECTOR)
>  		return;
>  
> @@ -95,10 +99,25 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
>  	else
>  		__clear_bit(vector, synic->vec_bitmap);
>  
> +	auto_eoi_old = bitmap_weight(synic->auto_eoi_bitmap, 256);
> +
>  	if (synic_has_vector_auto_eoi(synic, vector))
>  		__set_bit(vector, synic->auto_eoi_bitmap);
>  	else
>  		__clear_bit(vector, synic->auto_eoi_bitmap);
> +
> +	auto_eoi_new = bitmap_weight(synic->auto_eoi_bitmap, 256);
> +
> +	/* Hyper-V SynIC auto EOI SINTs are not compatible with APICV */
> +	if (!auto_eoi_old && auto_eoi_new) {
> +		if (atomic_inc_return(&hv->synic_auto_eoi_used) == 1)
> +			kvm_request_apicv_update(vcpu->kvm, false,
> +						 APICV_INHIBIT_REASON_HYPERV);
> +	} else if (!auto_eoi_new && auto_eoi_old) {
> +		if (atomic_dec_return(&hv->synic_auto_eoi_used) == 0)
> +			kvm_request_apicv_update(vcpu->kvm, true,
> +						 APICV_INHIBIT_REASON_HYPERV);
> +	}

A summary of a bug as I explained in my main reply to the patch series:
synic_update_vector can be called on vmexit, holding the SRCU lock,
and it can't currently call kvm_request_apicv_update with SRCU lock held.
because kvm_request_apicv_update indirectly calls synchronize_srcu.

Either we have to add a parameter 'host' synic_update_vector 
that will specify that this function was 
called on msr write from userspace, or from the guest, and for the latter 
drop the srcu lock around kvm_request_apicv_update as it is done in 
svm_toggle_avic_for_irq_window or we should think on how
we can make kvm_request_apicv_update avoid the need to use srcu lock.

We can for example make it not run avic memslot update on current vcpu,
for the synic case, or maybe we can make it avoid memslots update completely.

Other than this bug, especially after I did read the SynIC 
spec, this looks reasonable.

One thing though that I noticed in the SynIC spec is that 
regardless of AutoEOI setting, when we do intercept EOI 
(which we can't with AVIC) (in apic_set_eoi) 
we call kvm_hv_synic_send_eoi, which seems to try to raise stimer again 
on this SINIx.
This is not relevant if STIMER is in direct mode though, then I think
we don't really send anything through SynIC anyway.

So besides the SRCU bug:

Tested-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky


>  }
>  
>  static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
> @@ -931,12 +950,6 @@ int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages)
>  
>  	synic = to_hv_synic(vcpu);
>  
> -	/*
> -	 * Hyper-V SynIC auto EOI SINT's are
> -	 * not compatible with APICV, so request
> -	 * to deactivate APICV permanently.
> -	 */
> -	kvm_request_apicv_update(vcpu->kvm, false, APICV_INHIBIT_REASON_HYPERV);
>  	synic->active = true;
>  	synic->dont_zero_synic_pages = dont_zero_synic_pages;
>  	synic->control = HV_SYNIC_CONTROL_ENABLE;
> @@ -2198,6 +2211,8 @@ int kvm_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
>  				ent->eax |= HV_X64_ENLIGHTENED_VMCS_RECOMMENDED;
>  			if (!cpu_smt_possible())
>  				ent->eax |= HV_X64_NO_NONARCH_CORESHARING;
> +			if (enable_apicv)
> +				ent->eax |= HV_DEPRECATING_AEOI_RECOMMENDED;
>  			/*
>  			 * Default number of spinlock retry attempts, matches
>  			 * HyperV 2016.

Re: [PATCH v2 0/5] KVM: x86: hyper-v: Conditionally allow SynIC with APICv/AVIC

[Maxim Levitsky]

On Tue, 2021-05-18 at 16:43 +0200, Vitaly Kuznetsov wrote:
> Changes since v1 (Sean):
> - Use common 'enable_apicv' variable for both APICv and AVIC instead of 
>  adding a new hook to 'struct kvm_x86_ops'.
> - Drop unneded CONFIG_X86_LOCAL_APIC checks from VMX/SVM code along the
>  way.
> 
> Original description:
> 
> APICV_INHIBIT_REASON_HYPERV is currently unconditionally forced upon
> SynIC activation as SynIC's AutoEOI is incompatible with APICv/AVIC. It is,
> however, possible to track whether the feature was actually used by the
> guest and only inhibit APICv/AVIC when needed.
> 
> The feature can be tested with QEMU's 'hv-passthrough' debug mode.
> 
> Note, 'avic' kvm-amd module parameter is '0' by default and thus needs to
> be explicitly enabled.
> 
> Vitaly Kuznetsov (5):
>   KVM: SVM: Drop unneeded CONFIG_X86_LOCAL_APIC check for AVIC
>   KVM: VMX: Drop unneeded CONFIG_X86_LOCAL_APIC check from
>     cpu_has_vmx_posted_intr()
>   KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC
>   KVM: x86: Invert APICv/AVIC enablement check
>   KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in
>     use
> 
>  arch/x86/include/asm/kvm_host.h |  5 ++++-
>  arch/x86/kvm/hyperv.c           | 27 +++++++++++++++++++++------
>  arch/x86/kvm/svm/avic.c         | 16 +++++-----------
>  arch/x86/kvm/svm/svm.c          | 24 +++++++++++++-----------
>  arch/x86/kvm/svm/svm.h          |  2 --
>  arch/x86/kvm/vmx/capabilities.h |  4 +---
>  arch/x86/kvm/vmx/vmx.c          |  2 --
>  arch/x86/kvm/x86.c              |  9 ++++++---
>  8 files changed, 50 insertions(+), 39 deletions(-)
> 

I tested this patch set and this is what I found.

For reference,
First of all, indeed to make AVIC work I need to:
 
1. Disable SVM - I wonder if I can make this on demand
too when the guest actually uses a nested guest or at least
enables nesting in IA32_FEATURE_CONTROL.
I naturally run most of my VMs with nesting enabled,
thus I tend to not have avic enabled due to this.
I'll prepare a patch soon for this.
 
2. Disable x2apic, naturally x2apic can't be used with avic.
In theory we can also disable avic when the guest switches on
the x2apic mode, but in practice the guest will likely to pick the x2apic
when it can.
 
3. (for hyperv) Disable 'hv_vapic', because otherwise hyper-v
uses its own PV APIC msrs which AVIC doesn't support.

This HV enlightment turns on in the CPUID both the 
HV_APIC_ACCESS_AVAILABLE which isn't that bad 
(it only tells that we have the VP assist page),
and HV_APIC_ACCESS_RECOMMENDED which hints the guest
to use HyperV PV APIC MSRS and use PV EOI field in 
the APIC access page, which means that the guest 
won't use the real apic at all.

4. and of course enable SynIC autoeoi deprecation.

Otherwise indeed windows enables autoeoi.

hv-passthrough indeed can't be used to test this
as it both enables autoeoi depreciation and *hv-vapic*. 
I had to use the patch that you posted
in 'About the performance of hyper-v' thread.
 
In addition to that when I don't use the autoeoi depreciation patch,
then the guest indeed enables autoeoi, and this triggers a deadlock.
 
The reason is that kvm_request_apicv_update must not be called with
srcu lock held vcpu->kvm->srcu (there is a warning about that
in kvm_request_apicv_update), but guest msr writes which come
from vcpu thread do hold it.
 
The other place where we disable AVIC on demand is svm_toggle_avic_for_irq_window.
And that code has a hack to drop this lock and take 
it back around the call to kvm_request_apicv_update.
This hack is safe as this code is called only from the vcpu thread.
 
Also for reference the reason for the fact that we need to
disable AVIC on the interrupt window request, or more correctly
why we still need to request interrupt windows with AVIC,
is that the local apic can act sadly as a pass-through device 
for legacy PIC, when one of its LINTn pins is configured in ExtINT mode.
In this mode when such pin is raised, the local apic asks the PIC for
the interrupt vector and then delivers it to the APIC
without touching the IRR/ISR.

The later means that if guest's interrupts are disabled,
such interrupt can't be queued via IRR to VAPIC
but instead the regular interrupt window has to be requested, 
but on AMD, the only way to request interrupt window
is to queue a VIRQ, and intercept its delivery,
a feature that is disabled when AVIC is active.
 
Finally for SynIC this srcu lock drop hack can be extended to this gross hack:
It seems to work though:


diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index bedd9b6cc26a..925b76e7b45e 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -85,7 +85,7 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic,
 }
 
 static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
-				int vector)
+				int vector, bool host)
 {
 	struct kvm_vcpu *vcpu = hv_synic_to_vcpu(synic);
 	struct kvm_hv *hv = to_kvm_hv(vcpu->kvm);
@@ -109,6 +109,9 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
 
 	auto_eoi_new = bitmap_weight(synic->auto_eoi_bitmap, 256);
 
+	if (!host)
+		srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
+
 	/* Hyper-V SynIC auto EOI SINTs are not compatible with APICV */
 	if (!auto_eoi_old && auto_eoi_new) {
 		printk("Synic: inhibiting avic %d %d\n", auto_eoi_old, auto_eoi_new);
@@ -121,6 +124,10 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
 			kvm_request_apicv_update(vcpu->kvm, true,
 						 APICV_INHIBIT_REASON_HYPERV);
 	}
+
+	if (!host)
+		vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
+
 }
 
 static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
@@ -149,9 +156,9 @@ static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
 
 	atomic64_set(&synic->sint[sint], data);
 
-	synic_update_vector(synic, old_vector);
+	synic_update_vector(synic, old_vector, host);
 
-	synic_update_vector(synic, vector);
+	synic_update_vector(synic, vector, host);
 
 	/* Load SynIC vectors into EOI exit bitmap */
 	kvm_make_request(KVM_REQ_SCAN_IOAPIC, hv_synic_to_vcpu(synic));


Assuming that we don't want this gross hack,  
I wonder if we can avoid full blown memslot 
update when we disable avic, but rather have some 
smaller hack like only manually patching its
NPT mapping to have RW permissions instead 
of reserved bits which we use for MMIO. 

The AVIC spec says that NPT is only used to check that
guest has RW permission to the page, 
while the HVA in the NPT entry itself is ignored.

Best regards,
	Maxim Levitsky

Re: [PATCH v2 0/5] KVM: x86: hyper-v: Conditionally allow SynIC with APICv/AVIC

[Vitaly Kuznetsov]

Maxim Levitsky <mlevitsk@redhat.com> writes:

> On Tue, 2021-05-18 at 16:43 +0200, Vitaly Kuznetsov wrote:
>> Changes since v1 (Sean):
>> - Use common 'enable_apicv' variable for both APICv and AVIC instead of 
>>  adding a new hook to 'struct kvm_x86_ops'.
>> - Drop unneded CONFIG_X86_LOCAL_APIC checks from VMX/SVM code along the
>>  way.
>> 
>> Original description:
>> 
>> APICV_INHIBIT_REASON_HYPERV is currently unconditionally forced upon
>> SynIC activation as SynIC's AutoEOI is incompatible with APICv/AVIC. It is,
>> however, possible to track whether the feature was actually used by the
>> guest and only inhibit APICv/AVIC when needed.
>> 
>> The feature can be tested with QEMU's 'hv-passthrough' debug mode.
>> 
>> Note, 'avic' kvm-amd module parameter is '0' by default and thus needs to
>> be explicitly enabled.
>> 
>> Vitaly Kuznetsov (5):
>>   KVM: SVM: Drop unneeded CONFIG_X86_LOCAL_APIC check for AVIC
>>   KVM: VMX: Drop unneeded CONFIG_X86_LOCAL_APIC check from
>>     cpu_has_vmx_posted_intr()
>>   KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC
>>   KVM: x86: Invert APICv/AVIC enablement check
>>   KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in
>>     use
>> 
>>  arch/x86/include/asm/kvm_host.h |  5 ++++-
>>  arch/x86/kvm/hyperv.c           | 27 +++++++++++++++++++++------
>>  arch/x86/kvm/svm/avic.c         | 16 +++++-----------
>>  arch/x86/kvm/svm/svm.c          | 24 +++++++++++++-----------
>>  arch/x86/kvm/svm/svm.h          |  2 --
>>  arch/x86/kvm/vmx/capabilities.h |  4 +---
>>  arch/x86/kvm/vmx/vmx.c          |  2 --
>>  arch/x86/kvm/x86.c              |  9 ++++++---
>>  8 files changed, 50 insertions(+), 39 deletions(-)
>> 
>
> I tested this patch set and this is what I found.
>
> For reference,
> First of all, indeed to make AVIC work I need to:
>  
> 1. Disable SVM - I wonder if I can make this on demand
> too when the guest actually uses a nested guest or at least
> enables nesting in IA32_FEATURE_CONTROL.
> I naturally run most of my VMs with nesting enabled,
> thus I tend to not have avic enabled due to this.
> I'll prepare a patch soon for this.
>  
> 2. Disable x2apic, naturally x2apic can't be used with avic.
> In theory we can also disable avic when the guest switches on
> the x2apic mode, but in practice the guest will likely to pick the x2apic
> when it can.
>  
> 3. (for hyperv) Disable 'hv_vapic', because otherwise hyper-v
> uses its own PV APIC msrs which AVIC doesn't support.
>
> This HV enlightment turns on in the CPUID both the 
> HV_APIC_ACCESS_AVAILABLE which isn't that bad 
> (it only tells that we have the VP assist page),
> and HV_APIC_ACCESS_RECOMMENDED which hints the guest
> to use HyperV PV APIC MSRS and use PV EOI field in 
> the APIC access page, which means that the guest 
> won't use the real apic at all.
>
> 4. and of course enable SynIC autoeoi deprecation.
>
> Otherwise indeed windows enables autoeoi.
>
> hv-passthrough indeed can't be used to test this
> as it both enables autoeoi depreciation and *hv-vapic*. 
> I had to use the patch that you posted
> in 'About the performance of hyper-v' thread.
>  
> In addition to that when I don't use the autoeoi depreciation patch,
> then the guest indeed enables autoeoi, and this triggers a deadlock.
>  

Hm, why don't I see in my testing? I'm pretty sure I'm testing both
cases...

> The reason is that kvm_request_apicv_update must not be called with
> srcu lock held vcpu->kvm->srcu (there is a warning about that
> in kvm_request_apicv_update), but guest msr writes which come
> from vcpu thread do hold it.
>  
> The other place where we disable AVIC on demand is svm_toggle_avic_for_irq_window.
> And that code has a hack to drop this lock and take 
> it back around the call to kvm_request_apicv_update.
> This hack is safe as this code is called only from the vcpu thread.
>  
> Also for reference the reason for the fact that we need to
> disable AVIC on the interrupt window request, or more correctly
> why we still need to request interrupt windows with AVIC,
> is that the local apic can act sadly as a pass-through device 
> for legacy PIC, when one of its LINTn pins is configured in ExtINT mode.
> In this mode when such pin is raised, the local apic asks the PIC for
> the interrupt vector and then delivers it to the APIC
> without touching the IRR/ISR.
>
> The later means that if guest's interrupts are disabled,
> such interrupt can't be queued via IRR to VAPIC
> but instead the regular interrupt window has to be requested, 
> but on AMD, the only way to request interrupt window
> is to queue a VIRQ, and intercept its delivery,
> a feature that is disabled when AVIC is active.
>  
> Finally for SynIC this srcu lock drop hack can be extended to this gross hack:
> It seems to work though:
>
>
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index bedd9b6cc26a..925b76e7b45e 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -85,7 +85,7 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic,
>  }
>  
>  static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
> -				int vector)
> +				int vector, bool host)
>  {
>  	struct kvm_vcpu *vcpu = hv_synic_to_vcpu(synic);
>  	struct kvm_hv *hv = to_kvm_hv(vcpu->kvm);
> @@ -109,6 +109,9 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
>  
>  	auto_eoi_new = bitmap_weight(synic->auto_eoi_bitmap, 256);
>  
> +	if (!host)
> +		srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
> +
>  	/* Hyper-V SynIC auto EOI SINTs are not compatible with APICV */
>  	if (!auto_eoi_old && auto_eoi_new) {
>  		printk("Synic: inhibiting avic %d %d\n", auto_eoi_old, auto_eoi_new);
> @@ -121,6 +124,10 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
>  			kvm_request_apicv_update(vcpu->kvm, true,
>  						 APICV_INHIBIT_REASON_HYPERV);
>  	}
> +
> +	if (!host)
> +		vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
> +
>  }
>  
>  static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
> @@ -149,9 +156,9 @@ static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
>  
>  	atomic64_set(&synic->sint[sint], data);
>  
> -	synic_update_vector(synic, old_vector);
> +	synic_update_vector(synic, old_vector, host);
>  
> -	synic_update_vector(synic, vector);
> +	synic_update_vector(synic, vector, host);
>  
>  	/* Load SynIC vectors into EOI exit bitmap */
>  	kvm_make_request(KVM_REQ_SCAN_IOAPIC, hv_synic_to_vcpu(synic));
>
>
> Assuming that we don't want this gross hack,  

Is it dangerous or just ugly?

> I wonder if we can avoid full blown memslot 
> update when we disable avic, but rather have some 
> smaller hack like only manually patching its
> NPT mapping to have RW permissions instead 
> of reserved bits which we use for MMIO. 
>
> The AVIC spec says that NPT is only used to check that
> guest has RW permission to the page, 
> while the HVA in the NPT entry itself is ignored.

Assuming kvm_request_apicv_update() is called very rarely, I'd rather
kicked all vCPUs out (similar to KVM_REQ_MCLOCK_INPROGRESS) and
schedule_work() to make memslot update happen ourside of sRCU lock.

>
> Best regards,
> 	Maxim Levitsky
>
>
>
>
>

-- 
Vitaly

Re: [PATCH v2 0/5] KVM: x86: hyper-v: Conditionally allow SynIC with APICv/AVIC

[Maxim Levitsky]

On Thu, 2021-05-27 at 10:35 +0200, Vitaly Kuznetsov wrote:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
> 
> > On Tue, 2021-05-18 at 16:43 +0200, Vitaly Kuznetsov wrote:
> > > Changes since v1 (Sean):
> > > - Use common 'enable_apicv' variable for both APICv and AVIC instead of 
> > >  adding a new hook to 'struct kvm_x86_ops'.
> > > - Drop unneded CONFIG_X86_LOCAL_APIC checks from VMX/SVM code along the
> > >  way.
> > > 
> > > Original description:
> > > 
> > > APICV_INHIBIT_REASON_HYPERV is currently unconditionally forced upon
> > > SynIC activation as SynIC's AutoEOI is incompatible with APICv/AVIC. It is,
> > > however, possible to track whether the feature was actually used by the
> > > guest and only inhibit APICv/AVIC when needed.
> > > 
> > > The feature can be tested with QEMU's 'hv-passthrough' debug mode.
> > > 
> > > Note, 'avic' kvm-amd module parameter is '0' by default and thus needs to
> > > be explicitly enabled.
> > > 
> > > Vitaly Kuznetsov (5):
> > >   KVM: SVM: Drop unneeded CONFIG_X86_LOCAL_APIC check for AVIC
> > >   KVM: VMX: Drop unneeded CONFIG_X86_LOCAL_APIC check from
> > >     cpu_has_vmx_posted_intr()
> > >   KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC
> > >   KVM: x86: Invert APICv/AVIC enablement check
> > >   KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in
> > >     use
> > > 
> > >  arch/x86/include/asm/kvm_host.h |  5 ++++-
> > >  arch/x86/kvm/hyperv.c           | 27 +++++++++++++++++++++------
> > >  arch/x86/kvm/svm/avic.c         | 16 +++++-----------
> > >  arch/x86/kvm/svm/svm.c          | 24 +++++++++++++-----------
> > >  arch/x86/kvm/svm/svm.h          |  2 --
> > >  arch/x86/kvm/vmx/capabilities.h |  4 +---
> > >  arch/x86/kvm/vmx/vmx.c          |  2 --
> > >  arch/x86/kvm/x86.c              |  9 ++++++---
> > >  8 files changed, 50 insertions(+), 39 deletions(-)
> > > 
> > 
> > I tested this patch set and this is what I found.
> > 
> > For reference,
> > First of all, indeed to make AVIC work I need to:
> >  
> > 1. Disable SVM - I wonder if I can make this on demand
> > too when the guest actually uses a nested guest or at least
> > enables nesting in IA32_FEATURE_CONTROL.
> > I naturally run most of my VMs with nesting enabled,
> > thus I tend to not have avic enabled due to this.
> > I'll prepare a patch soon for this.
> >  
> > 2. Disable x2apic, naturally x2apic can't be used with avic.
> > In theory we can also disable avic when the guest switches on
> > the x2apic mode, but in practice the guest will likely to pick the x2apic
> > when it can.
> >  
> > 3. (for hyperv) Disable 'hv_vapic', because otherwise hyper-v
> > uses its own PV APIC msrs which AVIC doesn't support.
> > 
> > This HV enlightment turns on in the CPUID both the 
> > HV_APIC_ACCESS_AVAILABLE which isn't that bad 
> > (it only tells that we have the VP assist page),
> > and HV_APIC_ACCESS_RECOMMENDED which hints the guest
> > to use HyperV PV APIC MSRS and use PV EOI field in 
> > the APIC access page, which means that the guest 
> > won't use the real apic at all.
> > 
> > 4. and of course enable SynIC autoeoi deprecation.
> > 
> > Otherwise indeed windows enables autoeoi.
> > 
> > hv-passthrough indeed can't be used to test this
> > as it both enables autoeoi depreciation and *hv-vapic*. 
> > I had to use the patch that you posted
> > in 'About the performance of hyper-v' thread.
> >  
> > In addition to that when I don't use the autoeoi depreciation patch,
> > then the guest indeed enables autoeoi, and this triggers a deadlock.
> >  
> 
> Hm, why don't I see in my testing? I'm pretty sure I'm testing both
> cases...

Hi!

For me it hangs when windows enables the autoeoi for the first time.

I use 5.13-rc3 kernel with kvm/queue merged, your patches and some my patches
that shouldn't affect things. 
I use qemu commit 3791642c8d60029adf9b00bcb4e34d7d8a1aea4d

I use the following qemu command line:

/home/mlevitsk/Qemu/master/build-master/output/bin/qemu-system-x86_64
-smp 4
-name debug-threads=on
-pidfile /run/vmspawn/win10_ojiejx07/qemu.pid
-accel kvm,kernel-irqchip=on
-nodefaults
-display none
-smp maxcpus=64,sockets=1,cores=32,threads=2
-machine q35,sata=off,usb=off,vmport=off,smbus=off
-rtc base=localtime,clock=host
-global mc146818rtc.lost_tick_policy=discard
-global kvm-pit.lost_tick_policy=discard
-no-hpet
-global ICH9-LPC.disable_s3=1
-global ICH9-LPC.disable_s4=1
-boot menu=on,strict=on,splash-time=1000
-L .bios
-machine pflash0=flash0,pflash1=flash1,smm=off
-blockdev node-name=flash0,driver=file,filename=./.bios/OVMF_CODE_nosmm.fd,read-only=on
-blockdev node-name=flash1,driver=file,filename=./.bios/OVMF_VARS.fd
-device pcie-root-port,slot=0,id=rport.0,bus=pcie.0,addr=0x1c.0x0,multifunction=on
-device pcie-root-port,slot=1,id=rport.1,bus=pcie.0,addr=0x1c.0x1
-device pcie-root-port,slot=2,id=rport.2,bus=pcie.0,addr=0x1c.0x2
-device pcie-root-port,slot=3,id=rport.3,bus=pcie.0,addr=0x1c.0x3
-device pcie-root-port,slot=4,id=rport.4,bus=pcie.0,addr=0x1c.0x4
-device pcie-root-port,slot=5,id=rport.5,bus=pcie.0,addr=0x1c.0x5
-device pcie-root-port,slot=6,id=rport.6,bus=pcie.0,addr=0x1c.0x6
-device pcie-root-port,slot=7,id=rport.7,bus=pcie.0,addr=0x1c.0x7
-device pcie-root-port,slot=8,id=rport.8,bus=pcie.0,addr=0x1d.0x0,multifunction=on
-device pcie-root-port,slot=9,id=rport.9,bus=pcie.0,addr=0x1d.0x1
-device pcie-root-port,slot=10,id=rport.10,bus=pcie.0,addr=0x1d.0x2
-device pcie-root-port,slot=11,id=rport.11,bus=pcie.0,addr=0x1d.0x3
-cpu host,host-cache-info,hv_relaxed,hv_spinlocks=0x1fff,hv_vpindex,hv_runtime,hv_synic,hv-tlbflush,hv-frequencies,hv_stimer,hv-stimer-direct,hv_time,-x2apic,topoext,-svm,invtsc,hv-passthrough,-hv-
vapic
-overcommit mem-lock=on
-m 6G
-device virtio-scsi,id=scsi-ctrl,bus=rport.0,iothread=,num_queues=4
-drive if=none,id=os_image,file=./disk_s1.qcow2,aio=native,discard=unmap,cache=none
-device scsi-hd,drive=os_image,bus=scsi-ctrl.0,bootindex=1,id=auto_id21
-netdev tap,id=tap0,vhost=on,ifname=tap0_windows10,script=no,downscript=no
-device virtio-net-pci,id=net0,mac=02:00:00:A9:4D:A7,netdev=tap0,bus=rport.1,disable-legacy=on
-display gtk,gl=off,zoom-to-fit=on,window-close=off
-device virtio-vga,virgl=off,id=auto_id23
-device qemu-xhci,id=usb0,bus=pcie.0,addr=0x14.0x0,p3=16,p2=16
-device usb-tablet,id=auto_id24
-audiodev pa,id=pulseaudio0,server=/run/user/103992/pulse/native,timer-period=2000,out.mixing-engine=off,out.fixed-settings=off,out.buffer-length=50000
-device ich9-intel-hda,id=sound0,msi=on,bus=pcie.0,addr=0x1f.0x4
-device hda-micro,id=sound0-codec0,bus=sound0.0,cad=0,audiodev=pulseaudio0
-device virtio-serial-pci,id=virtio-serial0,bus=rport.2,disable-legacy=on
-chardev socket,id=chr_qga,path=/run/vmspawn/win10_ojiejx07/guest_agent.socket,server,nowait
-device virtserialport,bus=virtio-serial0.0,nr=1,chardev=chr_qga,name=org.qemu.guest_agent.0,id=auto_id25
-chardev socket,path=/run/vmspawn/win10_ojiejx07/hmp_monitor.socket,id=internal_hmp_monitor_socket_chardev,server=on,wait=off
-mon chardev=internal_hmp_monitor_socket_chardev,mode=readline
-chardev socket,path=/run/vmspawn/win10_ojiejx07/qmp_monitor.socket,id=internal_qmp_monitor_socket_chardev,server=on,wait=off
-mon chardev=internal_qmp_monitor_socket_chardev,mode=control
-chardev socket,path=/run/vmspawn/win10_ojiejx07/serial.socket,id=internal_serial0_chardev,server=on,logfile=/mnt/shared/home/mlevitsk/VM/win10/.logs/serial.log,wait=off
-device isa-serial,chardev=internal_serial0_chardev,index=0,id=auto_id28
-chardev file,path=/mnt/shared/home/mlevitsk/VM/win10/.logs/ovmf.log,id=internal_debugcon0_chardev
-device isa-debugcon,chardev=internal_debugcon0_chardev,iobase=1026,id=auto_id29


And then I get this eventually in dmesg:

[  245.196253] INFO: task qemu-system-x86:3487 blocked for more than 122 seconds.
[  245.196461]       Tainted: G           O      5.13.0-rc2.unstable #28
[  245.196640] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[  245.196855] task:qemu-system-x86 state:D stack:    0 pid: 3487 ppid:  3480 flags:0x00000000
[  245.197095] Call Trace:
[  245.197194]  __schedule+0x2d0/0x940
[  245.197307]  schedule+0x4f/0xc0
[  245.197402]  schedule_preempt_disabled+0xe/0x20
[  245.197535]  __mutex_lock.constprop.0+0x2ab/0x480
[  245.197675]  __mutex_lock_slowpath+0x13/0x20
[  245.197802]  mutex_lock+0x34/0x40
[  245.197905]  kvm_vm_ioctl+0x395/0xee0 [kvm]
[  245.198092]  ? _copy_to_user+0x20/0x40
[  245.198213]  ? put_timespec64+0x3d/0x60
[  245.198334]  ? poll_select_finish+0x1b3/0x220
[  245.198465]  __x64_sys_ioctl+0x8e/0xc0
[  245.198577]  do_syscall_64+0x3a/0x80
[  245.198688]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[  245.198838] RIP: 0033:0x7f66caec74eb
[  245.198944] RSP: 002b:00007ffd0e8ddc98 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
[  245.199157] RAX: ffffffffffffffda RBX: 00000000000a0000 RCX: 00007f66caec74eb
[  245.199354] RDX: 00007ffd0e8dde00 RSI: 000000004010ae42 RDI: 000000000000001e
[  245.199550] RBP: 00007ffd0e8ddd90 R08: 0000000000855628 R09: 0000000000000000
[  245.199745] R10: 00007ffd0e8ef080 R11: 0000000000000206 R12: 00000000004231b0
[  245.199948] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[  245.200206] INFO: task CPU 0/KVM:3543 blocked for more than 122 seconds.
[  245.200403]       Tainted: G           O      5.13.0-rc2.unstable #28
[  245.200590] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[  245.200812] task:CPU 0/KVM       state:D stack:    0 pid: 3543 ppid:  3480 flags:0x00004000
[  245.201047] Call Trace:
[  245.201122]  __schedule+0x2d0/0x940
[  245.201226]  schedule+0x4f/0xc0
[  245.201318]  schedule_timeout+0xfe/0x140
[  245.201432]  wait_for_completion+0x88/0xe0
[  245.201547]  __synchronize_srcu+0x79/0xa0
[  245.201662]  ? __bpf_trace_rcu_stall_warning+0x20/0x20
[  245.201808]  synchronize_srcu_expedited+0x1e/0x40
[  245.201941]  install_new_memslots+0x5c/0xa0 [kvm]
[  245.202122]  kvm_set_memslot+0x361/0x680 [kvm]
[  245.202292]  kvm_delete_memslot+0x68/0xe0 [kvm]
[  245.202464]  __kvm_set_memory_region+0x517/0x560 [kvm]
[  245.202653]  __x86_set_memory_region+0xe3/0x200 [kvm]
[  245.202848]  avic_update_access_page+0x75/0xa0 [kvm_amd]
[  245.203003]  svm_pre_update_apicv_exec_ctrl+0x12/0x20 [kvm_amd]
[  245.203176]  kvm_request_apicv_update+0xf6/0x160 [kvm]
[  245.203367]  synic_update_vector.cold+0x6d/0xb3 [kvm]
[  245.203565]  kvm_hv_set_msr_common+0x57e/0xc00 [kvm]
[  245.203760]  ? mutex_lock+0x13/0x40
[  245.203861]  kvm_set_msr_common+0x162/0xe60 [kvm]
[  245.204068]  svm_set_msr+0x40b/0x800 [kvm_amd]
[  245.204240]  __kvm_set_msr+0x8f/0x1e0 [kvm]
[  245.204430]  kvm_emulate_wrmsr+0x3a/0x180 [kvm]
[  245.204620]  msr_interception+0x1c/0x40 [kvm_amd]
[  245.204763]  svm_invoke_exit_handler+0x2a/0xe0 [kvm_amd]
[  245.204921]  handle_exit+0xb8/0x220 [kvm_amd]
[  245.205047]  kvm_arch_vcpu_ioctl_run+0xbe7/0x17c0 [kvm]
[  245.205251]  ? kthread_queue_work+0x3d/0x80
[  245.205370]  ? timerqueue_add+0x6e/0xc0
[  245.205482]  ? enqueue_hrtimer+0x39/0x80
[  245.205595]  kvm_vcpu_ioctl+0x247/0x600 [kvm]
[  245.205763]  ? tick_program_event+0x41/0x80
[  245.205887]  __x64_sys_ioctl+0x8e/0xc0
[  245.206002]  do_syscall_64+0x3a/0x80
[  245.206119]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[  245.206270] RIP: 0033:0x7f66caec74eb
[  245.206376] RSP: 002b:00007f66b56f7608 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[  245.206595] RAX: ffffffffffffffda RBX: 0000000002939360 RCX: 00007f66caec74eb
[  245.206799] RDX: 0000000000000000 RSI: 000000000000ae80 RDI: 0000000000000026
[  245.206998] RBP: 00007f66b56f7700 R08: 0000000000d87130 R09: 000000000000ffff
[  245.207198] R10: 0000000000918ea4 R11: 0000000000000246 R12: 00007ffd0e8dd92e
[  245.207395] R13: 00007ffd0e8dd92f R14: 0000000000000000 R15: 00007f66b56f9640


> 
> > The reason is that kvm_request_apicv_update must not be called with
> > srcu lock held vcpu->kvm->srcu (there is a warning about that
> > in kvm_request_apicv_update), but guest msr writes which come
> > from vcpu thread do hold it.
> >  
> > The other place where we disable AVIC on demand is svm_toggle_avic_for_irq_window.
> > And that code has a hack to drop this lock and take 
> > it back around the call to kvm_request_apicv_update.
> > This hack is safe as this code is called only from the vcpu thread.
> >  
> > Also for reference the reason for the fact that we need to
> > disable AVIC on the interrupt window request, or more correctly
> > why we still need to request interrupt windows with AVIC,
> > is that the local apic can act sadly as a pass-through device 
> > for legacy PIC, when one of its LINTn pins is configured in ExtINT mode.
> > In this mode when such pin is raised, the local apic asks the PIC for
> > the interrupt vector and then delivers it to the APIC
> > without touching the IRR/ISR.
> > 
> > The later means that if guest's interrupts are disabled,
> > such interrupt can't be queued via IRR to VAPIC
> > but instead the regular interrupt window has to be requested, 
> > but on AMD, the only way to request interrupt window
> > is to queue a VIRQ, and intercept its delivery,
> > a feature that is disabled when AVIC is active.
> >  
> > Finally for SynIC this srcu lock drop hack can be extended to this gross hack:
> > It seems to work though:
> > 
> > 
> > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > index bedd9b6cc26a..925b76e7b45e 100644
> > --- a/arch/x86/kvm/hyperv.c
> > +++ b/arch/x86/kvm/hyperv.c
> > @@ -85,7 +85,7 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic,
> >  }
> >  
> >  static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
> > -				int vector)
> > +				int vector, bool host)
> >  {
> >  	struct kvm_vcpu *vcpu = hv_synic_to_vcpu(synic);
> >  	struct kvm_hv *hv = to_kvm_hv(vcpu->kvm);
> > @@ -109,6 +109,9 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
> >  
> >  	auto_eoi_new = bitmap_weight(synic->auto_eoi_bitmap, 256);
> >  
> > +	if (!host)
> > +		srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
> > +
> >  	/* Hyper-V SynIC auto EOI SINTs are not compatible with APICV */
> >  	if (!auto_eoi_old && auto_eoi_new) {
> >  		printk("Synic: inhibiting avic %d %d\n", auto_eoi_old, auto_eoi_new);
> > @@ -121,6 +124,10 @@ static void synic_update_vector(struct kvm_vcpu_hv_synic *synic,
> >  			kvm_request_apicv_update(vcpu->kvm, true,
> >  						 APICV_INHIBIT_REASON_HYPERV);
> >  	}
> > +
> > +	if (!host)
> > +		vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
> > +
> >  }
> >  
> >  static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
> > @@ -149,9 +156,9 @@ static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
> >  
> >  	atomic64_set(&synic->sint[sint], data);
> >  
> > -	synic_update_vector(synic, old_vector);
> > +	synic_update_vector(synic, old_vector, host);
> >  
> > -	synic_update_vector(synic, vector);
> > +	synic_update_vector(synic, vector, host);
> >  
> >  	/* Load SynIC vectors into EOI exit bitmap */
> >  	kvm_make_request(KVM_REQ_SCAN_IOAPIC, hv_synic_to_vcpu(synic));
> > 
> > 
> > Assuming that we don't want this gross hack,  
> 
> Is it dangerous or just ugly?

It *should* work, but as it is always with locking,
if something changes, the assumption that MSR write
is called with SRCU held only on guest initiated writes
might be not true anymore.

Honestly I don't like the workaround that drops the lock in
svm_toggle_avic_for_irq_window either for the same reason.

> 
> > I wonder if we can avoid full blown memslot 
> > update when we disable avic, but rather have some 
> > smaller hack like only manually patching its
> > NPT mapping to have RW permissions instead 
> > of reserved bits which we use for MMIO. 
> > 
> > The AVIC spec says that NPT is only used to check that
> > guest has RW permission to the page, 
> > while the HVA in the NPT entry itself is ignored.
> 
> Assuming kvm_request_apicv_update() is called very rarely, I'd rather
> kicked all vCPUs out (similar to KVM_REQ_MCLOCK_INPROGRESS) and
> schedule_work() to make memslot update happen ourside of sRCU lock.

Adding Suravee Suthikulpanit to CC.


I tested it again and indeed I only see a burst of kvm_request_apicv_update
which ends when Windows enables IO apic
(this is a result of svm_toggle_avic_for_irq_window)


The AVIC disable due to SynIC autoeoi also happens I think
once per VCPU (I didn't verify this) and that is it.

So yes I do vote to make APICV update done in safer manner
as you suggest.


BTW I forgot about another reason that disables AVIC
The 'kvm-pit.lost_tick_policy=discard' has to be set,
since otherwise the in-kernel PIT reinject code relies
on EOI interception and thus disables AVIC as well.


Best regards,
	Maxim Levitsky

> 
> > Best regards,
> > 	Maxim Levitsky
> > 
> > 
> > 
> > 
> >