[PATCH v2 00/12] Introducing AMD x2APIC Virtualization (x2AVIC) support

[PATCH v2 00/12] Introducing AMD x2APIC Virtualization (x2AVIC) support

[Suravee Suthikulpanit]

Previously, with AVIC, guest needs to disable x2APIC capability and
can only run in APIC mode to activate hardware-accelerated interrupt
virtualization.  With x2AVIC, guest can run in x2APIC mode.
This feature is indicated by the CPUID Fn8000_000A EDX[14],
and it can be activated by setting bit 31 (enable AVIC) and
bit 30 (x2APIC mode) of VMCB offset 60h.

The mode of interrupt virtualization can dynamically change during runtime.
For example, when AVIC is enabled, the hypervisor currently keeps track of
the AVIC activation and set the VMCB bit 31 accordingly. With x2AVIC,
the guest OS can also switch between APIC and x2APIC modes during runtime.
The kvm_amd driver needs to also keep track and set the VMCB
bit 30 accordingly. 

Besides, for x2AVIC, kvm_amd driver needs to disable interception for the
x2APIC MSR range to allow AVIC hardware to virtualize register accesses.

Testing:
  * This series has been tested booting a Linux VM with x2APIC physical
    and logical modes upto 512 vCPUs.

Regards,
Suravee

Changes from v1
(https://lore.kernel.org/lkml/20220405230855.15376-1-suravee.suthikulpanit@amd.com/T/)
  * Rebase to v5.18
  * Patch 7/12: remove logic to check for x2AVIC feature.

Suravee Suthikulpanit (12):
  x86/cpufeatures: Introduce x2AVIC CPUID bit
  KVM: x86: lapic: Rename [GET/SET]_APIC_DEST_FIELD to
    [GET/SET]_XAPIC_DEST_FIELD
  KVM: SVM: Detect X2APIC virtualization (x2AVIC) support
  KVM: SVM: Update max number of vCPUs supported for x2AVIC mode
  KVM: SVM: Update avic_kick_target_vcpus to support 32-bit APIC ID
  KVM: SVM: Do not support updating APIC ID when in x2APIC mode
  KVM: SVM: Adding support for configuring x2APIC MSRs interception
  KVM: SVM: Update AVIC settings when changing APIC mode
  KVM: SVM: Introduce helper functions to (de)activate AVIC and x2AVIC
  KVM: SVM: Do not throw warning when calling avic_vcpu_load on a
    running vcpu
  KVM: SVM: Do not inhibit APICv when x2APIC is present
  kvm/x86: Remove APICV activate mode inconsistency check

 arch/x86/hyperv/hv_apic.c          |   2 +-
 arch/x86/include/asm/apicdef.h     |   4 +-
 arch/x86/include/asm/cpufeatures.h |   1 +
 arch/x86/include/asm/svm.h         |  16 ++-
 arch/x86/kernel/apic/apic.c        |   2 +-
 arch/x86/kernel/apic/ipi.c         |   2 +-
 arch/x86/kvm/lapic.c               |   2 +-
 arch/x86/kvm/svm/avic.c            | 152 ++++++++++++++++++++++++++---
 arch/x86/kvm/svm/svm.c             |  55 ++++++-----
 arch/x86/kvm/svm/svm.h             |   7 +-
 arch/x86/kvm/x86.c                 |  13 +--
 11 files changed, 204 insertions(+), 52 deletions(-)

-- 
2.25.1

[PATCH v2 01/12] x86/cpufeatures: Introduce x2AVIC CPUID bit

[Suravee Suthikulpanit]

Introduce a new feature bit for virtualized x2APIC (x2AVIC) in
CPUID_Fn8000000A_EDX [SVM Revision and Feature Identification].

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/include/asm/cpufeatures.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 73e643ae94b6..ece314d43e08 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -343,6 +343,7 @@
 #define X86_FEATURE_AVIC		(15*32+13) /* Virtual Interrupt Controller */
 #define X86_FEATURE_V_VMSAVE_VMLOAD	(15*32+15) /* Virtual VMSAVE VMLOAD */
 #define X86_FEATURE_VGIF		(15*32+16) /* Virtual GIF */
+#define X86_FEATURE_X2AVIC		(15*32+18) /* Virtual x2apic */
 #define X86_FEATURE_V_SPEC_CTRL		(15*32+20) /* Virtual SPEC_CTRL */
 #define X86_FEATURE_SVME_ADDR_CHK	(15*32+28) /* "" SVME addr check */
 
-- 
2.25.1

[PATCH v2 02/12] KVM: x86: lapic: Rename [GET/SET]_APIC_DEST_FIELD to [GET/SET]_XAPIC_DEST_FIELD

[Suravee Suthikulpanit]

To signify that the macros only support 8-bit xAPIC destination ID.

Suggested-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/hyperv/hv_apic.c      | 2 +-
 arch/x86/include/asm/apicdef.h | 4 ++--
 arch/x86/kernel/apic/apic.c    | 2 +-
 arch/x86/kernel/apic/ipi.c     | 2 +-
 arch/x86/kvm/lapic.c           | 2 +-
 arch/x86/kvm/svm/avic.c        | 2 +-
 6 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/hyperv/hv_apic.c b/arch/x86/hyperv/hv_apic.c
index db2d92fb44da..fb8b2c088681 100644
--- a/arch/x86/hyperv/hv_apic.c
+++ b/arch/x86/hyperv/hv_apic.c
@@ -46,7 +46,7 @@ static void hv_apic_icr_write(u32 low, u32 id)
 {
 	u64 reg_val;
 
-	reg_val = SET_APIC_DEST_FIELD(id);
+	reg_val = SET_XAPIC_DEST_FIELD(id);
 	reg_val = reg_val << 32;
 	reg_val |= low;
 
diff --git a/arch/x86/include/asm/apicdef.h b/arch/x86/include/asm/apicdef.h
index 5716f22f81ac..863c2cad5872 100644
--- a/arch/x86/include/asm/apicdef.h
+++ b/arch/x86/include/asm/apicdef.h
@@ -89,8 +89,8 @@
 #define		APIC_DM_EXTINT		0x00700
 #define		APIC_VECTOR_MASK	0x000FF
 #define	APIC_ICR2	0x310
-#define		GET_APIC_DEST_FIELD(x)	(((x) >> 24) & 0xFF)
-#define		SET_APIC_DEST_FIELD(x)	((x) << 24)
+#define		GET_XAPIC_DEST_FIELD(x)	(((x) >> 24) & 0xFF)
+#define		SET_XAPIC_DEST_FIELD(x)	((x) << 24)
 #define	APIC_LVTT	0x320
 #define	APIC_LVTTHMR	0x330
 #define	APIC_LVTPC	0x340
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index b70344bf6600..e6b754e43ed7 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -275,7 +275,7 @@ void native_apic_icr_write(u32 low, u32 id)
 	unsigned long flags;
 
 	local_irq_save(flags);
-	apic_write(APIC_ICR2, SET_APIC_DEST_FIELD(id));
+	apic_write(APIC_ICR2, SET_XAPIC_DEST_FIELD(id));
 	apic_write(APIC_ICR, low);
 	local_irq_restore(flags);
 }
diff --git a/arch/x86/kernel/apic/ipi.c b/arch/x86/kernel/apic/ipi.c
index d1fb874fbe64..2a6509e8c840 100644
--- a/arch/x86/kernel/apic/ipi.c
+++ b/arch/x86/kernel/apic/ipi.c
@@ -99,7 +99,7 @@ void native_send_call_func_ipi(const struct cpumask *mask)
 
 static inline int __prepare_ICR2(unsigned int mask)
 {
-	return SET_APIC_DEST_FIELD(mask);
+	return SET_XAPIC_DEST_FIELD(mask);
 }
 
 static inline void __xapic_wait_icr_idle(void)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 66b0eb0bda94..f68c92c99abf 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1325,7 +1325,7 @@ void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high)
 	if (apic_x2apic_mode(apic))
 		irq.dest_id = icr_high;
 	else
