* [PATCH v3 1/4] KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC
2021-06-09 15:09 [PATCH v3 0/4] KVM: x86: hyper-v: Conditionally allow SynIC with APICv/AVIC Vitaly Kuznetsov
@ 2021-06-09 15:09 ` Vitaly Kuznetsov
2021-06-09 15:09 ` [PATCH v3 2/4] KVM: x86: Drop vendor specific functions for APICv/AVIC enablement Vitaly Kuznetsov
` (4 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Vitaly Kuznetsov @ 2021-06-09 15:09 UTC (permalink / raw)
To: kvm, Paolo Bonzini
Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Maxim Levitsky,
linux-kernel
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 9c7ced0e3171..bb4f827cbacd 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1423,6 +1423,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 0e62e6a2438c..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 */
-bool avic;
-module_param(avic, bool, 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 e088086f3de6..200aabe6de23 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -185,6 +185,13 @@ module_param(vls, int, 0444);
static int vgif = true;
module_param(vgif, 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);
@@ -1009,14 +1016,12 @@ 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");
+ enable_apicv = avic = avic && npt_enabled && boot_cpu_has(X86_FEATURE_AVIC);
- amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
- }
+ 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 2908c6ab5bb4..8959b85319bb 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -480,8 +480,6 @@ extern struct kvm_x86_nested_ops svm_nested_ops;
#define VMCB_AVIC_APIC_BAR_MASK 0xFFFFFFFFFF000ULL
-extern bool 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 50b42d7a8a11..4eb369e66372 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 b594275d49b5..7139725303da 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v3 2/4] KVM: x86: Drop vendor specific functions for APICv/AVIC enablement
2021-06-09 15:09 [PATCH v3 0/4] KVM: x86: hyper-v: Conditionally allow SynIC with APICv/AVIC Vitaly Kuznetsov
2021-06-09 15:09 ` [PATCH v3 1/4] KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC Vitaly Kuznetsov
@ 2021-06-09 15:09 ` Vitaly Kuznetsov
2021-06-09 15:09 ` [PATCH v3 3/4] KVM: x86: Check for pending interrupts when APICv is getting disabled Vitaly Kuznetsov
` (3 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Vitaly Kuznetsov @ 2021-06-09 15:09 UTC (permalink / raw)
To: kvm, Paolo Bonzini
Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Maxim Levitsky,
linux-kernel
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.
Reviewed-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
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 bb4f827cbacd..ed4e2aae13d1 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1663,7 +1663,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 200aabe6de23..2e766a608faf 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 4eb369e66372..e354433334cd 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6999,7 +6999,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 7139725303da..0f461f111a0b 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)
{
@@ -10741,6 +10740,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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v3 3/4] KVM: x86: Check for pending interrupts when APICv is getting disabled
2021-06-09 15:09 [PATCH v3 0/4] KVM: x86: hyper-v: Conditionally allow SynIC with APICv/AVIC Vitaly Kuznetsov
2021-06-09 15:09 ` [PATCH v3 1/4] KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC Vitaly Kuznetsov
2021-06-09 15:09 ` [PATCH v3 2/4] KVM: x86: Drop vendor specific functions for APICv/AVIC enablement Vitaly Kuznetsov
@ 2021-06-09 15:09 ` Vitaly Kuznetsov
2021-06-09 15:09 ` [PATCH v3 4/4] KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in use Vitaly Kuznetsov
` (2 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Vitaly Kuznetsov @ 2021-06-09 15:09 UTC (permalink / raw)
To: kvm, Paolo Bonzini
Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Maxim Levitsky,
linux-kernel
When APICv is active, interrupt injection doesn't raise KVM_REQ_EVENT
request (see __apic_accept_irq()) as the required work is done by hardware.
In case KVM_REQ_APICV_UPDATE collides with such injection, the interrupt
may never get delivered.
Currently, the described situation is hardly possible: all
kvm_request_apicv_update() calls normally happen upon VM creation when
no interrupts are pending. We are, however, going to move unconditional
kvm_request_apicv_update() call from kvm_hv_activate_synic() to
synic_update_vector() and without this fix 'hyperv_connections' test from
kvm-unit-tests gets stuck on IPI delivery attempt right after configuring
a SynIC route which triggers APICv disablement.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
arch/x86/kvm/x86.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0f461f111a0b..605dda19642b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8979,6 +8979,15 @@ void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu)
vcpu->arch.apicv_active = kvm_apicv_activated(vcpu->kvm);
kvm_apic_update_apicv(vcpu);
static_call(kvm_x86_refresh_apicv_exec_ctrl)(vcpu);
+
+ /*
+ * When APICv gets disabled, we may still have injected interrupts
+ * pending. At the same time, KVM_REQ_EVENT may not be set as APICv was
+ * still active when the interrupt got accepted. Make sure
+ * inject_pending_event() is called to check for that.
+ */
+ if (!vcpu->arch.apicv_active)
+ kvm_make_request(KVM_REQ_EVENT, vcpu);
}
EXPORT_SYMBOL_GPL(kvm_vcpu_update_apicv);
--
2.31.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v3 4/4] KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in use
2021-06-09 15:09 [PATCH v3 0/4] KVM: x86: hyper-v: Conditionally allow SynIC with APICv/AVIC Vitaly Kuznetsov
` (2 preceding siblings ...)
2021-06-09 15:09 ` [PATCH v3 3/4] KVM: x86: Check for pending interrupts when APICv is getting disabled Vitaly Kuznetsov
@ 2021-06-09 15:09 ` Vitaly Kuznetsov
2021-06-10 13:17 ` [PATCH v3 0/4] KVM: x86: hyper-v: Conditionally allow SynIC with APICv/AVIC Paolo Bonzini
2021-06-11 18:50 ` Maxim Levitsky
5 siblings, 0 replies; 11+ messages in thread
From: Vitaly Kuznetsov @ 2021-06-09 15:09 UTC (permalink / raw)
To: kvm, Paolo Bonzini
Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Maxim Levitsky,
linux-kernel
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 | 7 +++++
arch/x86/kvm/hyperv.c | 51 +++++++++++++++++++++++++++++----
2 files changed, 52 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index ed4e2aae13d1..01ee8f29d6e3 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -91,6 +91,8 @@
#define KVM_REQ_MSR_FILTER_CHANGED KVM_ARCH_REQ(29)
#define KVM_REQ_UPDATE_CPU_DIRTY_LOGGING \
KVM_ARCH_REQ_FLAGS(30, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
+#define KVM_REQ_APICV_INPROGRESS \
+ KVM_ARCH_REQ_FLAGS(31, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
#define CR0_RESERVED_BITS \
(~(unsigned long)(X86_CR0_PE | X86_CR0_MP | X86_CR0_EM | X86_CR0_TS \
@@ -936,8 +938,13 @@ 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;
+
+ struct work_struct apic_update_work;
};
struct msr_bitmap_range {
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index f00830e5202f..08cfc9ec1ef2 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -84,9 +84,32 @@ static bool synic_has_vector_auto_eoi(struct kvm_vcpu_hv_synic *synic,
return false;
}
+static void hv_apicv_update_work(struct work_struct *work)
+{
+ struct kvm_hv *hv = container_of(work, struct kvm_hv,
+ apic_update_work);
+ struct kvm_arch *ka = container_of(hv, struct kvm_arch,
+ hyperv);
+ struct kvm *kvm = container_of(ka, struct kvm, arch);
+ struct kvm_vcpu *vcpu;
+ int i;
+
+ mutex_lock(&kvm->lock);
+ kvm_request_apicv_update(kvm, atomic_read(&hv->synic_auto_eoi_used) == 0,
+ APICV_INHIBIT_REASON_HYPERV);
+ kvm_for_each_vcpu(i, vcpu, kvm)
+ kvm_clear_request(KVM_REQ_APICV_INPROGRESS, vcpu);
+ mutex_unlock(&kvm->lock);
+}
+
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;
+ bool apicv_update_needed = false;
+
if (vector < HV_SYNIC_FIRST_VALID_VECTOR)
return;
@@ -95,10 +118,28 @@ 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)
+ apicv_update_needed = true;
+ } else if (!auto_eoi_new && auto_eoi_old) {
+ if (atomic_dec_return(&hv->synic_auto_eoi_used) == 0)
+ apicv_update_needed = true;
+ }
+
+ if (apicv_update_needed) {
+ kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_APICV_INPROGRESS);
+ schedule_work(&hv->apic_update_work);
+ }
}
static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
@@ -931,12 +972,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;
@@ -2041,6 +2076,7 @@ void kvm_hv_init_vm(struct kvm *kvm)
mutex_init(&hv->hv_lock);
idr_init(&hv->conn_to_evt);
+ INIT_WORK(&hv->apic_update_work, hv_apicv_update_work);
}
void kvm_hv_destroy_vm(struct kvm *kvm)
@@ -2052,6 +2088,7 @@ void kvm_hv_destroy_vm(struct kvm *kvm)
idr_for_each_entry(&hv->conn_to_evt, eventfd, i)
eventfd_ctx_put(eventfd);
idr_destroy(&hv->conn_to_evt);
+ cancel_work_sync(&hv->apic_update_work);
}
static int kvm_hv_eventfd_assign(struct kvm *kvm, u32 conn_id, int fd)
@@ -2206,6 +2243,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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3 0/4] KVM: x86: hyper-v: Conditionally allow SynIC with APICv/AVIC
2021-06-09 15:09 [PATCH v3 0/4] KVM: x86: hyper-v: Conditionally allow SynIC with APICv/AVIC Vitaly Kuznetsov
` (3 preceding siblings ...)
2021-06-09 15:09 ` [PATCH v3 4/4] KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in use Vitaly Kuznetsov
@ 2021-06-10 13:17 ` Paolo Bonzini
2021-06-11 18:50 ` Maxim Levitsky
5 siblings, 0 replies; 11+ messages in thread
From: Paolo Bonzini @ 2021-06-10 13:17 UTC (permalink / raw)
To: Vitaly Kuznetsov, kvm
Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Maxim Levitsky,
linux-kernel
On 09/06/21 17:09, Vitaly Kuznetsov wrote:
> Changes since v2:
> - First two patches got merged, rebase.
> - Use 'enable_apicv = avic = ...' in PATCH1 [Paolo]
> - Collect R-b tags for PATCH2 [Sean, Max]
> - Use hv_apicv_update_work() to get out of SRCU lock [Max]
> - "KVM: x86: Check for pending interrupts when APICv is getting disabled"
> added.
>
> 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 series can be tested with the followin hack:
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 9a48f138832d..65a9974f80d9 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -147,6 +147,13 @@ void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
> vcpu->arch.ia32_misc_enable_msr &
> MSR_IA32_MISC_ENABLE_MWAIT);
> }
> +
> + /* Dirty hack: force HV_DEPRECATING_AEOI_RECOMMENDED. Not to be merged! */
> + best = kvm_find_cpuid_entry(vcpu, HYPERV_CPUID_ENLIGHTMENT_INFO, 0);
> + if (best) {
> + best->eax &= ~HV_X64_APIC_ACCESS_RECOMMENDED;
> + best->eax |= HV_DEPRECATING_AEOI_RECOMMENDED;
> + }
> }
> EXPORT_SYMBOL_GPL(kvm_update_cpuid_runtime);
>
> Vitaly Kuznetsov (4):
> KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC
> KVM: x86: Drop vendor specific functions for APICv/AVIC enablement
> KVM: x86: Check for pending interrupts when APICv is getting disabled
> KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in
> use
>
> arch/x86/include/asm/kvm_host.h | 9 +++++-
> arch/x86/kvm/hyperv.c | 51 +++++++++++++++++++++++++++++----
> arch/x86/kvm/svm/avic.c | 14 ++++-----
> arch/x86/kvm/svm/svm.c | 22 ++++++++------
> arch/x86/kvm/svm/svm.h | 2 --
> arch/x86/kvm/vmx/capabilities.h | 1 -
> arch/x86/kvm/vmx/vmx.c | 2 --
> arch/x86/kvm/x86.c | 18 ++++++++++--
> 8 files changed, 86 insertions(+), 33 deletions(-)
>
Queued, thanks.
Paolo
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3 0/4] KVM: x86: hyper-v: Conditionally allow SynIC with APICv/AVIC
2021-06-09 15:09 [PATCH v3 0/4] KVM: x86: hyper-v: Conditionally allow SynIC with APICv/AVIC Vitaly Kuznetsov
` (4 preceding siblings ...)
2021-06-10 13:17 ` [PATCH v3 0/4] KVM: x86: hyper-v: Conditionally allow SynIC with APICv/AVIC Paolo Bonzini
@ 2021-06-11 18:50 ` Maxim Levitsky
2021-06-14 7:40 ` Vitaly Kuznetsov
5 siblings, 1 reply; 11+ messages in thread
From: Maxim Levitsky @ 2021-06-11 18:50 UTC (permalink / raw)
To: Vitaly Kuznetsov, kvm, Paolo Bonzini
Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, linux-kernel
On Wed, 2021-06-09 at 17:09 +0200, Vitaly Kuznetsov wrote:
> Changes since v2:
> - First two patches got merged, rebase.
> - Use 'enable_apicv = avic = ...' in PATCH1 [Paolo]
> - Collect R-b tags for PATCH2 [Sean, Max]
> - Use hv_apicv_update_work() to get out of SRCU lock [Max]
> - "KVM: x86: Check for pending interrupts when APICv is getting disabled"
> added.
>
> 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 series can be tested with the followin hack:
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 9a48f138832d..65a9974f80d9 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -147,6 +147,13 @@ void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
> vcpu->arch.ia32_misc_enable_msr &
> MSR_IA32_MISC_ENABLE_MWAIT);
> }
> +
> + /* Dirty hack: force HV_DEPRECATING_AEOI_RECOMMENDED. Not to be merged! */
> + best = kvm_find_cpuid_entry(vcpu, HYPERV_CPUID_ENLIGHTMENT_INFO, 0);
> + if (best) {
> + best->eax &= ~HV_X64_APIC_ACCESS_RECOMMENDED;
> + best->eax |= HV_DEPRECATING_AEOI_RECOMMENDED;
> + }
> }
> EXPORT_SYMBOL_GPL(kvm_update_cpuid_runtime);
>
> Vitaly Kuznetsov (4):
> KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC
> KVM: x86: Drop vendor specific functions for APICv/AVIC enablement
> KVM: x86: Check for pending interrupts when APICv is getting disabled
> KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in
> use
>
> arch/x86/include/asm/kvm_host.h | 9 +++++-
> arch/x86/kvm/hyperv.c | 51 +++++++++++++++++++++++++++++----
> arch/x86/kvm/svm/avic.c | 14 ++++-----
> arch/x86/kvm/svm/svm.c | 22 ++++++++------
> arch/x86/kvm/svm/svm.h | 2 --
> arch/x86/kvm/vmx/capabilities.h | 1 -
> arch/x86/kvm/vmx/vmx.c | 2 --
> arch/x86/kvm/x86.c | 18 ++++++++++--
> 8 files changed, 86 insertions(+), 33 deletions(-)
>
Hi!
I hate to say it, but at least one of my VMs doesn't boot amymore
with avic=1, after the recent updates. I'll bisect this soon,
but this is likely related to this series.
I will also review this series very soon.
When the VM fails, it hangs on the OVMF screen and I see this
in qemu logs:
KVM: injection failed, MSI lost (Operation not permitted)
KVM: injection failed, MSI lost (Operation not permitted)
KVM: injection failed, MSI lost (Operation not permitted)
KVM: injection failed, MSI lost (Operation not permitted)
KVM: injection failed, MSI lost (Operation not permitted)
KVM: injection failed, MSI lost (Operation not permitted)
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3 0/4] KVM: x86: hyper-v: Conditionally allow SynIC with APICv/AVIC
2021-06-11 18:50 ` Maxim Levitsky
@ 2021-06-14 7:40 ` Vitaly Kuznetsov
2021-06-14 9:51 ` Maxim Levitsky
0 siblings, 1 reply; 11+ messages in thread
From: Vitaly Kuznetsov @ 2021-06-14 7:40 UTC (permalink / raw)
To: Maxim Levitsky
Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, linux-kernel, kvm,
Paolo Bonzini
Maxim Levitsky <mlevitsk@redhat.com> writes:
> On Wed, 2021-06-09 at 17:09 +0200, Vitaly Kuznetsov wrote:
>> Changes since v2:
>> - First two patches got merged, rebase.
>> - Use 'enable_apicv = avic = ...' in PATCH1 [Paolo]
>> - Collect R-b tags for PATCH2 [Sean, Max]
>> - Use hv_apicv_update_work() to get out of SRCU lock [Max]
>> - "KVM: x86: Check for pending interrupts when APICv is getting disabled"
>> added.
>>
>> 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 series can be tested with the followin hack:
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 9a48f138832d..65a9974f80d9 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -147,6 +147,13 @@ void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
>> vcpu->arch.ia32_misc_enable_msr &
>> MSR_IA32_MISC_ENABLE_MWAIT);
>> }
>> +
>> + /* Dirty hack: force HV_DEPRECATING_AEOI_RECOMMENDED. Not to be merged! */
>> + best = kvm_find_cpuid_entry(vcpu, HYPERV_CPUID_ENLIGHTMENT_INFO, 0);
>> + if (best) {
>> + best->eax &= ~HV_X64_APIC_ACCESS_RECOMMENDED;
>> + best->eax |= HV_DEPRECATING_AEOI_RECOMMENDED;
>> + }
>> }
>> EXPORT_SYMBOL_GPL(kvm_update_cpuid_runtime);
>>
>> Vitaly Kuznetsov (4):
>> KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC
>> KVM: x86: Drop vendor specific functions for APICv/AVIC enablement
>> KVM: x86: Check for pending interrupts when APICv is getting disabled
>> KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in
>> use
>>
>> arch/x86/include/asm/kvm_host.h | 9 +++++-
>> arch/x86/kvm/hyperv.c | 51 +++++++++++++++++++++++++++++----
>> arch/x86/kvm/svm/avic.c | 14 ++++-----
>> arch/x86/kvm/svm/svm.c | 22 ++++++++------
>> arch/x86/kvm/svm/svm.h | 2 --
>> arch/x86/kvm/vmx/capabilities.h | 1 -
>> arch/x86/kvm/vmx/vmx.c | 2 --
>> arch/x86/kvm/x86.c | 18 ++++++++++--
>> 8 files changed, 86 insertions(+), 33 deletions(-)
>>
>
> Hi!
>
> I hate to say it, but at least one of my VMs doesn't boot amymore
> with avic=1, after the recent updates. I'll bisect this soon,
> but this is likely related to this series.
>
> I will also review this series very soon.
>
> When the VM fails, it hangs on the OVMF screen and I see this
> in qemu logs:
>
> KVM: injection failed, MSI lost (Operation not permitted)
> KVM: injection failed, MSI lost (Operation not permitted)
> KVM: injection failed, MSI lost (Operation not permitted)
> KVM: injection failed, MSI lost (Operation not permitted)
> KVM: injection failed, MSI lost (Operation not permitted)
> KVM: injection failed, MSI lost (Operation not permitted)
>
-EPERM?? Interesting... strace(1) may come handy...
$ git grep EPERM kvm/queue arch/x86/kvm/
kvm/queue:arch/x86/kvm/x86.c: ret = -KVM_EPERM;
(just this one)
kvm_emulate_hypercall():
...
b3646477d458f arch/x86/kvm/x86.c (Jason Baron 2021-01-14 22:27:56 -0500 8433) if (static_call(kvm_x86_get_cpl)(vcpu) != 0) {
07708c4af1346 arch/x86/kvm/x86.c (Jan Kiszka 2009-08-03 18:43:28 +0200 8434) ret = -KVM_EPERM;
696ca779a928d arch/x86/kvm/x86.c (Radim Krčmář 2018-05-24 17:50:56 +0200 8435) goto out;
07708c4af1346 arch/x86/kvm/x86.c (Jan Kiszka 2009-08-03 18:43:28 +0200 8436) }
...
Doesn't seem we have any updates here, curious what your bisection will
point us to.
--
Vitaly
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3 0/4] KVM: x86: hyper-v: Conditionally allow SynIC with APICv/AVIC
2021-06-14 7:40 ` Vitaly Kuznetsov
@ 2021-06-14 9:51 ` Maxim Levitsky
2021-06-14 13:08 ` Paolo Bonzini
0 siblings, 1 reply; 11+ messages in thread
From: Maxim Levitsky @ 2021-06-14 9:51 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, linux-kernel, kvm,
Paolo Bonzini
On Mon, 2021-06-14 at 09:40 +0200, Vitaly Kuznetsov wrote:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
>
> > On Wed, 2021-06-09 at 17:09 +0200, Vitaly Kuznetsov wrote:
> > > Changes since v2:
> > > - First two patches got merged, rebase.
> > > - Use 'enable_apicv = avic = ...' in PATCH1 [Paolo]
> > > - Collect R-b tags for PATCH2 [Sean, Max]
> > > - Use hv_apicv_update_work() to get out of SRCU lock [Max]
> > > - "KVM: x86: Check for pending interrupts when APICv is getting disabled"
> > > added.
> > >
> > > 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 series can be tested with the followin hack:
> > >
> > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > index 9a48f138832d..65a9974f80d9 100644
> > > --- a/arch/x86/kvm/cpuid.c
> > > +++ b/arch/x86/kvm/cpuid.c
> > > @@ -147,6 +147,13 @@ void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
> > > vcpu->arch.ia32_misc_enable_msr &
> > > MSR_IA32_MISC_ENABLE_MWAIT);
> > > }
> > > +
> > > + /* Dirty hack: force HV_DEPRECATING_AEOI_RECOMMENDED. Not to be merged! */
> > > + best = kvm_find_cpuid_entry(vcpu, HYPERV_CPUID_ENLIGHTMENT_INFO, 0);
> > > + if (best) {
> > > + best->eax &= ~HV_X64_APIC_ACCESS_RECOMMENDED;
> > > + best->eax |= HV_DEPRECATING_AEOI_RECOMMENDED;
> > > + }
> > > }
> > > EXPORT_SYMBOL_GPL(kvm_update_cpuid_runtime);
> > >
> > > Vitaly Kuznetsov (4):
> > > KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC
> > > KVM: x86: Drop vendor specific functions for APICv/AVIC enablement
> > > KVM: x86: Check for pending interrupts when APICv is getting disabled
> > > KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in
> > > use
> > >
> > > arch/x86/include/asm/kvm_host.h | 9 +++++-
> > > arch/x86/kvm/hyperv.c | 51 +++++++++++++++++++++++++++++----
> > > arch/x86/kvm/svm/avic.c | 14 ++++-----
> > > arch/x86/kvm/svm/svm.c | 22 ++++++++------
> > > arch/x86/kvm/svm/svm.h | 2 --
> > > arch/x86/kvm/vmx/capabilities.h | 1 -
> > > arch/x86/kvm/vmx/vmx.c | 2 --
> > > arch/x86/kvm/x86.c | 18 ++++++++++--
> > > 8 files changed, 86 insertions(+), 33 deletions(-)
> > >
> >
> > Hi!
> >
> > I hate to say it, but at least one of my VMs doesn't boot amymore
> > with avic=1, after the recent updates. I'll bisect this soon,
> > but this is likely related to this series.
> >
> > I will also review this series very soon.
> >
> > When the VM fails, it hangs on the OVMF screen and I see this
> > in qemu logs:
> >
> > KVM: injection failed, MSI lost (Operation not permitted)
> > KVM: injection failed, MSI lost (Operation not permitted)
> > KVM: injection failed, MSI lost (Operation not permitted)
> > KVM: injection failed, MSI lost (Operation not permitted)
> > KVM: injection failed, MSI lost (Operation not permitted)
> > KVM: injection failed, MSI lost (Operation not permitted)
> >
>
> -EPERM?? Interesting... strace(1) may come handy...
Hi Vitaly!
I spent all yesterday debugging this and I found out what is going on:
(spoiler alert: hacks are bad)
The call to kvm_request_apicv_update was moved to a delayed work which is fine at first glance
but turns out that we both don't notice that kvm doesn't allow to update the guest
memory map from non vcpu thread which is what kvm_request_apicv_update does
on AVIC.
The memslot update is to switch between regular r/w mapped dummy page
which is not really used but doesn't hurt to be there, and between paging entry with
reserved bits, used for MMIO, which AVIC sadly needs because it is written in the
spec that AVIC's MMIO despite being redirected to the avic_vapic_bar, still needs a valid
R/W mapping in the NPT, whose physical address is ignored.
So, in avic_update_access_page we have this nice hack:
if ((kvm->arch.apic_access_page_done == activate) ||
(kvm->mm != current->mm))
goto out;
So instead of crashing this function just does nothing.
So AVIC MMIO is still mapped R/W to a dummy page, but the AVIC itself
is disabled on all vCPUs by kvm_request_apicv_update (with
KVM_REQ_APICV_UPDATE request)
So now all guest APIC writes just disappear to that dummy
page, and we have a guest that seems to run but can't really
continue.
The -EPERM in the error message I reported, is just -1, returned by
KVM_SIGNAL_MSI which is likely result of gross missmatch between
state of the KVM's APIC registers and that dummy page which contains
whatever the guest wrote there and what the guest thinks
the APIC registers are.
I am curently thinking on how to do the whole thing with
KVM's requests, I'll try to prepare a patch today.
Best regards,
Maxim Levitsky
>
> $ git grep EPERM kvm/queue arch/x86/kvm/
> kvm/queue:arch/x86/kvm/x86.c: ret = -KVM_EPERM;
> (just this one)
>
> kvm_emulate_hypercall():
> ...
> b3646477d458f arch/x86/kvm/x86.c (Jason Baron 2021-01-14 22:27:56 -0500 8433) if (static_call(kvm_x86_get_cpl)(vcpu) != 0) {
> 07708c4af1346 arch/x86/kvm/x86.c (Jan Kiszka 2009-08-03 18:43:28 +0200 8434) ret = -KVM_EPERM;
> 696ca779a928d arch/x86/kvm/x86.c (Radim Krčmář 2018-05-24 17:50:56 +0200 8435) goto out;
> 07708c4af1346 arch/x86/kvm/x86.c (Jan Kiszka 2009-08-03 18:43:28 +0200 8436) }
> ...
>
> Doesn't seem we have any updates here, curious what your bisection will
> point us to.
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3 0/4] KVM: x86: hyper-v: Conditionally allow SynIC with APICv/AVIC
2021-06-14 9:51 ` Maxim Levitsky
@ 2021-06-14 13:08 ` Paolo Bonzini
2021-06-14 18:21 ` Maxim Levitsky
0 siblings, 1 reply; 11+ messages in thread
From: Paolo Bonzini @ 2021-06-14 13:08 UTC (permalink / raw)
To: Maxim Levitsky, Vitaly Kuznetsov
Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, linux-kernel, kvm
On 14/06/21 11:51, Maxim Levitsky wrote:
> On Mon, 2021-06-14 at 09:40 +0200, Vitaly Kuznetsov wrote:
>> Maxim Levitsky <mlevitsk@redhat.com> writes:
>>
>>> On Wed, 2021-06-09 at 17:09 +0200, Vitaly Kuznetsov wrote:
>>>> Changes since v2:
>>>> - First two patches got merged, rebase.
>>>> - Use 'enable_apicv = avic = ...' in PATCH1 [Paolo]
>>>> - Collect R-b tags for PATCH2 [Sean, Max]
>>>> - Use hv_apicv_update_work() to get out of SRCU lock [Max]
>>>> - "KVM: x86: Check for pending interrupts when APICv is getting disabled"
>>>> added.
>>>>
>>>> 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 series can be tested with the followin hack:
>>>>
>>>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>>>> index 9a48f138832d..65a9974f80d9 100644
>>>> --- a/arch/x86/kvm/cpuid.c
>>>> +++ b/arch/x86/kvm/cpuid.c
>>>> @@ -147,6 +147,13 @@ void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
>>>> vcpu->arch.ia32_misc_enable_msr &
>>>> MSR_IA32_MISC_ENABLE_MWAIT);
>>>> }
>>>> +
>>>> + /* Dirty hack: force HV_DEPRECATING_AEOI_RECOMMENDED. Not to be merged! */
>>>> + best = kvm_find_cpuid_entry(vcpu, HYPERV_CPUID_ENLIGHTMENT_INFO, 0);
>>>> + if (best) {
>>>> + best->eax &= ~HV_X64_APIC_ACCESS_RECOMMENDED;
>>>> + best->eax |= HV_DEPRECATING_AEOI_RECOMMENDED;
>>>> + }
>>>> }
>>>> EXPORT_SYMBOL_GPL(kvm_update_cpuid_runtime);
>>>>
>>>> Vitaly Kuznetsov (4):
>>>> KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC
>>>> KVM: x86: Drop vendor specific functions for APICv/AVIC enablement
>>>> KVM: x86: Check for pending interrupts when APICv is getting disabled
>>>> KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in
>>>> use
>>>>
>>>> arch/x86/include/asm/kvm_host.h | 9 +++++-
>>>> arch/x86/kvm/hyperv.c | 51 +++++++++++++++++++++++++++++----
>>>> arch/x86/kvm/svm/avic.c | 14 ++++-----
>>>> arch/x86/kvm/svm/svm.c | 22 ++++++++------
>>>> arch/x86/kvm/svm/svm.h | 2 --
>>>> arch/x86/kvm/vmx/capabilities.h | 1 -
>>>> arch/x86/kvm/vmx/vmx.c | 2 --
>>>> arch/x86/kvm/x86.c | 18 ++++++++++--
>>>> 8 files changed, 86 insertions(+), 33 deletions(-)
>>>>
>>>
>>> Hi!
>>>
>>> I hate to say it, but at least one of my VMs doesn't boot amymore
>>> with avic=1, after the recent updates. I'll bisect this soon,
>>> but this is likely related to this series.
>>>
>>> I will also review this series very soon.
>>>
>>> When the VM fails, it hangs on the OVMF screen and I see this
>>> in qemu logs:
>>>
>>> KVM: injection failed, MSI lost (Operation not permitted)
>>> KVM: injection failed, MSI lost (Operation not permitted)
>>> KVM: injection failed, MSI lost (Operation not permitted)
>>> KVM: injection failed, MSI lost (Operation not permitted)
>>> KVM: injection failed, MSI lost (Operation not permitted)
>>> KVM: injection failed, MSI lost (Operation not permitted)
>>>
>>
>> -EPERM?? Interesting... strace(1) may come handy...
>
>
> Hi Vitaly!
>
> I spent all yesterday debugging this and I found out what is going on:
> (spoiler alert: hacks are bad)
>
> The call to kvm_request_apicv_update was moved to a delayed work which is fine at first glance
> but turns out that we both don't notice that kvm doesn't allow to update the guest
> memory map from non vcpu thread which is what kvm_request_apicv_update does
> on AVIC.
>
> The memslot update is to switch between regular r/w mapped dummy page
> which is not really used but doesn't hurt to be there, and between paging entry with
> reserved bits, used for MMIO, which AVIC sadly needs because it is written in the
> spec that AVIC's MMIO despite being redirected to the avic_vapic_bar, still needs a valid
> R/W mapping in the NPT, whose physical address is ignored.
>
> So, in avic_update_access_page we have this nice hack:
>
> if ((kvm->arch.apic_access_page_done == activate) ||
> (kvm->mm != current->mm))
> goto out;
>
> So instead of crashing this function just does nothing.
> So AVIC MMIO is still mapped R/W to a dummy page, but the AVIC itself
> is disabled on all vCPUs by kvm_request_apicv_update (with
> KVM_REQ_APICV_UPDATE request)
>
> So now all guest APIC writes just disappear to that dummy
> page, and we have a guest that seems to run but can't really
> continue.
>
> The -EPERM in the error message I reported, is just -1, returned by
> KVM_SIGNAL_MSI which is likely result of gross missmatch between
> state of the KVM's APIC registers and that dummy page which contains
> whatever the guest wrote there and what the guest thinks
> the APIC registers are.
>
> I am curently thinking on how to do the whole thing with
> KVM's requests, I'll try to prepare a patch today.
I'll drop the last two patches in the series.
Paolo
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3 0/4] KVM: x86: hyper-v: Conditionally allow SynIC with APICv/AVIC
2021-06-14 13:08 ` Paolo Bonzini
@ 2021-06-14 18:21 ` Maxim Levitsky
0 siblings, 0 replies; 11+ messages in thread
From: Maxim Levitsky @ 2021-06-14 18:21 UTC (permalink / raw)
To: Paolo Bonzini, Vitaly Kuznetsov
Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, linux-kernel, kvm
On Mon, 2021-06-14 at 15:08 +0200, Paolo Bonzini wrote:
> On 14/06/21 11:51, Maxim Levitsky wrote:
> > On Mon, 2021-06-14 at 09:40 +0200, Vitaly Kuznetsov wrote:
> > > Maxim Levitsky <mlevitsk@redhat.com> writes:
> > >
> > > > On Wed, 2021-06-09 at 17:09 +0200, Vitaly Kuznetsov wrote:
> > > > > Changes since v2:
> > > > > - First two patches got merged, rebase.
> > > > > - Use 'enable_apicv = avic = ...' in PATCH1 [Paolo]
> > > > > - Collect R-b tags for PATCH2 [Sean, Max]
> > > > > - Use hv_apicv_update_work() to get out of SRCU lock [Max]
> > > > > - "KVM: x86: Check for pending interrupts when APICv is getting disabled"
> > > > > added.
> > > > >
> > > > > 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 series can be tested with the followin hack:
> > > > >
> > > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > > > index 9a48f138832d..65a9974f80d9 100644
> > > > > --- a/arch/x86/kvm/cpuid.c
> > > > > +++ b/arch/x86/kvm/cpuid.c
> > > > > @@ -147,6 +147,13 @@ void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
> > > > > vcpu->arch.ia32_misc_enable_msr &
> > > > > MSR_IA32_MISC_ENABLE_MWAIT);
> > > > > }
> > > > > +
> > > > > + /* Dirty hack: force HV_DEPRECATING_AEOI_RECOMMENDED. Not to be merged! */
> > > > > + best = kvm_find_cpuid_entry(vcpu, HYPERV_CPUID_ENLIGHTMENT_INFO, 0);
> > > > > + if (best) {
> > > > > + best->eax &= ~HV_X64_APIC_ACCESS_RECOMMENDED;
> > > > > + best->eax |= HV_DEPRECATING_AEOI_RECOMMENDED;
> > > > > + }
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(kvm_update_cpuid_runtime);
> > > > >
> > > > > Vitaly Kuznetsov (4):
> > > > > KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC
> > > > > KVM: x86: Drop vendor specific functions for APICv/AVIC enablement
> > > > > KVM: x86: Check for pending interrupts when APICv is getting disabled
> > > > > KVM: x86: hyper-v: Deactivate APICv only when AutoEOI feature is in
> > > > > use
> > > > >
> > > > > arch/x86/include/asm/kvm_host.h | 9 +++++-
> > > > > arch/x86/kvm/hyperv.c | 51 +++++++++++++++++++++++++++++----
> > > > > arch/x86/kvm/svm/avic.c | 14 ++++-----
> > > > > arch/x86/kvm/svm/svm.c | 22 ++++++++------
> > > > > arch/x86/kvm/svm/svm.h | 2 --
> > > > > arch/x86/kvm/vmx/capabilities.h | 1 -
> > > > > arch/x86/kvm/vmx/vmx.c | 2 --
> > > > > arch/x86/kvm/x86.c | 18 ++++++++++--
> > > > > 8 files changed, 86 insertions(+), 33 deletions(-)
> > > > >
> > > >
> > > > Hi!
> > > >
> > > > I hate to say it, but at least one of my VMs doesn't boot amymore
> > > > with avic=1, after the recent updates. I'll bisect this soon,
> > > > but this is likely related to this series.
> > > >
> > > > I will also review this series very soon.
> > > >
> > > > When the VM fails, it hangs on the OVMF screen and I see this
> > > > in qemu logs:
> > > >
> > > > KVM: injection failed, MSI lost (Operation not permitted)
> > > > KVM: injection failed, MSI lost (Operation not permitted)
> > > > KVM: injection failed, MSI lost (Operation not permitted)
> > > > KVM: injection failed, MSI lost (Operation not permitted)
> > > > KVM: injection failed, MSI lost (Operation not permitted)
> > > > KVM: injection failed, MSI lost (Operation not permitted)
> > > >
> > >
> > > -EPERM?? Interesting... strace(1) may come handy...
> >
> > Hi Vitaly!
> >
> > I spent all yesterday debugging this and I found out what is going on:
> > (spoiler alert: hacks are bad)
> >
> > The call to kvm_request_apicv_update was moved to a delayed work which is fine at first glance
> > but turns out that we both don't notice that kvm doesn't allow to update the guest
> > memory map from non vcpu thread which is what kvm_request_apicv_update does
> > on AVIC.
> >
> > The memslot update is to switch between regular r/w mapped dummy page
> > which is not really used but doesn't hurt to be there, and between paging entry with
> > reserved bits, used for MMIO, which AVIC sadly needs because it is written in the
> > spec that AVIC's MMIO despite being redirected to the avic_vapic_bar, still needs a valid
> > R/W mapping in the NPT, whose physical address is ignored.
> >
> > So, in avic_update_access_page we have this nice hack:
> >
> > if ((kvm->arch.apic_access_page_done == activate) ||
> > (kvm->mm != current->mm))
> > goto out;
> >
> > So instead of crashing this function just does nothing.
> > So AVIC MMIO is still mapped R/W to a dummy page, but the AVIC itself
> > is disabled on all vCPUs by kvm_request_apicv_update (with
> > KVM_REQ_APICV_UPDATE request)
> >
> > So now all guest APIC writes just disappear to that dummy
> > page, and we have a guest that seems to run but can't really
> > continue.
> >
> > The -EPERM in the error message I reported, is just -1, returned by
> > KVM_SIGNAL_MSI which is likely result of gross missmatch between
> > state of the KVM's APIC registers and that dummy page which contains
> > whatever the guest wrote there and what the guest thinks
> > the APIC registers are.
> >
> > I am curently thinking on how to do the whole thing with
> > KVM's requests, I'll try to prepare a patch today.
>
> I'll drop the last two patches in the series.
Actually only the patch 4 is problemetic, and patch 3 IMHO does fix an issue.
Best regards,
Maxim Levitsky
>
> Paolo
>
^ permalink raw reply [flat|nested] 11+ messages in thread