-		irq.dest_id = GET_APIC_DEST_FIELD(icr_high);
+		irq.dest_id = GET_XAPIC_DEST_FIELD(icr_high);
 
 	trace_kvm_apic_ipi(icr_low, irq.dest_id);
 
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index a1cf9c31273b..655a7d20f8ee 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -299,7 +299,7 @@ static void avic_kick_target_vcpus(struct kvm *kvm, struct kvm_lapic *source,
 	 */
 	kvm_for_each_vcpu(i, vcpu, kvm) {
 		if (kvm_apic_match_dest(vcpu, source, icrl & APIC_SHORT_MASK,
-					GET_APIC_DEST_FIELD(icrh),
+					GET_XAPIC_DEST_FIELD(icrh),
 					icrl & APIC_DEST_MASK)) {
 			vcpu->arch.apic->irr_pending = true;
 			svm_complete_interrupt_delivery(vcpu,
-- 
2.25.1

[PATCH v2 03/12] KVM: SVM: Detect X2APIC virtualization (x2AVIC) support

[Suravee Suthikulpanit]

Add CPUID check for the x2APIC virtualization (x2AVIC) feature.
If available, the SVM driver can support both AVIC and x2AVIC modes
when load the kvm_amd driver with avic=1. The operating mode will be
determined at runtime depending on the guest APIC mode.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/include/asm/svm.h |  3 +++
 arch/x86/kvm/svm/avic.c    | 34 ++++++++++++++++++++++++++++++++++
 arch/x86/kvm/svm/svm.c     |  8 ++------
 arch/x86/kvm/svm/svm.h     |  1 +
 4 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index f70a5108d464..2c2a104b777e 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -195,6 +195,9 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
 #define AVIC_ENABLE_SHIFT 31
 #define AVIC_ENABLE_MASK (1 << AVIC_ENABLE_SHIFT)
 
+#define X2APIC_MODE_SHIFT 30
+#define X2APIC_MODE_MASK (1 << X2APIC_MODE_SHIFT)
+
 #define LBR_CTL_ENABLE_MASK BIT_ULL(0)
 #define VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK BIT_ULL(1)
 
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 655a7d20f8ee..fefac51063d3 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -40,6 +40,12 @@
 #define AVIC_GATAG_TO_VMID(x)		((x >> AVIC_VCPU_ID_BITS) & AVIC_VM_ID_MASK)
 #define AVIC_GATAG_TO_VCPUID(x)		(x & AVIC_VCPU_ID_MASK)
 
+enum avic_modes {
+	AVIC_MODE_NONE = 0,
+	AVIC_MODE_X1,
+	AVIC_MODE_X2,
+};
+
 /* Note:
  * This hash table is used to map VM_ID to a struct kvm_svm,
  * when handling AMD IOMMU GALOG notification to schedule in
@@ -50,6 +56,7 @@ static DEFINE_HASHTABLE(svm_vm_data_hash, SVM_VM_DATA_HASH_BITS);
 static u32 next_vm_id = 0;
 static bool next_vm_id_wrapped = 0;
 static DEFINE_SPINLOCK(svm_vm_data_hash_lock);
+static enum avic_modes avic_mode;
 
 /*
  * This is a wrapper of struct amd_iommu_ir_data.
@@ -1004,3 +1011,30 @@ void avic_vcpu_unblocking(struct kvm_vcpu *vcpu)
 
 	avic_vcpu_load(vcpu);
 }
+
+/*
+ * Note:
+ * - The module param avic enable both xAPIC and x2APIC mode.
+ * - Hypervisor can support both xAVIC and x2AVIC in the same guest.
+ * - The mode can be switched at run-time.
+ */
+bool avic_hardware_setup(struct kvm_x86_ops *x86_ops)
+{
+	if (!npt_enabled)
+		return false;
+
+	if (boot_cpu_has(X86_FEATURE_AVIC)) {
+		avic_mode = AVIC_MODE_X1;
+		pr_info("AVIC enabled\n");
+	}
+
+	if (boot_cpu_has(X86_FEATURE_X2AVIC)) {
+		avic_mode = AVIC_MODE_X2;
+		pr_info("x2AVIC enabled\n");
+	}
+
+	if (avic_mode != AVIC_MODE_NONE)
+		amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
+
+	return !!avic_mode;
+}
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index bd4c64b362d2..5ec770a1b4e8 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4806,13 +4806,9 @@ static __init int svm_hardware_setup(void)
 			nrips = false;
 	}
 
-	enable_apicv = avic = avic && npt_enabled && boot_cpu_has(X86_FEATURE_AVIC);
+	enable_apicv = avic = avic && avic_hardware_setup(&svm_x86_ops);
 
-	if (enable_apicv) {
-		pr_info("AVIC enabled\n");
-
-		amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
-	} else {
+	if (!enable_apicv) {
 		svm_x86_ops.vcpu_blocking = NULL;
 		svm_x86_ops.vcpu_unblocking = NULL;
 	}
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index f77a7d2d39dd..c44326eeb3f2 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -571,6 +571,7 @@ extern struct kvm_x86_nested_ops svm_nested_ops;
 
 /* avic.c */
 
+bool avic_hardware_setup(struct kvm_x86_ops *ops);
 int avic_ga_log_notifier(u32 ga_tag);
 void avic_vm_destroy(struct kvm *kvm);
 int avic_vm_init(struct kvm *kvm);
-- 
2.25.1

[PATCH v2 04/12] KVM: SVM: Update max number of vCPUs supported for x2AVIC mode

[Suravee Suthikulpanit]

xAVIC and x2AVIC modes can support diffferent number of vcpus.
Update existing logics to support each mode accordingly.

Also, modify the maximum physical APIC ID for AVIC to 255 to reflect
the actual value supported by the architecture.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/include/asm/svm.h | 12 +++++++++---
 arch/x86/kvm/svm/avic.c    |  8 +++++---
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index 2c2a104b777e..4c26b0d47d76 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -258,10 +258,16 @@ enum avic_ipi_failure_cause {
 
 
 /*
- * 0xff is broadcast, so the max index allowed for physical APIC ID
- * table is 0xfe.  APIC IDs above 0xff are reserved.
+ * For AVIC, the max index allowed for physical APIC ID
+ * table is 0xff (255).
  */
-#define AVIC_MAX_PHYSICAL_ID_COUNT	0xff
+#define AVIC_MAX_PHYSICAL_ID		0XFEULL
+
+/*
+ * For x2AVIC, the max index allowed for physical APIC ID
+ * table is 0x1ff (511).
+ */
+#define X2AVIC_MAX_PHYSICAL_ID		0x1FFUL
 
 #define AVIC_HPA_MASK	~((0xFFFULL << 52) | 0xFFF)
 #define VMCB_AVIC_APIC_BAR_MASK		0xFFFFFFFFFF000ULL
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index fefac51063d3..6c4519db3fc3 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -183,7 +183,7 @@ void avic_init_vmcb(struct vcpu_svm *svm)
 	vmcb->control.avic_backing_page = bpa & AVIC_HPA_MASK;
 	vmcb->control.avic_logical_id = lpa & AVIC_HPA_MASK;
 	vmcb->control.avic_physical_id = ppa & AVIC_HPA_MASK;
-	vmcb->control.avic_physical_id |= AVIC_MAX_PHYSICAL_ID_COUNT;
+	vmcb->control.avic_physical_id |= AVIC_MAX_PHYSICAL_ID;
 	vmcb->control.avic_vapic_bar = APIC_DEFAULT_PHYS_BASE & VMCB_AVIC_APIC_BAR_MASK;
 
 	if (kvm_apicv_activated(svm->vcpu.kvm))
@@ -198,7 +198,8 @@ static u64 *avic_get_physical_id_entry(struct kvm_vcpu *vcpu,
 	u64 *avic_physical_id_table;
 	struct kvm_svm *kvm_svm = to_kvm_svm(vcpu->kvm);
 
-	if (index >= AVIC_MAX_PHYSICAL_ID_COUNT)
+	if ((avic_mode == AVIC_MODE_X1 && index > AVIC_MAX_PHYSICAL_ID) ||
+	    (avic_mode == AVIC_MODE_X2 && index > X2AVIC_MAX_PHYSICAL_ID))
 		return NULL;
 
 	avic_physical_id_table = page_address(kvm_svm->avic_physical_id_table_page);
@@ -245,7 +246,8 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu)
 	int id = vcpu->vcpu_id;
 	struct vcpu_svm *svm = to_svm(vcpu);
 
-	if (id >= AVIC_MAX_PHYSICAL_ID_COUNT)
+	if ((avic_mode == AVIC_MODE_X1 && id > AVIC_MAX_PHYSICAL_ID) ||
+	    (avic_mode == AVIC_MODE_X2 && id > X2AVIC_MAX_PHYSICAL_ID))
 		return -EINVAL;
 
 	if (!vcpu->arch.apic->regs)
-- 
2.25.1

[PATCH v2 05/12] KVM: SVM: Update avic_kick_target_vcpus to support 32-bit APIC ID

[Suravee Suthikulpanit]

In x2APIC mode, ICRH contains 32-bit destination APIC ID.
So, update the avic_kick_target_vcpus() accordingly.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/kvm/svm/avic.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 6c4519db3fc3..609dcbe52a86 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -307,9 +307,15 @@ static void avic_kick_target_vcpus(struct kvm *kvm, struct kvm_lapic *source,
 	 * since entered the guest will have processed pending IRQs at VMRUN.
 	 */
 	kvm_for_each_vcpu(i, vcpu, kvm) {
+		u32 dest;
+
+		if (apic_x2apic_mode(vcpu->arch.apic))
+			dest = icrh;
+		else
+			dest = GET_XAPIC_DEST_FIELD(icrh);
+
 		if (kvm_apic_match_dest(vcpu, source, icrl & APIC_SHORT_MASK,
-					GET_XAPIC_DEST_FIELD(icrh),
-					icrl & APIC_DEST_MASK)) {
+					dest, icrl & APIC_DEST_MASK)) {
 			vcpu->arch.apic->irr_pending = true;
 			svm_complete_interrupt_delivery(vcpu,
 							icrl & APIC_MODE_MASK,
-- 
2.25.1

[PATCH v2 06/12] KVM: SVM: Do not support updating APIC ID when in x2APIC mode

[Suravee Suthikulpanit]

In X2APIC mode, the Logical Destination Register is read-only,
which provides a fixed mapping between the logical and physical
APIC IDs. Therefore, there is no Logical APIC ID table in X2AVIC
and the processor uses the X2APIC ID in the backing page to create
a vCPU’s logical ID.

In addition, KVM does not support updating APIC ID in x2APIC mode,
which means AVIC does not need to handle this case.

Therefore, check x2APIC mode when handling physical and logical
APIC ID update, and when invalidating logical APIC ID table.

Suggested-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/kvm/svm/avic.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 609dcbe52a86..22ee1098e2a5 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -424,8 +424,13 @@ static void avic_invalidate_logical_id_entry(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 	bool flat = svm->dfr_reg == APIC_DFR_FLAT;
-	u32 *entry = avic_get_logical_id_entry(vcpu, svm->ldr_reg, flat);
+	u32 *entry;
 
+	/* Note: x2AVIC does not use logical APIC ID table */
+	if (apic_x2apic_mode(vcpu->arch.apic))
+		return;
+
+	entry = avic_get_logical_id_entry(vcpu, svm->ldr_reg, flat);
 	if (entry)
 		clear_bit(AVIC_LOGICAL_ID_ENTRY_VALID_BIT, (unsigned long *)entry);
 }
@@ -437,6 +442,10 @@ static int avic_handle_ldr_update(struct kvm_vcpu *vcpu)
 	u32 ldr = kvm_lapic_get_reg(vcpu->arch.apic, APIC_LDR);
 	u32 id = kvm_xapic_id(vcpu->arch.apic);
 
+	/* AVIC does not support LDR update for x2APIC */
+	if (apic_x2apic_mode(vcpu->arch.apic))
+		return 0;
+
 	if (ldr == svm->ldr_reg)
 		return 0;
 
@@ -457,6 +466,14 @@ static int avic_handle_apic_id_update(struct kvm_vcpu *vcpu)
 	struct vcpu_svm *svm = to_svm(vcpu);
 	u32 id = kvm_xapic_id(vcpu->arch.apic);
 
+	/*
+	 * KVM does not support apic ID update for x2APIC.
+	 * Also, need to check if the APIC ID exceed 254.
+	 */
+	if (apic_x2apic_mode(vcpu->arch.apic) ||
+	    (vcpu->vcpu_id >= APIC_BROADCAST))
+		return 0;
+
 	if (vcpu->vcpu_id == id)
 		return 0;
 
-- 
2.25.1

Re: [PATCH v2 06/12] KVM: SVM: Do not support updating APIC ID when in x2APIC mode

[Maxim Levitsky]

On Tue, 2022-04-12 at 06:58 -0500, Suravee Suthikulpanit wrote:
> In X2APIC mode, the Logical Destination Register is read-only,
> which provides a fixed mapping between the logical and physical
> APIC IDs. Therefore, there is no Logical APIC ID table in X2AVIC
> and the processor uses the X2APIC ID in the backing page to create
> a vCPU’s logical ID.
> 
> In addition, KVM does not support updating APIC ID in x2APIC mode,
> which means AVIC does not need to handle this case.
> 
> Therefore, check x2APIC mode when handling physical and logical
> APIC ID update, and when invalidating logical APIC ID table.
> 
> Suggested-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/kvm/svm/avic.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 609dcbe52a86..22ee1098e2a5 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -424,8 +424,13 @@ static void avic_invalidate_logical_id_entry(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	bool flat = svm->dfr_reg == APIC_DFR_FLAT;
> -	u32 *entry = avic_get_logical_id_entry(vcpu, svm->ldr_reg, flat);
> +	u32 *entry;
>  
> +	/* Note: x2AVIC does not use logical APIC ID table */
> +	if (apic_x2apic_mode(vcpu->arch.apic))
> +		return;
> +
> +	entry = avic_get_logical_id_entry(vcpu, svm->ldr_reg, flat);
>  	if (entry)
>  		clear_bit(AVIC_LOGICAL_ID_ENTRY_VALID_BIT, (unsigned long *)entry);
>  }
> @@ -437,6 +442,10 @@ static int avic_handle_ldr_update(struct kvm_vcpu *vcpu)
>  	u32 ldr = kvm_lapic_get_reg(vcpu->arch.apic, APIC_LDR);
>  	u32 id = kvm_xapic_id(vcpu->arch.apic);
>  
> +	/* AVIC does not support LDR update for x2APIC */
> +	if (apic_x2apic_mode(vcpu->arch.apic))
> +		return 0;
> +
>  	if (ldr == svm->ldr_reg)
>  		return 0;
>  
> @@ -457,6 +466,14 @@ static int avic_handle_apic_id_update(struct kvm_vcpu *vcpu)
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	u32 id = kvm_xapic_id(vcpu->arch.apic);
>  
> +	/*
> +	 * KVM does not support apic ID update for x2APIC.
> +	 * Also, need to check if the APIC ID exceed 254.
> +	 */
> +	if (apic_x2apic_mode(vcpu->arch.apic) ||
> +	    (vcpu->vcpu_id >= APIC_BROADCAST))
> +		return 0;
> +
>  	if (vcpu->vcpu_id == id)
>  		return 0;
>  

Honestly I won't even bother with this - avic_handle_apic_id_update
zeros the initial apic id entry, after it moves it contents to new location.

If it is called again (and it will be called next time there is avic inhibition),
it will just fail as the initial entry will be zero, or copy wrong entry
if something else was written there meanwhile.

That code just needs to be removed, and AVIC be conditional on read-only apic id.

But overall this patch doesn't make things worse, so

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

Best regards,
	Maxim Levitsky

[PATCH v2 07/12] KVM: SVM: Adding support for configuring x2APIC MSRs interception

[Suravee Suthikulpanit]

When enabling x2APIC virtualization (x2AVIC), the interception of
x2APIC MSRs must be disabled to let the hardware virtualize guest
MSR accesses.

Current implementation keeps track of list of MSR interception state
in the svm_direct_access_msrs array. Therefore, extends the array to
include x2APIC MSRs.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/kvm/svm/svm.c | 29 ++++++++++++++++++++++++++++-
 arch/x86/kvm/svm/svm.h |  5 +++--
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 5ec770a1b4e8..c85663b62d4e 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -76,7 +76,7 @@ static uint64_t osvw_len = 4, osvw_status;
 
 static DEFINE_PER_CPU(u64, current_tsc_ratio);
 
-static const struct svm_direct_access_msrs {
+static struct svm_direct_access_msrs {
 	u32 index;   /* Index of the MSR */
 	bool always; /* True if intercept is initially cleared */
 } direct_access_msrs[MAX_DIRECT_ACCESS_MSRS] = {
@@ -774,6 +774,32 @@ static void add_msr_offset(u32 offset)
 	BUG();
 }
 
+static void init_direct_access_msrs(void)
+{
+	int i, j;
+
+	/* Find first MSR_INVALID */
+	for (i = 0; i < MAX_DIRECT_ACCESS_MSRS; i++) {
+		if (direct_access_msrs[i].index == MSR_INVALID)
+			break;
+	}
+	BUG_ON(i >= MAX_DIRECT_ACCESS_MSRS);
+
+	/*
+	 * Initialize direct_access_msrs entries to intercept X2APIC MSRs
+	 * (range 0x800 to 0x8ff)
+	 */
+	for (j = 0; j < 0x100; j++) {
+		direct_access_msrs[i + j].index = APIC_BASE_MSR + j;
+		direct_access_msrs[i + j].always = false;
+	}
+	BUG_ON(i + j >= MAX_DIRECT_ACCESS_MSRS);
+
+	/* Initialize last entry */
+	direct_access_msrs[i + j].index = MSR_INVALID;
+	direct_access_msrs[i + j].always = true;
+}
+
 static void init_msrpm_offsets(void)
 {
 	int i;
@@ -4739,6 +4765,7 @@ static __init int svm_hardware_setup(void)
 	memset(iopm_va, 0xff, PAGE_SIZE * (1 << order));
 	iopm_base = page_to_pfn(iopm_pages) << PAGE_SHIFT;
 
+	init_direct_access_msrs();
 	init_msrpm_offsets();
 
 	supported_xcr0 &= ~(XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR);
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index c44326eeb3f2..e340c86941be 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -29,8 +29,9 @@
 #define	IOPM_SIZE PAGE_SIZE * 3
 #define	MSRPM_SIZE PAGE_SIZE * 2
 
-#define MAX_DIRECT_ACCESS_MSRS	20
-#define MSRPM_OFFSETS	16
+#define MAX_DIRECT_ACCESS_MSRS	(20 + 0x100)
+#define MSRPM_OFFSETS	30
+
 extern u32 msrpm_offsets[MSRPM_OFFSETS] __read_mostly;
 extern bool npt_enabled;
 extern bool intercept_smi;
-- 
2.25.1

Re: [PATCH v2 07/12] KVM: SVM: Adding support for configuring x2APIC MSRs interception

[Maxim Levitsky]

On Tue, 2022-04-12 at 06:58 -0500, Suravee Suthikulpanit wrote:
> When enabling x2APIC virtualization (x2AVIC), the interception of
> x2APIC MSRs must be disabled to let the hardware virtualize guest
> MSR accesses.
> 
> Current implementation keeps track of list of MSR interception state
> in the svm_direct_access_msrs array. Therefore, extends the array to
> include x2APIC MSRs.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/kvm/svm/svm.c | 29 ++++++++++++++++++++++++++++-
>  arch/x86/kvm/svm/svm.h |  5 +++--
>  2 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 5ec770a1b4e8..c85663b62d4e 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -76,7 +76,7 @@ static uint64_t osvw_len = 4, osvw_status;
>  
>  static DEFINE_PER_CPU(u64, current_tsc_ratio);
>  
> -static const struct svm_direct_access_msrs {
> +static struct svm_direct_access_msrs {
>  	u32 index;   /* Index of the MSR */
>  	bool always; /* True if intercept is initially cleared */
>  } direct_access_msrs[MAX_DIRECT_ACCESS_MSRS] = {
> @@ -774,6 +774,32 @@ static void add_msr_offset(u32 offset)
>  	BUG();
>  }
>  
> +static void init_direct_access_msrs(void)
> +{
> +	int i, j;
> +
> +	/* Find first MSR_INVALID */
> +	for (i = 0; i < MAX_DIRECT_ACCESS_MSRS; i++) {
> +		if (direct_access_msrs[i].index == MSR_INVALID)
> +			break;
> +	}
> +	BUG_ON(i >= MAX_DIRECT_ACCESS_MSRS);
> +
> +	/*
> +	 * Initialize direct_access_msrs entries to intercept X2APIC MSRs
> +	 * (range 0x800 to 0x8ff)
> +	 */
> +	for (j = 0; j < 0x100; j++) {
> +		direct_access_msrs[i + j].index = APIC_BASE_MSR + j;
> +		direct_access_msrs[i + j].always = false;
> +	}

That looks *much cleaner* code wise even though it is slower 
because now we have 256 more msrs in this list.

So the best of the two worlds I think would be to add only APIC msrs that AVIC
actually handles to the list.

SDM has a table of these registers in 

"15.29.3.1 Virtual APIC Register Accesses"

I would add the registers that are either read/write allowed 
or at least cause traps.

Besides this, the patch looks fine.

Best regards,
	Maxim Levitsky


> +	BUG_ON(i + j >= MAX_DIRECT_ACCESS_MSRS);
> +
> +	/* Initialize last entry */
> +	direct_access_msrs[i + j].index = MSR_INVALID;
> +	direct_access_msrs[i + j].always = true;
> +}
> +
>  static void init_msrpm_offsets(void)
>  {
>  	int i;
> @@ -4739,6 +4765,7 @@ static __init int svm_hardware_setup(void)
>  	memset(iopm_va, 0xff, PAGE_SIZE * (1 << order));
>  	iopm_base = page_to_pfn(iopm_pages) << PAGE_SHIFT;
>  
> +	init_direct_access_msrs();
>  	init_msrpm_offsets();
>  
>  	supported_xcr0 &= ~(XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR);
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index c44326eeb3f2..e340c86941be 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -29,8 +29,9 @@
>  #define	IOPM_SIZE PAGE_SIZE * 3
>  #define	MSRPM_SIZE PAGE_SIZE * 2
>  
> -#define MAX_DIRECT_ACCESS_MSRS	20
> -#define MSRPM_OFFSETS	16
> +#define MAX_DIRECT_ACCESS_MSRS	(20 + 0x100)
> +#define MSRPM_OFFSETS	30
> +
>  extern u32 msrpm_offsets[MSRPM_OFFSETS] __read_mostly;
>  extern bool npt_enabled;
>  extern bool intercept_smi;

[PATCH v2 08/12] KVM: SVM: Update AVIC settings when changing APIC mode

[Suravee Suthikulpanit]

When APIC mode is updated (e.g. disabled, xAPIC, or x2APIC),
KVM needs to call kvm_vcpu_update_apicv() to update AVIC settings
accordingly.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/kvm/svm/avic.c | 15 +++++++++++++++
 arch/x86/kvm/svm/svm.c  |  1 +
 2 files changed, 16 insertions(+)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 22ee1098e2a5..01392b8364f4 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -616,6 +616,21 @@ void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu)
 	avic_handle_ldr_update(vcpu);
 }
 
+void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	if (!lapic_in_kernel(vcpu) || (avic_mode == AVIC_MODE_NONE))
+		return;
+
+	if (kvm_get_apic_mode(vcpu) == LAPIC_MODE_INVALID) {
+		WARN_ONCE(true, "Invalid local APIC state (vcpu_id=%d)", vcpu->vcpu_id);
+		return;
+	}
+
+	kvm_vcpu_update_apicv(&svm->vcpu);
+}
+
 static int avic_set_pi_irte_mode(struct kvm_vcpu *vcpu, bool activate)
 {
 	int ret = 0;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index c85663b62d4e..b7dbd8bb2c0a 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4606,6 +4606,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
 	.enable_nmi_window = svm_enable_nmi_window,
 	.enable_irq_window = svm_enable_irq_window,
 	.update_cr8_intercept = svm_update_cr8_intercept,
+	.set_virtual_apic_mode = avic_set_virtual_apic_mode,
 	.refresh_apicv_exec_ctrl = avic_refresh_apicv_exec_ctrl,
 	.check_apicv_inhibit_reasons = avic_check_apicv_inhibit_reasons,
 	.apicv_post_state_restore = avic_apicv_post_state_restore,
-- 
2.25.1

Re: [PATCH v2 08/12] KVM: SVM: Update AVIC settings when changing APIC mode

[Maxim Levitsky]

On Tue, 2022-04-12 at 06:58 -0500, Suravee Suthikulpanit wrote:
> When APIC mode is updated (e.g. disabled, xAPIC, or x2APIC),
> KVM needs to call kvm_vcpu_update_apicv() to update AVIC settings
> accordingly.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/kvm/svm/avic.c | 15 +++++++++++++++
>  arch/x86/kvm/svm/svm.c  |  1 +
>  2 files changed, 16 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 22ee1098e2a5..01392b8364f4 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -616,6 +616,21 @@ void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu)
>  	avic_handle_ldr_update(vcpu);
>  }
>  
> +void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
> +{
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +
> +	if (!lapic_in_kernel(vcpu) || (avic_mode == AVIC_MODE_NONE))
> +		return;
> +
> +	if (kvm_get_apic_mode(vcpu) == LAPIC_MODE_INVALID) {
> +		WARN_ONCE(true, "Invalid local APIC state (vcpu_id=%d)", vcpu->vcpu_id);
> +		return;
> +	}
> +
> +	kvm_vcpu_update_apicv(&svm->vcpu);

I think it makes sense to call avic_refresh_apicv_exec_ctrl directly here.
 
I am not sure that kvm_vcpu_update_apicv will even call it
because it has an optimization of doing nothing when inhibition status
didn't change.
 
 
Another semi-related note:
 
the current way the x2avic msrs are configured creates slight performance 
problem for nesting:
 
The problem is that when entering a nested guest, AVIC on the current vCPU
is inhibited, but this is done only so that this vCPU *peers* don't
try to use AVIC to send IPIs to it, so there is no need to update vmcb01
msr interception bitmap, and vmcb02 should have all these msrs intercepted always.
Same with returning to host.

It also should be checked that during nested entry, at least vmcb01 msr bitmap
is updated - TL;DR - please check that x2avic works when there is a nested guest running.
 

My avic nested coexistence patches are already upstream (kvm/queue) so this is already
relevant.
 
With nested x2avic (which will be very easy to do after my nested AVIC code is
upstream, vmcb02 will need to have x2avic msrs not intercepted as long as L1
doesn't want to intercept them if nested x2avic is enabled).

Best regards,
	Maxim Levitsky


> +}
> +
>  static int avic_set_pi_irte_mode(struct kvm_vcpu *vcpu, bool activate)
>  {
>  	int ret = 0;
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index c85663b62d4e..b7dbd8bb2c0a 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4606,6 +4606,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
>  	.enable_nmi_window = svm_enable_nmi_window,
>  	.enable_irq_window = svm_enable_irq_window,
>  	.update_cr8_intercept = svm_update_cr8_intercept,
> +	.set_virtual_apic_mode = avic_set_virtual_apic_mode,
>  	.refresh_apicv_exec_ctrl = avic_refresh_apicv_exec_ctrl,
>  	.check_apicv_inhibit_reasons = avic_check_apicv_inhibit_reasons,
>  	.apicv_post_state_restore = avic_apicv_post_state_restore,

Re: [PATCH v2 08/12] KVM: SVM: Update AVIC settings when changing APIC mode

[Suravee Suthikulpanit]

Maxim, Sean

On 4/18/22 7:55 PM, Maxim Levitsky wrote:
> On Tue, 2022-04-12 at 06:58 -0500, Suravee Suthikulpanit wrote:
>> When APIC mode is updated (e.g. disabled, xAPIC, or x2APIC),
>> KVM needs to call kvm_vcpu_update_apicv() to update AVIC settings
>> accordingly.
>>
>> Signed-off-by: Suravee Suthikulpanit<suravee.suthikulpanit@amd.com>
>> ---
>>   arch/x86/kvm/svm/avic.c | 15 +++++++++++++++
>>   arch/x86/kvm/svm/svm.c  |  1 +
>>   2 files changed, 16 insertions(+)
>>
>> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
>> index 22ee1098e2a5..01392b8364f4 100644
>> --- a/arch/x86/kvm/svm/avic.c
>> +++ b/arch/x86/kvm/svm/avic.c
>> @@ -616,6 +616,21 @@ void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu)
>>   	avic_handle_ldr_update(vcpu);
>>   }
>>   
>> +void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
>> +{
>> +	struct vcpu_svm *svm = to_svm(vcpu);
>> +
>> +	if (!lapic_in_kernel(vcpu) || (avic_mode == AVIC_MODE_NONE))
>> +		return;
>> +
>> +	if (kvm_get_apic_mode(vcpu) == LAPIC_MODE_INVALID) {
>> +		WARN_ONCE(true, "Invalid local APIC state (vcpu_id=%d)", vcpu->vcpu_id);
>> +		return;
>> +	}
>> +
>> +	kvm_vcpu_update_apicv(&svm->vcpu);
> I think it makes sense to call avic_refresh_apicv_exec_ctrl directly here.
>   
> I am not sure that kvm_vcpu_update_apicv will even call it
> because it has an optimization of doing nothing when inhibition status
> didn't change.
>   
>   
> Another semi-related note:
>   
> the current way the x2avic msrs are configured creates slight performance
> problem for nesting:
>   
> The problem is that when entering a nested guest, AVIC on the current vCPU
> is inhibited, but this is done only so that this vCPU*peers*  don't
> try to use AVIC to send IPIs to it, so there is no need to update vmcb01
> msr interception bitmap, and vmcb02 should have all these msrs intercepted always.
> Same with returning to host.
> 
> It also should be checked that during nested entry, at least vmcb01 msr bitmap
> is updated - TL;DR - please check that x2avic works when there is a nested guest running.

In the kvm/queue branch, I found a regression on nested SVM guest, where L2 guest cannot
launch. The bad commit is:

commit a4cfff3f0f8c07f1f7873a82bdeb3995807dac8c (bisect)
Merge: 42dcbe7d8bac 8d5678a76689
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Fri Apr 8 12:43:40 2022 -0400

     Merge branch 'kvm-older-features' into HEAD

     Merge branch for features that did not make it into 5.18:

     * New ioctls to get/set TSC frequency for a whole VM

     * Allow userspace to opt out of hypercall patching

     Nested virtualization improvements for AMD:

     * Support for "nested nested" optimizations (nested vVMLOAD/VMSAVE,
       nested vGIF)

     * Allow AVIC to co-exist with a nested guest running

     * Fixes for LBR virtualizations when a nested guest is running,
       and nested LBR virtualization support

     * PAUSE filtering for nested hypervisors

     Guest support:

     * Decoupling of vcpu_is_preempted from PV spinlocks

     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I am still working on the bisect into the merge commits.

Regards,
Suravee

Re: [PATCH v2 08/12] KVM: SVM: Update AVIC settings when changing APIC mode

[Maxim Levitsky]

On Mon, 2022-05-02 at 21:07 +0700, Suravee Suthikulpanit wrote:
> Maxim, Sean
> 
> On 4/18/22 7:55 PM, Maxim Levitsky wrote:
> > On Tue, 2022-04-12 at 06:58 -0500, Suravee Suthikulpanit wrote:
> > > When APIC mode is updated (e.g. disabled, xAPIC, or x2APIC),
> > > KVM needs to call kvm_vcpu_update_apicv() to update AVIC settings
> > > accordingly.
> > > 
> > > Signed-off-by: Suravee Suthikulpanit<suravee.suthikulpanit@amd.com>
> > > ---
> > >   arch/x86/kvm/svm/avic.c | 15 +++++++++++++++
> > >   arch/x86/kvm/svm/svm.c  |  1 +
> > >   2 files changed, 16 insertions(+)
> > > 
> > > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> > > index 22ee1098e2a5..01392b8364f4 100644
> > > --- a/arch/x86/kvm/svm/avic.c
> > > +++ b/arch/x86/kvm/svm/avic.c
> > > @@ -616,6 +616,21 @@ void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu)
> > >   	avic_handle_ldr_update(vcpu);
> > >   }
> > >   
> > > +void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
> > > +{
> > > +	struct vcpu_svm *svm = to_svm(vcpu);
> > > +
> > > +	if (!lapic_in_kernel(vcpu) || (avic_mode == AVIC_MODE_NONE))
> > > +		return;
> > > +
> > > +	if (kvm_get_apic_mode(vcpu) == LAPIC_MODE_INVALID) {
> > > +		WARN_ONCE(true, "Invalid local APIC state (vcpu_id=%d)", vcpu->vcpu_id);
> > > +		return;
> > > +	}
> > > +
> > > +	kvm_vcpu_update_apicv(&svm->vcpu);
> > I think it makes sense to call avic_refresh_apicv_exec_ctrl directly here.
> >   
> > I am not sure that kvm_vcpu_update_apicv will even call it
> > because it has an optimization of doing nothing when inhibition status
> > didn't change.
> >   
> >   
> > Another semi-related note:
> >   
> > the current way the x2avic msrs are configured creates slight performance
> > problem for nesting:
> >   
> > The problem is that when entering a nested guest, AVIC on the current vCPU
> > is inhibited, but this is done only so that this vCPU*peers*  don't
> > try to use AVIC to send IPIs to it, so there is no need to update vmcb01
> > msr interception bitmap, and vmcb02 should have all these msrs intercepted always.
> > Same with returning to host.
> > 
> > It also should be checked that during nested entry, at least vmcb01 msr bitmap
> > is updated - TL;DR - please check that x2avic works when there is a nested guest running.
> 
> In the kvm/queue branch, I found a regression on nested SVM guest, where L2 guest cannot
> launch. The bad commit is:
> 
> commit a4cfff3f0f8c07f1f7873a82bdeb3995807dac8c (bisect)
> Merge: 42dcbe7d8bac 8d5678a76689
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Fri Apr 8 12:43:40 2022 -0400
> 
>      Merge branch 'kvm-older-features' into HEAD
> 
>      Merge branch for features that did not make it into 5.18:
> 
>      * New ioctls to get/set TSC frequency for a whole VM
> 
>      * Allow userspace to opt out of hypercall patching
> 
>      Nested virtualization improvements for AMD:
> 
>      * Support for "nested nested" optimizations (nested vVMLOAD/VMSAVE,
>        nested vGIF)
> 
>      * Allow AVIC to co-exist with a nested guest running
> 
>      * Fixes for LBR virtualizations when a nested guest is running,
>        and nested LBR virtualization support
> 
>      * PAUSE filtering for nested hypervisors
> 
>      Guest support:
> 
>      * Decoupling of vcpu_is_preempted from PV spinlocks
> 
>      Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> I am still working on the bisect into the merge commits.
> 
> Regards,
> Suravee
> 

What happens when the guest can't launch? It sure works for me for kvm/queue
from yesterday.

I'll test again tomorrow.


Best regards,
	Maxim Levitsky

Re: [PATCH v2 08/12] KVM: SVM: Update AVIC settings when changing APIC mode

[Suravee Suthikulpanit]

Maxim,

On 5/3/22 12:13 AM, Maxim Levitsky wrote:
>> In the kvm/queue branch, I found a regression on nested SVM guest, where L2 guest cannot
>> launch. The bad commit is:
>>
>> commit a4cfff3f0f8c07f1f7873a82bdeb3995807dac8c (bisect)
>> Merge: 42dcbe7d8bac 8d5678a76689
>> Author: Paolo Bonzini<pbonzini@redhat.com>
>> Date:   Fri Apr 8 12:43:40 2022 -0400
>>
>>       Merge branch 'kvm-older-features' into HEAD
>>
>>       Merge branch for features that did not make it into 5.18:
>>
>>       * New ioctls to get/set TSC frequency for a whole VM
>>
>>       * Allow userspace to opt out of hypercall patching
>>
>>       Nested virtualization improvements for AMD:
>>
>>       * Support for "nested nested" optimizations (nested vVMLOAD/VMSAVE,
>>         nested vGIF)
>>
>>       * Allow AVIC to co-exist with a nested guest running
>>
>>       * Fixes for LBR virtualizations when a nested guest is running,
>>         and nested LBR virtualization support
>>
>>       * PAUSE filtering for nested hypervisors
>>
>>       Guest support:
>>
>>       * Decoupling of vcpu_is_preempted from PV spinlocks
>>
>>       Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>>
>> I am still working on the bisect into the merge commits.
>>
>> Regards,
>> Suravee
>>
> What happens when the guest can't launch? It sure works for me for kvm/queue
> from yesterday.
> 
> I'll test again tomorrow.

I have bisected it to this commit:

commit 74fd41ed16fd71725e69e2cb90b755505326c2e6
Author: Maxim Levitsky <mlevitsk@redhat.com>
Date:   Tue Mar 22 19:40:47 2022 +0200

     KVM: x86: nSVM: support PAUSE filtering when L0 doesn't intercept PAUSE

     Expose the pause filtering and threshold in the guest CPUID
     and support PAUSE filtering when possible:

     - If the L0 doesn't intercept PAUSE (cpu_pm=on), then allow L1 to
       have full control over PAUSE filtering.

     - if the L1 doesn't intercept PAUSE, use host values and update
       the adaptive count/threshold even when running nested.

     - Otherwise always exit to L1; it is not really possible to merge
       the fields correctly.  It is expected that in this case, userspace
       will not enable this feature in the guest CPUID, to avoid having the
       guest update both fields pointlessly.

     Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
     Message-Id: <20220322174050.241850-4-mlevitsk@redhat.com>
     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I can revert this one or specify pause_filter_count=0 pause_filter_thresh=0,
and then I can boot the L2 guest.

Regards,
Suravee

Re: [PATCH v2 08/12] KVM: SVM: Update AVIC settings when changing APIC mode

[Maxim Levitsky]

On Tue, 2022-05-03 at 20:04 +0700, Suravee Suthikulpanit wrote:
> Maxim,
> 
> On 5/3/22 12:13 AM, Maxim Levitsky wrote:
> > > In the kvm/queue branch, I found a regression on nested SVM guest, where L2 guest cannot
> > > launch. The bad commit is:
> > > 
> > > commit a4cfff3f0f8c07f1f7873a82bdeb3995807dac8c (bisect)
> > > Merge: 42dcbe7d8bac 8d5678a76689
> > > Author: Paolo Bonzini<pbonzini@redhat.com>
> > > Date:   Fri Apr 8 12:43:40 2022 -0400
> > > 
> > >       Merge branch 'kvm-older-features' into HEAD
> > > 
> > >       Merge branch for features that did not make it into 5.18:
> > > 
> > >       * New ioctls to get/set TSC frequency for a whole VM
> > > 
> > >       * Allow userspace to opt out of hypercall patching
> > > 
> > >       Nested virtualization improvements for AMD:
> > > 
> > >       * Support for "nested nested" optimizations (nested vVMLOAD/VMSAVE,
> > >         nested vGIF)
> > > 
> > >       * Allow AVIC to co-exist with a nested guest running
> > > 
> > >       * Fixes for LBR virtualizations when a nested guest is running,
> > >         and nested LBR virtualization support
> > > 
> > >       * PAUSE filtering for nested hypervisors
> > > 
> > >       Guest support:
> > > 
> > >       * Decoupling of vcpu_is_preempted from PV spinlocks
> > > 
> > >       Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> > > 
> > > I am still working on the bisect into the merge commits.
> > > 
> > > Regards,
> > > Suravee
> > > 
> > What happens when the guest can't launch? It sure works for me for kvm/queue
> > from yesterday.
> > 
> > I'll test again tomorrow.
> 
> I have bisected it to this commit:
> 
> commit 74fd41ed16fd71725e69e2cb90b755505326c2e6
> Author: Maxim Levitsky <mlevitsk@redhat.com>
> Date:   Tue Mar 22 19:40:47 2022 +0200
> 
>      KVM: x86: nSVM: support PAUSE filtering when L0 doesn't intercept PAUSE
> 
>      Expose the pause filtering and threshold in the guest CPUID
>      and support PAUSE filtering when possible:
> 
>      - If the L0 doesn't intercept PAUSE (cpu_pm=on), then allow L1 to
>        have full control over PAUSE filtering.
> 
>      - if the L1 doesn't intercept PAUSE, use host values and update
>        the adaptive count/threshold even when running nested.
> 
>      - Otherwise always exit to L1; it is not really possible to merge
>        the fields correctly.  It is expected that in this case, userspace
>        will not enable this feature in the guest CPUID, to avoid having the
>        guest update both fields pointlessly.
> 
>      Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
>      Message-Id: <20220322174050.241850-4-mlevitsk@redhat.com>
>      Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> I can revert this one or specify pause_filter_count=0 pause_filter_thresh=0,
> and then I can boot the L2 guest.
> 
> Regards,
> Suravee
> 

This is really wierd.

Could you share the qemu command line for L1 and L2 guest, and as much as possible
info on what happens when you boot L2? I tested latest kvm/queue and I don't see
any issues with booting nested guest.

Which hardware you test on? I test on Zen2 (3970X) mostly.

How many vCPUs L2 has? Could you do a kvm trace of the L2, from L1,
to see what it does prior to hang?


Best regards,
	Maxim Levitsky

Re: [PATCH v2 08/12] KVM: SVM: Update AVIC settings when changing APIC mode

[Maxim Levitsky]

On Wed, 2022-05-04 at 14:46 +0300, Maxim Levitsky wrote:
> On Tue, 2022-05-03 at 20:04 +0700, Suravee Suthikulpanit wrote:
> > Maxim,
> > 
> > On 5/3/22 12:13 AM, Maxim Levitsky wrote:
> > > > In the kvm/queue branch, I found a regression on nested SVM guest, where L2 guest cannot
> > > > launch. The bad commit is:
> > > > 
> > > > commit a4cfff3f0f8c07f1f7873a82bdeb3995807dac8c (bisect)
> > > > Merge: 42dcbe7d8bac 8d5678a76689
> > > > Author: Paolo Bonzini<pbonzini@redhat.com>
> > > > Date:   Fri Apr 8 12:43:40 2022 -0400
> > > > 
> > > >       Merge branch 'kvm-older-features' into HEAD
> > > > 
> > > >       Merge branch for features that did not make it into 5.18:
> > > > 
> > > >       * New ioctls to get/set TSC frequency for a whole VM
> > > > 
> > > >       * Allow userspace to opt out of hypercall patching
> > > > 
> > > >       Nested virtualization improvements for AMD:
> > > > 
> > > >       * Support for "nested nested" optimizations (nested vVMLOAD/VMSAVE,
> > > >         nested vGIF)
> > > > 
> > > >       * Allow AVIC to co-exist with a nested guest running
> > > > 
> > > >       * Fixes for LBR virtualizations when a nested guest is running,
> > > >         and nested LBR virtualization support
> > > > 
> > > >       * PAUSE filtering for nested hypervisors
> > > > 
> > > >       Guest support:
> > > > 
> > > >       * Decoupling of vcpu_is_preempted from PV spinlocks
> > > > 
> > > >       Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> > > > 
> > > > I am still working on the bisect into the merge commits.
> > > > 
> > > > Regards,
> > > > Suravee
> > > > 
> > > What happens when the guest can't launch? It sure works for me for kvm/queue
> > > from yesterday.
> > > 
> > > I'll test again tomorrow.
> > 
> > I have bisected it to this commit:
> > 
> > commit 74fd41ed16fd71725e69e2cb90b755505326c2e6
> > Author: Maxim Levitsky <mlevitsk@redhat.com>
> > Date:   Tue Mar 22 19:40:47 2022 +0200
> > 
> >      KVM: x86: nSVM: support PAUSE filtering when L0 doesn't intercept PAUSE
> > 
> >      Expose the pause filtering and threshold in the guest CPUID
> >      and support PAUSE filtering when possible:
> > 
> >      - If the L0 doesn't intercept PAUSE (cpu_pm=on), then allow L1 to
> >        have full control over PAUSE filtering.
> > 
> >      - if the L1 doesn't intercept PAUSE, use host values and update
> >        the adaptive count/threshold even when running nested.
> > 
> >      - Otherwise always exit to L1; it is not really possible to merge
> >        the fields correctly.  It is expected that in this case, userspace
> >        will not enable this feature in the guest CPUID, to avoid having the
> >        guest update both fields pointlessly.
> > 
> >      Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> >      Message-Id: <20220322174050.241850-4-mlevitsk@redhat.com>
> >      Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > I can revert this one or specify pause_filter_count=0 pause_filter_thresh=0,
> > and then I can boot the L2 guest.
> > 
> > Regards,
> > Suravee
> > 
> 
> This is really wierd.
> 
> Could you share the qemu command line for L1 and L2 guest, and as much as possible
> info on what happens when you boot L2? I tested latest kvm/queue and I don't see
> any issues with booting nested guest.
> 
> Which hardware you test on? I test on Zen2 (3970X) mostly.
> 
> How many vCPUs L2 has? Could you do a kvm trace of the L2, from L1,
> to see what it does prior to hang?


Also assuming that you boot the L2 with -cpu host, could you not expose these two
features to it?

-cpu host,pause-filter=off,pfthreshold=off

Best regards,
	Maxim Levitsky


> 
> 
> Best regards,
> 	Maxim Levitsky
> 
> 

Re: [PATCH v2 08/12] KVM: SVM: Update AVIC settings when changing APIC mode

[Maxim Levitsky]

On Wed, 2022-05-04 at 14:49 +0300, Maxim Levitsky wrote:
> On Wed, 2022-05-04 at 14:46 +0300, Maxim Levitsky wrote:
> > On Tue, 2022-05-03 at 20:04 +0700, Suravee Suthikulpanit wrote:
> > > Maxim,
> > > 
> > > On 5/3/22 12:13 AM, Maxim Levitsky wrote:
> > > > > In the kvm/queue branch, I found a regression on nested SVM guest, where L2 guest cannot
> > > > > launch. The bad commit is:
> > > > > 
> > > > > commit a4cfff3f0f8c07f1f7873a82bdeb3995807dac8c (bisect)
> > > > > Merge: 42dcbe7d8bac 8d5678a76689
> > > > > Author: Paolo Bonzini<pbonzini@redhat.com>
> > > > > Date:   Fri Apr 8 12:43:40 2022 -0400
> > > > > 
> > > > >       Merge branch 'kvm-older-features' into HEAD
> > > > > 
> > > > >       Merge branch for features that did not make it into 5.18:
> > > > > 
> > > > >       * New ioctls to get/set TSC frequency for a whole VM
> > > > > 
> > > > >       * Allow userspace to opt out of hypercall patching
> > > > > 
> > > > >       Nested virtualization improvements for AMD:
> > > > > 
> > > > >       * Support for "nested nested" optimizations (nested vVMLOAD/VMSAVE,
> > > > >         nested vGIF)
> > > > > 
> > > > >       * Allow AVIC to co-exist with a nested guest running
> > > > > 
> > > > >       * Fixes for LBR virtualizations when a nested guest is running,
> > > > >         and nested LBR virtualization support
> > > > > 
> > > > >       * PAUSE filtering for nested hypervisors
> > > > > 
> > > > >       Guest support:
> > > > > 
> > > > >       * Decoupling of vcpu_is_preempted from PV spinlocks
> > > > > 
> > > > >       Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> > > > > 
> > > > > I am still working on the bisect into the merge commits.
> > > > > 
> > > > > Regards,
> > > > > Suravee
> > > > > 
> > > > What happens when the guest can't launch? It sure works for me for kvm/queue
> > > > from yesterday.
> > > > 
> > > > I'll test again tomorrow.
> > > 
> > > I have bisected it to this commit:
> > > 
> > > commit 74fd41ed16fd71725e69e2cb90b755505326c2e6
> > > Author: Maxim Levitsky <mlevitsk@redhat.com>
> > > Date:   Tue Mar 22 19:40:47 2022 +0200
> > > 
> > >      KVM: x86: nSVM: support PAUSE filtering when L0 doesn't intercept PAUSE
> > > 
> > >      Expose the pause filtering and threshold in the guest CPUID
> > >      and support PAUSE filtering when possible:
> > > 
> > >      - If the L0 doesn't intercept PAUSE (cpu_pm=on), then allow L1 to
> > >        have full control over PAUSE filtering.
> > > 
> > >      - if the L1 doesn't intercept PAUSE, use host values and update
> > >        the adaptive count/threshold even when running nested.
> > > 
> > >      - Otherwise always exit to L1; it is not really possible to merge
> > >        the fields correctly.  It is expected that in this case, userspace
> > >        will not enable this feature in the guest CPUID, to avoid having the
> > >        guest update both fields pointlessly.
> > > 
> > >      Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > >      Message-Id: <20220322174050.241850-4-mlevitsk@redhat.com>
> > >      Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > > 
> > > I can revert this one or specify pause_filter_count=0 pause_filter_thresh=0,
> > > and then I can boot the L2 guest.

Another question? Where does it help to set this? In L0 kernel or in L1 kernel?

Best regards,
	Maxim Levitsky

> > > 
> > > Regards,
> > > Suravee
> > > 
> > 
> > This is really wierd.
> > 
> > Could you share the qemu command line for L1 and L2 guest, and as much as possible
> > info on what happens when you boot L2? I tested latest kvm/queue and I don't see
> > any issues with booting nested guest.
> > 
> > Which hardware you test on? I test on Zen2 (3970X) mostly.
> > 
> > How many vCPUs L2 has? Could you do a kvm trace of the L2, from L1,
> > to see what it does prior to hang?
> 
> Also assuming that you boot the L2 with -cpu host, could you not expose these two
> features to it?
> 
> -cpu host,pause-filter=off,pfthreshold=off
> 
> Best regards,
> 	Maxim Levitsky
> 
> 
> > 
> > Best regards,
> > 	Maxim Levitsky
> > 
> > 

[PATCH v2 09/12] KVM: SVM: Introduce helper functions to (de)activate AVIC and x2AVIC

[Suravee Suthikulpanit]

Refactor the current logic for (de)activate AVIC into helper functions,
and also add logic for (de)activate x2AVIC. The helper function are used
when initializing AVIC and switching from AVIC to x2AVIC mode
(handled by svm_refresh_spicv_exec_ctrl()).

When an AVIC-enabled guest switches from APIC to x2APIC mode during
runtime, the SVM driver needs to perform the following steps:

1. Set the x2APIC mode bit for AVIC in VMCB along with the maximum
APIC ID support for each mode accodingly.

2. Disable x2APIC MSRs interception in order to allow the hardware
to virtualize x2APIC MSRs accesses.

Reported-by: kernel test robot <lkp@intel.com>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/include/asm/svm.h |  1 +
 arch/x86/kvm/svm/avic.c    | 48 ++++++++++++++++++++++++++++++++++----
 2 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index 4c26b0d47d76..13d315b4eaba 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -256,6 +256,7 @@ enum avic_ipi_failure_cause {
 	AVIC_IPI_FAILURE_INVALID_BACKING_PAGE,
 };
 
+#define AVIC_PHYSICAL_MAX_INDEX_MASK	GENMASK_ULL(9, 0)
 
 /*
  * For AVIC, the max index allowed for physical APIC ID
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 01392b8364f4..462a1801916d 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -66,6 +66,45 @@ struct amd_svm_iommu_ir {
 	void *data;		/* Storing pointer to struct amd_ir_data */
 };
 
+static inline void avic_set_x2apic_msr_interception(struct vcpu_svm *svm, bool disable)
+{
+	int i;
+
+	for (i = 0x800; i <= 0x8ff; i++)
+		set_msr_interception(&svm->vcpu, svm->msrpm, i,
+				     !disable, !disable);
+}
+
+static void avic_activate_vmcb(struct vcpu_svm *svm)
+{
+	struct vmcb *vmcb = svm->vmcb01.ptr;
+
+	vmcb->control.int_ctl &= ~(AVIC_ENABLE_MASK | X2APIC_MODE_MASK);
+	vmcb->control.avic_physical_id &= ~AVIC_PHYSICAL_MAX_INDEX_MASK;
+
+	vmcb->control.int_ctl |= AVIC_ENABLE_MASK;
+	if (apic_x2apic_mode(svm->vcpu.arch.apic)) {
+		vmcb->control.int_ctl |= X2APIC_MODE_MASK;
+		vmcb->control.avic_physical_id |= X2AVIC_MAX_PHYSICAL_ID;
+		/* Disabling MSR intercept for x2APIC registers */
+		avic_set_x2apic_msr_interception(svm, false);
+	} else {
+		vmcb->control.avic_physical_id |= AVIC_MAX_PHYSICAL_ID;
+		/* Enabling MSR intercept for x2APIC registers */
+		avic_set_x2apic_msr_interception(svm, true);
+	}
+}
+
+static void avic_deactivate_vmcb(struct vcpu_svm *svm)
+{
+	struct vmcb *vmcb = svm->vmcb01.ptr;
+
+	vmcb->control.int_ctl &= ~(AVIC_ENABLE_MASK | X2APIC_MODE_MASK);
+	vmcb->control.avic_physical_id &= ~AVIC_PHYSICAL_MAX_INDEX_MASK;
+
+	/* Enabling MSR intercept for x2APIC registers */
+	avic_set_x2apic_msr_interception(svm, true);
+}
 
 /* Note:
  * This function is called from IOMMU driver to notify
@@ -183,13 +222,12 @@ void avic_init_vmcb(struct vcpu_svm *svm)
 	vmcb->control.avic_backing_page = bpa & AVIC_HPA_MASK;
 	vmcb->control.avic_logical_id = lpa & AVIC_HPA_MASK;
 	vmcb->control.avic_physical_id = ppa & AVIC_HPA_MASK;
-	vmcb->control.avic_physical_id |= AVIC_MAX_PHYSICAL_ID;
 	vmcb->control.avic_vapic_bar = APIC_DEFAULT_PHYS_BASE & VMCB_AVIC_APIC_BAR_MASK;
 
 	if (kvm_apicv_activated(svm->vcpu.kvm))
-		vmcb->control.int_ctl |= AVIC_ENABLE_MASK;
+		avic_activate_vmcb(svm);
 	else
-		vmcb->control.int_ctl &= ~AVIC_ENABLE_MASK;
+		avic_deactivate_vmcb(svm);
 }
 
 static u64 *avic_get_physical_id_entry(struct kvm_vcpu *vcpu,
@@ -1009,9 +1047,9 @@ void avic_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
 		 * accordingly before re-activating.
 		 */
 		avic_apicv_post_state_restore(vcpu);
-		vmcb->control.int_ctl |= AVIC_ENABLE_MASK;
+		avic_activate_vmcb(svm);
 	} else {
-		vmcb->control.int_ctl &= ~AVIC_ENABLE_MASK;
+		avic_deactivate_vmcb(svm);
 	}
 	vmcb_mark_dirty(vmcb, VMCB_AVIC);
 
-- 
2.25.1

[PATCH v2 10/12] KVM: SVM: Do not throw warning when calling avic_vcpu_load on a running vcpu

[Suravee Suthikulpanit]

Originalliy, this WARN_ON is designed to detect when calling
avic_vcpu_load() on an already running vcpu in AVIC mode (i.e. the AVIC
is_running bit is set).

However, for x2AVIC, the vCPU can switch from xAPIC to x2APIC mode while in
running state, in which the avic_vcpu_load() will be called from
svm_refresh_apicv_exec_ctrl().

Therefore, remove this warning since it is no longer appropriate.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/kvm/svm/avic.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 462a1801916d..085a82e95cb0 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -980,7 +980,6 @@ void __avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 		return;
 
 	entry = READ_ONCE(*(svm->avic_physical_id_cache));
-	WARN_ON(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK);
 
 	entry &= ~AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
 	entry |= (h_physical_id & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK);
-- 
2.25.1

[PATCH v2 11/12] KVM: SVM: Do not inhibit APICv when x2APIC is present

[Suravee Suthikulpanit]

Currently, AVIC is inhibited when booting a VM w/ x2APIC support.
This is because AVIC cannot virtualize x2APIC mode in the VM.
With x2AVIC support, the APICV_INHIBIT_REASON_X2APIC is
no longer enforced.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/kvm/svm/avic.c | 19 +++++++++++++++++++
 arch/x86/kvm/svm/svm.c  | 17 ++---------------
 arch/x86/kvm/svm/svm.h  |  1 +
 3 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 085a82e95cb0..abcf761c0c53 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -21,6 +21,7 @@
 
 #include <asm/irq_remapping.h>
 
+#include "cpuid.h"
 #include "trace.h"
 #include "lapic.h"
 #include "x86.h"
@@ -159,6 +160,24 @@ void avic_vm_destroy(struct kvm *kvm)
 	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
 }
 
+void avic_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu, int nested)
+{
+	/*
+	 * If the X2APIC feature is exposed to the guest,
+	 * disable AVIC unless X2AVIC mode is enabled.
+	 */
+	if (avic_mode == AVIC_MODE_X1 &&
+	    guest_cpuid_has(vcpu, X86_FEATURE_X2APIC))
+		kvm_set_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_X2APIC);
+
+	/*
+	 * Currently, AVIC does not work with nested virtualization.
+	 * So, we disable AVIC when cpuid for SVM is set in the L1 guest.
+	 */
+	if (nested && guest_cpuid_has(vcpu, X86_FEATURE_SVM))
+		kvm_set_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_NESTED);
+}
+
 int avic_vm_init(struct kvm *kvm)
 {
 	unsigned long flags;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index b7dbd8bb2c0a..931998d1d8c4 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3961,7 +3961,6 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 	struct kvm_cpuid_entry2 *best;
-	struct kvm *kvm = vcpu->kvm;
 
 	vcpu->arch.xsaves_enabled = guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
 				    boot_cpu_has(X86_FEATURE_XSAVE) &&
@@ -3982,21 +3981,9 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 			vcpu->arch.reserved_gpa_bits &= ~(1UL << (best->ebx & 0x3f));
 	}
 
-	if (kvm_vcpu_apicv_active(vcpu)) {
-		/*
-		 * AVIC does not work with an x2APIC mode guest. If the X2APIC feature
-		 * is exposed to the guest, disable AVIC.
-		 */
-		if (guest_cpuid_has(vcpu, X86_FEATURE_X2APIC))
-			kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_X2APIC);
+	if (kvm_vcpu_apicv_active(vcpu))
+		avic_vcpu_after_set_cpuid(vcpu, nested);
 
-		/*
-		 * Currently, AVIC does not work with nested virtualization.
-		 * So, we disable AVIC when cpuid for SVM is set in the L1 guest.
-		 */
-		if (nested && guest_cpuid_has(vcpu, X86_FEATURE_SVM))
-			kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_NESTED);
-	}
 	init_vmcb_after_set_cpuid(vcpu);
 }
 
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index e340c86941be..0312eec7c7f5 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -583,6 +583,7 @@ int avic_init_vcpu(struct vcpu_svm *svm);
 void __avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu);
 void __avic_vcpu_put(struct kvm_vcpu *vcpu);
 void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu);
+void avic_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu, int nested);
 void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu);
 void avic_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu);
 bool avic_check_apicv_inhibit_reasons(enum kvm_apicv_inhibit reason);
-- 
2.25.1

Re: [PATCH v2 11/12] KVM: SVM: Do not inhibit APICv when x2APIC is present

[Maxim Levitsky]

On Tue, 2022-04-12 at 06:58 -0500, Suravee Suthikulpanit wrote:
> Currently, AVIC is inhibited when booting a VM w/ x2APIC support.
> This is because AVIC cannot virtualize x2APIC mode in the VM.
> With x2AVIC support, the APICV_INHIBIT_REASON_X2APIC is
> no longer enforced.
> 
> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/kvm/svm/avic.c | 19 +++++++++++++++++++
>  arch/x86/kvm/svm/svm.c  | 17 ++---------------
>  arch/x86/kvm/svm/svm.h  |  1 +
>  3 files changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 085a82e95cb0..abcf761c0c53 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -21,6 +21,7 @@
>  
>  #include <asm/irq_remapping.h>
>  
> +#include "cpuid.h"
>  #include "trace.h"
>  #include "lapic.h"
>  #include "x86.h"
> @@ -159,6 +160,24 @@ void avic_vm_destroy(struct kvm *kvm)
>  	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
>  }
>  
> +void avic_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu, int nested)
> +{
> +	/*
> +	 * If the X2APIC feature is exposed to the guest,
> +	 * disable AVIC unless X2AVIC mode is enabled.
> +	 */
> +	if (avic_mode == AVIC_MODE_X1 &&
> +	    guest_cpuid_has(vcpu, X86_FEATURE_X2APIC))
> +		kvm_set_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_X2APIC);
> +
> +	/*
> +	 * Currently, AVIC does not work with nested virtualization.
> +	 * So, we disable AVIC when cpuid for SVM is set in the L1 guest.
> +	 */
> +	if (nested && guest_cpuid_has(vcpu, X86_FEATURE_SVM))
> +		kvm_set_apicv_inhibit(vcpu->kvm, APICV_INHIBIT_REASON_NESTED);
> +}
> +
>  int avic_vm_init(struct kvm *kvm)
>  {
>  	unsigned long flags;
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index b7dbd8bb2c0a..931998d1d8c4 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3961,7 +3961,6 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	struct kvm_cpuid_entry2 *best;
> -	struct kvm *kvm = vcpu->kvm;
>  
>  	vcpu->arch.xsaves_enabled = guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
>  				    boot_cpu_has(X86_FEATURE_XSAVE) &&
> @@ -3982,21 +3981,9 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  			vcpu->arch.reserved_gpa_bits &= ~(1UL << (best->ebx & 0x3f));
>  	}
>  
> -	if (kvm_vcpu_apicv_active(vcpu)) {
> -		/*
> -		 * AVIC does not work with an x2APIC mode guest. If the X2APIC feature
> -		 * is exposed to the guest, disable AVIC.
> -		 */
> -		if (guest_cpuid_has(vcpu, X86_FEATURE_X2APIC))
> -			kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_X2APIC);
> +	if (kvm_vcpu_apicv_active(vcpu))
> +		avic_vcpu_after_set_cpuid(vcpu, nested);
>  
> -		/*
> -		 * Currently, AVIC does not work with nested virtualization.
> -		 * So, we disable AVIC when cpuid for SVM is set in the L1 guest.
> -		 */
> -		if (nested && guest_cpuid_has(vcpu, X86_FEATURE_SVM))
> -			kvm_set_apicv_inhibit(kvm, APICV_INHIBIT_REASON_NESTED);
> -	}
>  	init_vmcb_after_set_cpuid(vcpu);
>  }
>  
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index e340c86941be..0312eec7c7f5 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -583,6 +583,7 @@ int avic_init_vcpu(struct vcpu_svm *svm);
>  void __avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu);
>  void __avic_vcpu_put(struct kvm_vcpu *vcpu);
>  void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu);
> +void avic_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu, int nested);
>  void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu);
>  void avic_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu);
>  bool avic_check_apicv_inhibit_reasons(enum kvm_apicv_inhibit reason);

Hi!


I just got an idea, while writing a kvm selftest that would use AVIC,
and finding out that selftest code uploads the '-host' cpuid right away
which has x2apic enabled and that inhibits AVIC, and later clearing x2apic
in the cpuid doesn't un-inhibit it.
 
That can be fixed in few ways but that got me thinking:
 
Why do we inhibit AVIC when the guest uses x2apic, even without X2AVIC?
I think that if we didn't it would just work, and even work faster than
pure software x2apic.
 
My thinking is:
 
- when a vcpu itself uses its x2apic, even if its avic is not inhibited, 
the guest will write x2apic msrs which kvm intercepts and will correctly emulate a proper x2apic.
 
- vcpu peers will also use x2apic msrs and again it will work correctly 
(even when there are more than 256 vcpus).
 
- and the host + iommu will still be able to use AVIC's doorbell to send interrupts to the guest
and that doesn't need apic ids or anything, it should work just fine. 

Also AVIC should have no issues scanning IRR and injecting interrupts on VM entry, 
x2apic mode doesn't matter for that.
 
AVIC mmio can still be though discovered by the guest which is technically against x86 spec
(in x2apic mode, mmio supposed to not work) but that can be fixed easily by disabing
the AVIC memslot if any of the vCPUs are in x2apic mode, or this can be ignored since
it should not cause any issues.
We seem to have a quirk for that KVM_X86_QUIRK_LAPIC_MMIO_HOLE.
 
On top of all this, removing this inhibit will also allow to test AVIC with guest
which does have x2apic in the CPUID but doesn't use it (e.g kvm unit test, or
linux booted with nox2apic, which is also nice IMHO)
 
What do you think?

Best regards,
	Maxim Levitsky

Re: [PATCH v2 11/12] KVM: SVM: Do not inhibit APICv when x2APIC is present

[Suravee Suthikulpanit]

Hi Maim,

On 4/19/22 8:29 PM, Maxim Levitsky wrote:
> On Tue, 2022-04-12 at 06:58 -0500, Suravee Suthikulpanit wrote:
> 
> Hi!
> 
> 
> I just got an idea, while writing a kvm selftest that would use AVIC,
> and finding out that selftest code uploads the '-host' cpuid right away
> which has x2apic enabled and that inhibits AVIC, and later clearing x2apic
> in the cpuid doesn't un-inhibit it.
>   
> That can be fixed in few ways but that got me thinking:
>   
> Why do we inhibit AVIC when the guest uses x2apic, even without X2AVIC?
> I think that if we didn't it would just work, and even work faster than
> pure software x2apic.
>   
> My thinking is:
>   
> - when a vcpu itself uses its x2apic, even if its avic is not inhibited,
> the guest will write x2apic msrs which kvm intercepts and will correctly emulate a proper x2apic.
>   
> - vcpu peers will also use x2apic msrs and again it will work correctly
> (even when there are more than 256 vcpus).
>   
> - and the host + iommu will still be able to use AVIC's doorbell to send interrupts to the guest
> and that doesn't need apic ids or anything, it should work just fine.
> 
> Also AVIC should have no issues scanning IRR and injecting interrupts on VM entry,
> x2apic mode doesn't matter for that.
>   
> AVIC mmio can still be though discovered by the guest which is technically against x86 spec
> (in x2apic mode, mmio supposed to not work) but that can be fixed easily by disabing
> the AVIC memslot if any of the vCPUs are in x2apic mode, or this can be ignored since
> it should not cause any issues.
> We seem to have a quirk for that KVM_X86_QUIRK_LAPIC_MMIO_HOLE.
>   
> On top of all this, removing this inhibit will also allow to test AVIC with guest
> which does have x2apic in the CPUID but doesn't use it (e.g kvm unit test, or
> linux booted with nox2apic, which is also nice IMHO)
>   
> What do you think?

This is actually a good idea!!! Let's call it hybrid-x2AVIC :)

I am working on prototype and test out the support for this, which will be introduced in V3.

Regards,
Suravee

Re: [PATCH v2 11/12] KVM: SVM: Do not inhibit APICv when x2APIC is present

[Maxim Levitsky]

On Tue, 2022-04-26 at 09:25 +0700, Suravee Suthikulpanit wrote:
> Hi Maim,
> 
> On 4/19/22 8:29 PM, Maxim Levitsky wrote:
> > On Tue, 2022-04-12 at 06:58 -0500, Suravee Suthikulpanit wrote:
> > 
> > Hi!
> > 
> > 
> > I just got an idea, while writing a kvm selftest that would use AVIC,
> > and finding out that selftest code uploads the '-host' cpuid right away
> > which has x2apic enabled and that inhibits AVIC, and later clearing x2apic
> > in the cpuid doesn't un-inhibit it.
> >   
> > That can be fixed in few ways but that got me thinking:
> >   
> > Why do we inhibit AVIC when the guest uses x2apic, even without X2AVIC?
> > I think that if we didn't it would just work, and even work faster than
> > pure software x2apic.
> >   
> > My thinking is:
> >   
> > - when a vcpu itself uses its x2apic, even if its avic is not inhibited,
> > the guest will write x2apic msrs which kvm intercepts and will correctly emulate a proper x2apic.
> >   
> > - vcpu peers will also use x2apic msrs and again it will work correctly
> > (even when there are more than 256 vcpus).
> >   
> > - and the host + iommu will still be able to use AVIC's doorbell to send interrupts to the guest
> > and that doesn't need apic ids or anything, it should work just fine.
> > 
> > Also AVIC should have no issues scanning IRR and injecting interrupts on VM entry,
> > x2apic mode doesn't matter for that.
> >   
> > AVIC mmio can still be though discovered by the guest which is technically against x86 spec
> > (in x2apic mode, mmio supposed to not work) but that can be fixed easily by disabing
> > the AVIC memslot if any of the vCPUs are in x2apic mode, or this can be ignored since
> > it should not cause any issues.
> > We seem to have a quirk for that KVM_X86_QUIRK_LAPIC_MMIO_HOLE.
> >   
> > On top of all this, removing this inhibit will also allow to test AVIC with guest
> > which does have x2apic in the CPUID but doesn't use it (e.g kvm unit test, or
> > linux booted with nox2apic, which is also nice IMHO)
> >   
> > What do you think?
> 
> This is actually a good idea!!! Let's call it hybrid-x2AVIC :)
> 
> I am working on prototype and test out the support for this, which will be introduced in V3.

Thanks! 

Best regards,
	Maxim Levitsky

> 
> Regards,
> Suravee
> 

Re: [PATCH v2 11/12] KVM: SVM: Do not inhibit APICv when x2APIC is present

[Maxim Levitsky]

On Tue, 2022-04-26 at 10:06 +0300, Maxim Levitsky wrote:
> On Tue, 2022-04-26 at 09:25 +0700, Suravee Suthikulpanit wrote:
> > Hi Maim,
> > 
> > On 4/19/22 8:29 PM, Maxim Levitsky wrote:
> > > On Tue, 2022-04-12 at 06:58 -0500, Suravee Suthikulpanit wrote:
> > > 
> > > Hi!
> > > 
> > > 
> > > I just got an idea, while writing a kvm selftest that would use AVIC,
> > > and finding out that selftest code uploads the '-host' cpuid right away
> > > which has x2apic enabled and that inhibits AVIC, and later clearing x2apic
> > > in the cpuid doesn't un-inhibit it.
> > >   
> > > That can be fixed in few ways but that got me thinking:
> > >   
> > > Why do we inhibit AVIC when the guest uses x2apic, even without X2AVIC?
> > > I think that if we didn't it would just work, and even work faster than
> > > pure software x2apic.
> > >   
> > > My thinking is:
> > >   
> > > - when a vcpu itself uses its x2apic, even if its avic is not inhibited,
> > > the guest will write x2apic msrs which kvm intercepts and will correctly emulate a proper x2apic.
> > >   
> > > - vcpu peers will also use x2apic msrs and again it will work correctly
> > > (even when there are more than 256 vcpus).
> > >   
> > > - and the host + iommu will still be able to use AVIC's doorbell to send interrupts to the guest
> > > and that doesn't need apic ids or anything, it should work just fine.
> > > 
> > > Also AVIC should have no issues scanning IRR and injecting interrupts on VM entry,
> > > x2apic mode doesn't matter for that.
> > >   
> > > AVIC mmio can still be though discovered by the guest which is technically against x86 spec
> > > (in x2apic mode, mmio supposed to not work) but that can be fixed easily by disabing
> > > the AVIC memslot if any of the vCPUs are in x2apic mode, or this can be ignored since
> > > it should not cause any issues.
> > > We seem to have a quirk for that KVM_X86_QUIRK_LAPIC_MMIO_HOLE.
> > >   
> > > On top of all this, removing this inhibit will also allow to test AVIC with guest
> > > which does have x2apic in the CPUID but doesn't use it (e.g kvm unit test, or
> > > linux booted with nox2apic, which is also nice IMHO)
> > >   
> > > What do you think?
> > 
> > This is actually a good idea!!! Let's call it hybrid-x2AVIC :)
> > 
> > I am working on prototype and test out the support for this, which will be introduced in V3.
> 
> Thanks! 
> 
> Best regards,
> 	Maxim Levitsky
> 
> > Regards,
> > Suravee
> > 

BTW, can I ask you to check something on the AMD side of things of AVIC?

I noticed that AMD's manual states that:

"Multiprocessor VM requirements. When running a VM which has multiple virtual CPUs, and the
VMM runs a virtual CPU on a core which had last run a different virtual CPU from the same VM,
regardless of the respective ASID values, care must be taken to flush the TLB on the VMRUN using a
TLB_CONTROL value of 3h. Failure to do so may result in stale mappings misdirecting virtual APIC
accesses to the previous virtual CPU's APIC backing page."

It it relevant to KVM? I don't fully understand why it was mentioned that ASID doesn't matter,
what makes it special about 'virtual CPU from the same VM' if ASID doesn't matter? 

Also, is this still relevant on modern AMD cpus, or was a workaround for some old CPU bug?

Best regards,
	Maxim Levitsky

Re: [PATCH v2 11/12] KVM: SVM: Do not inhibit APICv when x2APIC is present

[Sean Christopherson]

On Tue, Apr 26, 2022, Maxim Levitsky wrote:
> On Tue, 2022-04-26 at 10:06 +0300, Maxim Levitsky wrote:
> BTW, can I ask you to check something on the AMD side of things of AVIC?
> 
> I noticed that AMD's manual states that:
> 
> "Multiprocessor VM requirements. When running a VM which has multiple virtual CPUs, and the
> VMM runs a virtual CPU on a core which had last run a different virtual CPU from the same VM,
> regardless of the respective ASID values, care must be taken to flush the TLB on the VMRUN using a
> TLB_CONTROL value of 3h. Failure to do so may result in stale mappings misdirecting virtual APIC
> accesses to the previous virtual CPU's APIC backing page."
> 
> It it relevant to KVM? I don't fully understand why it was mentioned that ASID doesn't matter,
> what makes it special about 'virtual CPU from the same VM' if ASID doesn't matter? 

I believe it's calling out that, because vCPUs from the same VM likely share an ASID,
the magic TLB entry for the APIC-access page, which redirects to the virtual APIC page,
will be preserved.  And so if the hypervisor doesn't flush the ASID/TLB, accelerated
xAPIC accesses for the new vCPU will go to the previous vCPU's virtual APIC page.

Intel has the same requirement, though this specific scenario isn't as well documented.
E.g. even if using EPT and VPID, the EPT still needs to be invalidated because the
TLB can cache guest-physical mappings, which are not associated with a VPID.

Huh.  I was going to say that KVM does the necessary flushes in vmx_vcpu_load_vmcs()
and pre_svm_run(), but I don't think that's true.  KVM flushes if the _new_ VMCS/VMCB
is being migrated to a different pCPU, but neither VMX nor SVM flush when switching
between vCPUs that are both "loaded" on the current pCPU.

Switching between vmcs01 and vmcs02 is ok, because KVM always forces a different
EPTP, even if L1 is using shadow paging (the guest_mode bit in the role prevents
reusing a root).  nSVM is "ok" because it flushes on every transition anyways.

Re: [PATCH v2 11/12] KVM: SVM: Do not inhibit APICv when x2APIC is present

[Maxim Levitsky]

On Fri, 2022-04-29 at 17:00 +0000, Sean Christopherson wrote:
> On Tue, Apr 26, 2022, Maxim Levitsky wrote:
> > On Tue, 2022-04-26 at 10:06 +0300, Maxim Levitsky wrote:
> > BTW, can I ask you to check something on the AMD side of things of AVIC?
> > 
> > I noticed that AMD's manual states that:
> > 
> > "Multiprocessor VM requirements. When running a VM which has multiple virtual CPUs, and the
> > VMM runs a virtual CPU on a core which had last run a different virtual CPU from the same VM,
> > regardless of the respective ASID values, care must be taken to flush the TLB on the VMRUN using a
> > TLB_CONTROL value of 3h. Failure to do so may result in stale mappings misdirecting virtual APIC
> > accesses to the previous virtual CPU's APIC backing page."
> > 
> > It it relevant to KVM? I don't fully understand why it was mentioned that ASID doesn't matter,
> > what makes it special about 'virtual CPU from the same VM' if ASID doesn't matter? 
> 
> I believe it's calling out that, because vCPUs from the same VM likely share an ASID,
> the magic TLB entry for the APIC-access page, which redirects to the virtual APIC page,
> will be preserved.  And so if the hypervisor doesn't flush the ASID/TLB, accelerated
> xAPIC accesses for the new vCPU will go to the previous vCPU's virtual APIC page.

This is what I want to think as well, but the manual says explicitly 
"regardless of the respective ASID values"

On the face value of it, the only logical way to read this IMHO,
is that every time apic backing page is changed, we need to issue a TLB flush.

Best regards,
	Maxim Levitsky

> 
> Intel has the same requirement, though this specific scenario isn't as well documented.
> E.g. even if using EPT and VPID, the EPT still needs to be invalidated because the
> TLB can cache guest-physical mappings, which are not associated with a VPID.
> 
> Huh.  I was going to say that KVM does the necessary flushes in vmx_vcpu_load_vmcs()
> and pre_svm_run(), but I don't think that's true.  KVM flushes if the _new_ VMCS/VMCB
> is being migrated to a different pCPU, but neither VMX nor SVM flush when switching
> between vCPUs that are both "loaded" on the current pCPU.
> 
> Switching between vmcs01 and vmcs02 is ok, because KVM always forces a different
> EPTP, even if L1 is using shadow paging (the guest_mode bit in the role prevents
> reusing a root).  nSVM is "ok" because it flushes on every transition anyways.
> 

[PATCH v2 12/12] kvm/x86: Remove APICV activate mode inconsistency check

[Suravee Suthikulpanit]

When launching a VM with x2APIC and specify more than 255 vCPUs,
the guest kernel can disable x2APIC (e.g. specify nox2apic kernel option).
The VM fallbacks to xAPIC mode, and disable the vCPU ID 255.

In this case, APICV should be disabled for the vCPU ID 255.
Therefore, the APICV mode consisency check is no longer valid.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/kvm/x86.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0c0ca599a353..d0fac57e9996 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9765,6 +9765,11 @@ void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu)
 	down_read(&vcpu->kvm->arch.apicv_update_lock);
 
 	activate = kvm_apicv_activated(vcpu->kvm);
+
+	/* Do not activate AVIC when APIC is disabled */
+	if (kvm_get_apic_mode(vcpu) == LAPIC_MODE_DISABLED)
+		activate = false;
+
 	if (vcpu->arch.apicv_active == activate)
 		goto out;
 
@@ -10159,14 +10164,6 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	guest_timing_enter_irqoff();
 
 	for (;;) {
-		/*
-		 * Assert that vCPU vs. VM APICv state is consistent.  An APICv
-		 * update must kick and wait for all vCPUs before toggling the
-		 * per-VM state, and responsing vCPUs must wait for the update
-		 * to complete before servicing KVM_REQ_APICV_UPDATE.
-		 */
-		WARN_ON_ONCE(kvm_apicv_activated(vcpu->kvm) != kvm_vcpu_apicv_active(vcpu));
-
 		exit_fastpath = static_call(kvm_x86_vcpu_run)(vcpu);
 		if (likely(exit_fastpath != EXIT_FASTPATH_REENTER_GUEST))
 			break;
-- 
2.25.1

Re: [PATCH v2 12/12] kvm/x86: Remove APICV activate mode inconsistency check

[Maxim Levitsky]

On Tue, 2022-04-12 at 06:58 -0500, Suravee Suthikulpanit wrote:
> When launching a VM with x2APIC and specify more than 255 vCPUs,
> the guest kernel can disable x2APIC (e.g. specify nox2apic kernel option).
> The VM fallbacks to xAPIC mode, and disable the vCPU ID 255.
> 
> In this case, APICV should be disabled for the vCPU ID 255.
> Therefore, the APICV mode consisency check is no longer valid.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/kvm/x86.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 0c0ca599a353..d0fac57e9996 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9765,6 +9765,11 @@ void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu)
>  	down_read(&vcpu->kvm->arch.apicv_update_lock);
>  
>  	activate = kvm_apicv_activated(vcpu->kvm);
> +
> +	/* Do not activate AVIC when APIC is disabled */
> +	if (kvm_get_apic_mode(vcpu) == LAPIC_MODE_DISABLED)
> +		activate = false;
> +
>  	if (vcpu->arch.apicv_active == activate)
>  		goto out;
>  
> @@ -10159,14 +10164,6 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  	guest_timing_enter_irqoff();
>  
>  	for (;;) {
> -		/*
> -		 * Assert that vCPU vs. VM APICv state is consistent.  An APICv
> -		 * update must kick and wait for all vCPUs before toggling the
> -		 * per-VM state, and responsing vCPUs must wait for the update
> -		 * to complete before servicing KVM_REQ_APICV_UPDATE.
> -		 */
> -		WARN_ON_ONCE(kvm_apicv_activated(vcpu->kvm) != kvm_vcpu_apicv_active(vcpu));
> -
>  		exit_fastpath = static_call(kvm_x86_vcpu_run)(vcpu);
>  		if (likely(exit_fastpath != EXIT_FASTPATH_REENTER_GUEST))
>  			break;

That warning catches bugs, please don't remove it.

It can be made conditional on this vCPU having APIC enabled instead.

Best regards,
	Maxim Levitsky