[PATCH v2 0/4] KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN for CPU hotplug

[PATCH v2 0/4] KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN for CPU hotplug

[Vitaly Kuznetsov]

Changes since v1:
- Drop the allowlist of items which were allowed to change and just allow
the exact same CPUID data [Sean, Paolo]. Adjust selftest accordingly.
- Drop PATCH1 as the exact same change got merged upstream.

Recently, KVM made it illegal to change CPUID after KVM_RUN but
unfortunately this change is not fully compatible with existing VMMs.
In particular, QEMU reuses vCPU fds for CPU hotplug after unplug and it
calls KVM_SET_CPUID2. Relax the requirement by implementing an allowing
KVM_SET_CPUID{,2} with the exact same data.

Vitaly Kuznetsov (4):
  KVM: x86: Do runtime CPUID update before updating
    vcpu->arch.cpuid_entries
  KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN
  KVM: selftests: Rename 'get_cpuid_test' to 'cpuid_test'
  KVM: selftests: Test KVM_SET_CPUID2 after KVM_RUN

 arch/x86/kvm/cpuid.c                          | 67 ++++++++++++++++---
 arch/x86/kvm/x86.c                            | 19 ------
 tools/testing/selftests/kvm/.gitignore        |  2 +-
 tools/testing/selftests/kvm/Makefile          |  4 +-
 .../selftests/kvm/include/x86_64/processor.h  |  7 ++
 .../selftests/kvm/lib/x86_64/processor.c      | 33 +++++++--
 .../x86_64/{get_cpuid_test.c => cpuid_test.c} | 30 +++++++++
 7 files changed, 126 insertions(+), 36 deletions(-)
 rename tools/testing/selftests/kvm/x86_64/{get_cpuid_test.c => cpuid_test.c} (83%)

-- 
2.34.1

[PATCH v2 1/4] KVM: x86: Do runtime CPUID update before updating vcpu->arch.cpuid_entries

[Vitaly Kuznetsov]

kvm_update_cpuid_runtime() mangles CPUID data coming from userspace
VMM after updating 'vcpu->arch.cpuid_entries', this makes it
impossible to compare an update with what was previously
supplied. Introduce __kvm_update_cpuid_runtime() version which can be
used to tweak the input before it goes to 'vcpu->arch.cpuid_entries'
so the upcoming update check can compare tweaked data.

No functional change intended.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/cpuid.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index c55e57b30e81..812190a707f6 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -145,14 +145,21 @@ static void kvm_update_kvm_cpuid_base(struct kvm_vcpu *vcpu)
 	}
 }
 
-static struct kvm_cpuid_entry2 *kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcpu)
+static struct kvm_cpuid_entry2 *__kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcpu,
+					      struct kvm_cpuid_entry2 *entries, int nent)
 {
 	u32 base = vcpu->arch.kvm_cpuid_base;
 
 	if (!base)
 		return NULL;
 
-	return kvm_find_cpuid_entry(vcpu, base | KVM_CPUID_FEATURES, 0);
+	return cpuid_entry2_find(entries, nent, base | KVM_CPUID_FEATURES, 0);
+}
+
+static struct kvm_cpuid_entry2 *kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcpu)
+{
+	return __kvm_find_kvm_cpuid_features(vcpu, vcpu->arch.cpuid_entries,
+					     vcpu->arch.cpuid_nent);
 }
 
 void kvm_update_pv_runtime(struct kvm_vcpu *vcpu)
@@ -167,11 +174,12 @@ void kvm_update_pv_runtime(struct kvm_vcpu *vcpu)
 		vcpu->arch.pv_cpuid.features = best->eax;
 }
 
-void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
+static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *entries,
+				       int nent)
 {
 	struct kvm_cpuid_entry2 *best;
 
-	best = kvm_find_cpuid_entry(vcpu, 1, 0);
+	best = cpuid_entry2_find(entries, nent, 1, 0);
 	if (best) {
 		/* Update OSXSAVE bit */
 		if (boot_cpu_has(X86_FEATURE_XSAVE))
@@ -182,33 +190,38 @@ void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
 			   vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE);
 	}
 
-	best = kvm_find_cpuid_entry(vcpu, 7, 0);
+	best = cpuid_entry2_find(entries, nent, 7, 0);
 	if (best && boot_cpu_has(X86_FEATURE_PKU) && best->function == 0x7)
 		cpuid_entry_change(best, X86_FEATURE_OSPKE,
 				   kvm_read_cr4_bits(vcpu, X86_CR4_PKE));
 
-	best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
+	best = cpuid_entry2_find(entries, nent, 0xD, 0);
 	if (best)
 		best->ebx = xstate_required_size(vcpu->arch.xcr0, false);
 
-	best = kvm_find_cpuid_entry(vcpu, 0xD, 1);
+	best = cpuid_entry2_find(entries, nent, 0xD, 1);
 	if (best && (cpuid_entry_has(best, X86_FEATURE_XSAVES) ||
 		     cpuid_entry_has(best, X86_FEATURE_XSAVEC)))
 		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
 
-	best = kvm_find_kvm_cpuid_features(vcpu);
+	best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent);
 	if (kvm_hlt_in_guest(vcpu->kvm) && best &&
 		(best->eax & (1 << KVM_FEATURE_PV_UNHALT)))
 		best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT);
 
 	if (!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT)) {
-		best = kvm_find_cpuid_entry(vcpu, 0x1, 0);
+		best = cpuid_entry2_find(entries, nent, 0x1, 0);
 		if (best)
 			cpuid_entry_change(best, X86_FEATURE_MWAIT,
 					   vcpu->arch.ia32_misc_enable_msr &
 					   MSR_IA32_MISC_ENABLE_MWAIT);
 	}
 }
+
+void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
+{
+	__kvm_update_cpuid_runtime(vcpu, vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent);
+}
 EXPORT_SYMBOL_GPL(kvm_update_cpuid_runtime);
 
 static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
@@ -298,6 +311,8 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
 {
 	int r;
 
+	__kvm_update_cpuid_runtime(vcpu, e2, nent);
+
 	r = kvm_check_cpuid(vcpu, e2, nent);
 	if (r)
 		return r;
@@ -307,7 +322,6 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
 	vcpu->arch.cpuid_nent = nent;
 
 	kvm_update_kvm_cpuid_base(vcpu);
-	kvm_update_cpuid_runtime(vcpu);
 	kvm_vcpu_after_set_cpuid(vcpu);
 
 	return 0;
-- 
2.34.1

[PATCH v2 2/4] KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN

[Vitaly Kuznetsov]

Commit feb627e8d6f6 ("KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN")
forbade changing CPUID altogether but unfortunately this is not fully
compatible with existing VMMs. In particular, QEMU reuses vCPU fds for
CPU hotplug after unplug and it calls KVM_SET_CPUID2. Instead of full ban,
check whether the supplied CPUID data is equal to what was previously set.

Reported-by: Igor Mammedov <imammedo@redhat.com>
Fixes: feb627e8d6f6 ("KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN")
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/cpuid.c | 33 +++++++++++++++++++++++++++++++++
 arch/x86/kvm/x86.c   | 19 -------------------
 2 files changed, 33 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 812190a707f6..c2e9c860fc3f 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -119,6 +119,25 @@ static int kvm_check_cpuid(struct kvm_vcpu *vcpu,
 	return fpu_enable_guest_xfd_features(&vcpu->arch.guest_fpu, xfeatures);
 }
 
+/* Check whether the supplied CPUID data is equal to what is already set for the vCPU. */
+static int kvm_cpuid_check_equal(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
+				 int nent)
+{
+	struct kvm_cpuid_entry2 *best;
+	int i;
+
+	for (i = 0; i < nent; i++) {
+		best = kvm_find_cpuid_entry(vcpu, e2[i].function, e2[i].index);
+		if (!best)
+			return -EINVAL;
+
+		if (e2[i].eax != best->eax || e2[i].ebx != best->ebx ||
+		    e2[i].ecx != best->ecx || e2[i].edx != best->edx)
+			return -EINVAL;
+	}
+
+	return 0;
+}
 static void kvm_update_kvm_cpuid_base(struct kvm_vcpu *vcpu)
 {
 	u32 function;
@@ -313,6 +332,20 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
 
 	__kvm_update_cpuid_runtime(vcpu, e2, nent);
 
+	/*
+	 * KVM does not correctly handle changing guest CPUID after KVM_RUN, as
+	 * MAXPHYADDR, GBPAGES support, AMD reserved bit behavior, etc.. aren't
+	 * tracked in kvm_mmu_page_role.  As a result, KVM may miss guest page
+	 * faults due to reusing SPs/SPTEs. In practice no sane VMM mucks with
+	 * the core vCPU model on the fly. It would've been better to forbid any
+	 * KVM_SET_CPUID{,2} calls after KVM_RUN altogether but unfortunately
+	 * some VMMs (e.g. QEMU) reuse vCPU fds for CPU hotplug/unplug and do
+	 * KVM_SET_CPUID{,2} again. To support this legacy behavior, check
+	 * whether the supplied CPUID data is equal to what's already set.
+	 */
+	if (vcpu->arch.last_vmentry_cpu != -1)
+		return kvm_cpuid_check_equal(vcpu, e2, nent);
+
 	r = kvm_check_cpuid(vcpu, e2, nent);
 	if (r)
 		return r;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 76b4803dd3bd..ff1416010728 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5230,17 +5230,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		struct kvm_cpuid __user *cpuid_arg = argp;
 		struct kvm_cpuid cpuid;
 
-		/*
-		 * KVM does not correctly handle changing guest CPUID after KVM_RUN, as
-		 * MAXPHYADDR, GBPAGES support, AMD reserved bit behavior, etc.. aren't
-		 * tracked in kvm_mmu_page_role.  As a result, KVM may miss guest page
-		 * faults due to reusing SPs/SPTEs.  In practice no sane VMM mucks with
-		 * the core vCPU model on the fly, so fail.
-		 */
-		r = -EINVAL;
-		if (vcpu->arch.last_vmentry_cpu != -1)
-			goto out;
-
 		r = -EFAULT;
 		if (copy_from_user(&cpuid, cpuid_arg, sizeof(cpuid)))
 			goto out;
@@ -5251,14 +5240,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		struct kvm_cpuid2 __user *cpuid_arg = argp;
 		struct kvm_cpuid2 cpuid;
 
-		/*
-		 * KVM_SET_CPUID{,2} after KVM_RUN is forbidded, see the comment in
-		 * KVM_SET_CPUID case above.
-		 */
-		r = -EINVAL;
-		if (vcpu->arch.last_vmentry_cpu != -1)
-			goto out;
-
 		r = -EFAULT;
 		if (copy_from_user(&cpuid, cpuid_arg, sizeof(cpuid)))
 			goto out;
-- 
2.34.1

[PATCH v2 3/4] KVM: selftests: Rename 'get_cpuid_test' to 'cpuid_test'

[Vitaly Kuznetsov]

In preparation to reusing the existing 'get_cpuid_test' for testing
"KVM_SET_CPUID{,2} after KVM_RUN" rename it to 'cpuid_test' to avoid
the confusion.

No functional change intended.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 tools/testing/selftests/kvm/.gitignore                        | 2 +-
 tools/testing/selftests/kvm/Makefile                          | 4 ++--
 .../selftests/kvm/x86_64/{get_cpuid_test.c => cpuid_test.c}   | 0
 3 files changed, 3 insertions(+), 3 deletions(-)
 rename tools/testing/selftests/kvm/x86_64/{get_cpuid_test.c => cpuid_test.c} (100%)

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index 8c129961accf..20b4c921c9a2 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -8,11 +8,11 @@
 /s390x/memop
 /s390x/resets
 /s390x/sync_regs_test
+/x86_64/cpuid_test
 /x86_64/cr4_cpuid_sync_test
 /x86_64/debug_regs
 /x86_64/evmcs_test
 /x86_64/emulator_error_test
-/x86_64/get_cpuid_test
 /x86_64/get_msr_index_features
 /x86_64/kvm_clock_test
 /x86_64/kvm_pv_test
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index ee8cf2149824..ec78a8692899 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -43,11 +43,11 @@ LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handler
 LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
 LIBKVM_riscv = lib/riscv/processor.c lib/riscv/ucall.c
 
-TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
+TEST_GEN_PROGS_x86_64 = x86_64/cpuid_test
+TEST_GEN_PROGS_x86_64 += x86_64/cr4_cpuid_sync_test
 TEST_GEN_PROGS_x86_64 += x86_64/get_msr_index_features
 TEST_GEN_PROGS_x86_64 += x86_64/evmcs_test
 TEST_GEN_PROGS_x86_64 += x86_64/emulator_error_test
-TEST_GEN_PROGS_x86_64 += x86_64/get_cpuid_test
 TEST_GEN_PROGS_x86_64 += x86_64/hyperv_clock
 TEST_GEN_PROGS_x86_64 += x86_64/hyperv_cpuid
 TEST_GEN_PROGS_x86_64 += x86_64/hyperv_features
diff --git a/tools/testing/selftests/kvm/x86_64/get_cpuid_test.c b/tools/testing/selftests/kvm/x86_64/cpuid_test.c
similarity index 100%
rename from tools/testing/selftests/kvm/x86_64/get_cpuid_test.c
rename to tools/testing/selftests/kvm/x86_64/cpuid_test.c
-- 
2.34.1

[PATCH v2 4/4] KVM: selftests: Test KVM_SET_CPUID2 after KVM_RUN

[Vitaly Kuznetsov]

KVM forbids KVM_SET_CPUID2 after KVM_RUN was performed on a vCPU unless
the supplied CPUID data is equal to what was previously set. Test this.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 .../selftests/kvm/include/x86_64/processor.h  |  7 ++++
 .../selftests/kvm/lib/x86_64/processor.c      | 33 ++++++++++++++++---
 .../testing/selftests/kvm/x86_64/cpuid_test.c | 30 +++++++++++++++++
 3 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index e94ba0fc67d8..bb013d101c14 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -375,6 +375,8 @@ uint64_t kvm_get_feature_msr(uint64_t msr_index);
 struct kvm_cpuid2 *kvm_get_supported_cpuid(void);
 
 struct kvm_cpuid2 *vcpu_get_cpuid(struct kvm_vm *vm, uint32_t vcpuid);
+int __vcpu_set_cpuid(struct kvm_vm *vm, uint32_t vcpuid,
+		     struct kvm_cpuid2 *cpuid);
 void vcpu_set_cpuid(struct kvm_vm *vm, uint32_t vcpuid,
 		    struct kvm_cpuid2 *cpuid);
 
@@ -418,6 +420,11 @@ uint64_t vm_get_page_table_entry(struct kvm_vm *vm, int vcpuid, uint64_t vaddr);
 void vm_set_page_table_entry(struct kvm_vm *vm, int vcpuid, uint64_t vaddr,
 			     uint64_t pte);
 
+/*
+ * get_cpuid() - find matching CPUID entry and return pointer to it.
+ */
+struct kvm_cpuid_entry2 *get_cpuid(struct kvm_cpuid2 *cpuid, uint32_t function,
+				   uint32_t index);
 /*
  * set_cpuid() - overwrites a matching cpuid entry with the provided value.
  *		 matches based on ent->function && ent->index. returns true
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index babb0f28575c..d61e2326dc85 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -886,6 +886,17 @@ kvm_get_supported_cpuid_index(uint32_t function, uint32_t index)
 	return entry;
 }
 
+
+int __vcpu_set_cpuid(struct kvm_vm *vm, uint32_t vcpuid,
+		     struct kvm_cpuid2 *cpuid)
+{
+	struct vcpu *vcpu = vcpu_find(vm, vcpuid);
+
+	TEST_ASSERT(vcpu != NULL, "vcpu not found, vcpuid: %u", vcpuid);
+
+	return ioctl(vcpu->fd, KVM_SET_CPUID2, cpuid);
+}
+
 /*
  * VM VCPU CPUID Set
  *
@@ -903,12 +914,9 @@ kvm_get_supported_cpuid_index(uint32_t function, uint32_t index)
 void vcpu_set_cpuid(struct kvm_vm *vm,
 		uint32_t vcpuid, struct kvm_cpuid2 *cpuid)
 {
-	struct vcpu *vcpu = vcpu_find(vm, vcpuid);
 	int rc;
 
-	TEST_ASSERT(vcpu != NULL, "vcpu not found, vcpuid: %u", vcpuid);
-
-	rc = ioctl(vcpu->fd, KVM_SET_CPUID2, cpuid);
+	rc = __vcpu_set_cpuid(vm, vcpuid, cpuid);
 	TEST_ASSERT(rc == 0, "KVM_SET_CPUID2 failed, rc: %i errno: %i",
 		    rc, errno);
 
@@ -1384,6 +1392,23 @@ void assert_on_unhandled_exception(struct kvm_vm *vm, uint32_t vcpuid)
 	}
 }
 
+struct kvm_cpuid_entry2 *get_cpuid(struct kvm_cpuid2 *cpuid, uint32_t function,
+				   uint32_t index)
+{
+	int i;
+
+	for (i = 0; i < cpuid->nent; i++) {
+		struct kvm_cpuid_entry2 *cur = &cpuid->entries[i];
+
+		if (cur->function == function && cur->index == index)
+			return cur;
+	}
+
+	TEST_FAIL("CPUID function 0x%x index 0x%x not found ", function, index);
+
+	return NULL;
+}
+
 bool set_cpuid(struct kvm_cpuid2 *cpuid,
 	       struct kvm_cpuid_entry2 *ent)
 {
diff --git a/tools/testing/selftests/kvm/x86_64/cpuid_test.c b/tools/testing/selftests/kvm/x86_64/cpuid_test.c
index a711f83749ea..16d2465c5634 100644
--- a/tools/testing/selftests/kvm/x86_64/cpuid_test.c
+++ b/tools/testing/selftests/kvm/x86_64/cpuid_test.c
@@ -154,6 +154,34 @@ struct kvm_cpuid2 *vcpu_alloc_cpuid(struct kvm_vm *vm, vm_vaddr_t *p_gva, struct
 	return guest_cpuids;
 }
 
+static void set_cpuid_after_run(struct kvm_vm *vm, struct kvm_cpuid2 *cpuid)
+{
+	struct kvm_cpuid_entry2 *ent;
+	int rc;
+	u32 eax, ebx, x;
+
+	/* Setting unmodified CPUID is allowed */
+	rc = __vcpu_set_cpuid(vm, VCPU_ID, cpuid);
+	TEST_ASSERT(!rc, "Setting unmodified CPUID after KVM_RUN failed: %d", rc);
+
+	/* Changing CPU features is forbidden */
+	ent = get_cpuid(cpuid, 0x7, 0);
+	ebx = ent->ebx;
+	ent->ebx--;
+	rc = __vcpu_set_cpuid(vm, VCPU_ID, cpuid);
+	TEST_ASSERT(rc, "Changing CPU features should fail");
+	ent->ebx = ebx;
+
+	/* Changing MAXPHYADDR is forbidden */
+	ent = get_cpuid(cpuid, 0x80000008, 0);
+	eax = ent->eax;
+	x = eax & 0xff;
+	ent->eax = (eax & ~0xffu) | (x - 1);
+	rc = __vcpu_set_cpuid(vm, VCPU_ID, cpuid);
+	TEST_ASSERT(rc, "Changing MAXPHYADDR should fail");
+	ent->eax = eax;
+}
+
 int main(void)
 {
 	struct kvm_cpuid2 *supp_cpuid, *cpuid2;
@@ -175,5 +203,7 @@ int main(void)
 	for (stage = 0; stage < 3; stage++)
 		run_vcpu(vm, VCPU_ID, stage);
 
+	set_cpuid_after_run(vm, cpuid2);
+
 	kvm_vm_free(vm);
 }
-- 
2.34.1

Re: [PATCH v2 2/4] KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN

[Paolo Bonzini]

On 1/17/22 16:05, Vitaly Kuznetsov wrote:
>   
> +/* Check whether the supplied CPUID data is equal to what is already set for the vCPU. */
> +static int kvm_cpuid_check_equal(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
> +				 int nent)
> +{
> +	struct kvm_cpuid_entry2 *best;
> +	int i;
> +
> +	for (i = 0; i < nent; i++) {
> +		best = kvm_find_cpuid_entry(vcpu, e2[i].function, e2[i].index);
> +		if (!best)
> +			return -EINVAL;
> +
> +		if (e2[i].eax != best->eax || e2[i].ebx != best->ebx ||
> +		    e2[i].ecx != best->ecx || e2[i].edx != best->edx)
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}

What about this alternative implementation:

/* Check whether the supplied CPUID data is equal to what is already set for the vCPU. */
static int kvm_cpuid_check_equal(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
                                  int nent)
{
         struct kvm_cpuid_entry2 *orig;
         int i;

         if (nent != vcpu->arch.cpuid_nent)
                 return -EINVAL;

         for (i = 0; i < nent; i++) {
                 orig = &vcpu->arch.cpuid_entries[i];
                 if (e2[i].function != orig->function ||
                     e2[i].index != orig->index ||
                     e2[i].eax != orig->eax || e2[i].ebx != orig->ebx ||
                     e2[i].ecx != orig->ecx || e2[i].edx != orig->edx)
                         return -EINVAL;
         }

         return 0;
}

avoiding the repeated calls to kvm_find_cpuid_entry?

Paolo

Re: [PATCH v2 2/4] KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN

[Vitaly Kuznetsov]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 1/17/22 16:05, Vitaly Kuznetsov wrote:
>>   
>> +/* Check whether the supplied CPUID data is equal to what is already set for the vCPU. */
>> +static int kvm_cpuid_check_equal(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
>> +				 int nent)
>> +{
>> +	struct kvm_cpuid_entry2 *best;
>> +	int i;
>> +
>> +	for (i = 0; i < nent; i++) {
>> +		best = kvm_find_cpuid_entry(vcpu, e2[i].function, e2[i].index);
>> +		if (!best)
>> +			return -EINVAL;
>> +
>> +		if (e2[i].eax != best->eax || e2[i].ebx != best->ebx ||
>> +		    e2[i].ecx != best->ecx || e2[i].edx != best->edx)
>> +			return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>
> What about this alternative implementation:
>
> /* Check whether the supplied CPUID data is equal to what is already set for the vCPU. */
> static int kvm_cpuid_check_equal(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
>                                   int nent)
> {
>          struct kvm_cpuid_entry2 *orig;
>          int i;
>
>          if (nent != vcpu->arch.cpuid_nent)
>                  return -EINVAL;
>
>          for (i = 0; i < nent; i++) {
>                  orig = &vcpu->arch.cpuid_entries[i];
>                  if (e2[i].function != orig->function ||
>                      e2[i].index != orig->index ||
>                      e2[i].eax != orig->eax || e2[i].ebx != orig->ebx ||
>                      e2[i].ecx != orig->ecx || e2[i].edx != orig->edx)
>                          return -EINVAL;
>          }
>
>          return 0;
> }
>
> avoiding the repeated calls to kvm_find_cpuid_entry?
>

My version is a bit more permissive as it allows supplying CPUID entries
in any order, not necessarily matching the original. I *guess* this
doesn't matter much for the QEMU problem we're trying to workaround,
I'll have to check.

-- 
Vitaly

Re: [PATCH v2 0/4] KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN for CPU hotplug

[Igor Mammedov]

On Mon, 17 Jan 2022 16:05:38 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> Changes since v1:
> - Drop the allowlist of items which were allowed to change and just allow
> the exact same CPUID data [Sean, Paolo]. Adjust selftest accordingly.
> - Drop PATCH1 as the exact same change got merged upstream.
> 
> Recently, KVM made it illegal to change CPUID after KVM_RUN but
> unfortunately this change is not fully compatible with existing VMMs.
> In particular, QEMU reuses vCPU fds for CPU hotplug after unplug and it
> calls KVM_SET_CPUID2. Relax the requirement by implementing an allowing
> KVM_SET_CPUID{,2} with the exact same data.


Can you check following scenario:
 * on host that has IA32_TSX_CTRL and TSX enabled (RTM/HLE cpuid bits present)
 * boot 2 vcpus VM with TSX enabled on VMM side but with tsx=off on kernel CLI

     that should cause kernel to set MSR_IA32_TSX_CTRL to 3H from initial 0H
     and clear RTM+HLE bits in CPUID, check that RTM/HLE cpuid it cleared

 * hotunplug a VCPU and then replug it again
    if IA32_TSX_CTRL is reset to initial state, that should re-enable
    RTM/HLE cpuid bits and KVM_SET_CPUID2 might fail due to difference

and as Sean pointed out there might be other non constant leafs,
where exact match check could leave userspace broken.
     

> Vitaly Kuznetsov (4):
>   KVM: x86: Do runtime CPUID update before updating
>     vcpu->arch.cpuid_entries
>   KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN
>   KVM: selftests: Rename 'get_cpuid_test' to 'cpuid_test'
>   KVM: selftests: Test KVM_SET_CPUID2 after KVM_RUN
> 
>  arch/x86/kvm/cpuid.c                          | 67 ++++++++++++++++---
>  arch/x86/kvm/x86.c                            | 19 ------
>  tools/testing/selftests/kvm/.gitignore        |  2 +-
>  tools/testing/selftests/kvm/Makefile          |  4 +-
>  .../selftests/kvm/include/x86_64/processor.h  |  7 ++
>  .../selftests/kvm/lib/x86_64/processor.c      | 33 +++++++--
>  .../x86_64/{get_cpuid_test.c => cpuid_test.c} | 30 +++++++++
>  7 files changed, 126 insertions(+), 36 deletions(-)
>  rename tools/testing/selftests/kvm/x86_64/{get_cpuid_test.c => cpuid_test.c} (83%)
> 

Re: [PATCH v2 2/4] KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN

[Vitaly Kuznetsov]

Vitaly Kuznetsov <vkuznets@redhat.com> writes:

> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> On 1/17/22 16:05, Vitaly Kuznetsov wrote:
>>>   
>>> +/* Check whether the supplied CPUID data is equal to what is already set for the vCPU. */
>>> +static int kvm_cpuid_check_equal(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
>>> +				 int nent)
>>> +{
>>> +	struct kvm_cpuid_entry2 *best;
>>> +	int i;
>>> +
>>> +	for (i = 0; i < nent; i++) {
>>> +		best = kvm_find_cpuid_entry(vcpu, e2[i].function, e2[i].index);
>>> +		if (!best)
>>> +			return -EINVAL;
>>> +
>>> +		if (e2[i].eax != best->eax || e2[i].ebx != best->ebx ||
>>> +		    e2[i].ecx != best->ecx || e2[i].edx != best->edx)
>>> +			return -EINVAL;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>
>> What about this alternative implementation:
>>
...
>>
>> avoiding the repeated calls to kvm_find_cpuid_entry?
>>
>
> My version is a bit more permissive as it allows supplying CPUID entries
> in any order, not necessarily matching the original. I *guess* this
> doesn't matter much for the QEMU problem we're trying to workaround,
> I'll have to check.

I tried this with QEMU and nothing blew up, during CPU hotplug entries
come in the same order as the original. v3 which I've just sent
implements this suggestion.

-- 
Vitaly

Re: [PATCH v2 0/4] KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN for CPU hotplug

[Paolo Bonzini]

On 1/18/22 15:35, Igor Mammedov wrote:
> Can you check following scenario:
>   * on host that has IA32_TSX_CTRL and TSX enabled (RTM/HLE cpuid bits present)
>   * boot 2 vcpus VM with TSX enabled on VMM side but with tsx=off on kernel CLI
> 
>       that should cause kernel to set MSR_IA32_TSX_CTRL to 3H from initial 0H
>       and clear RTM+HLE bits in CPUID, check that RTM/HLE cpuid it cleared
> 
>   * hotunplug a VCPU and then replug it again
>      if IA32_TSX_CTRL is reset to initial state, that should re-enable
>      RTM/HLE cpuid bits and KVM_SET_CPUID2 might fail due to difference
> 
> and as Sean pointed out there might be other non constant leafs,
> where exact match check could leave userspace broken.


MSR_IA32_TSX_CTRL is handled differently straight during the CPUID call:

                 if (function == 7 && index == 0) {
                         u64 data;
                         if (!__kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data, true) &&
                             (data & TSX_CTRL_CPUID_CLEAR))
                                 *ebx &= ~(F(RTM) | F(HLE));
                 }


and I think we should redo all or most of kvm_update_cpuid_runtime
the same way.

Paolo

Re: [PATCH v2 0/4] KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN for CPU hotplug

[Vitaly Kuznetsov]

Igor Mammedov <imammedo@redhat.com> writes:

> On Mon, 17 Jan 2022 16:05:38 +0100
> Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
>> Changes since v1:
>> - Drop the allowlist of items which were allowed to change and just allow
>> the exact same CPUID data [Sean, Paolo]. Adjust selftest accordingly.
>> - Drop PATCH1 as the exact same change got merged upstream.
>> 
>> Recently, KVM made it illegal to change CPUID after KVM_RUN but
>> unfortunately this change is not fully compatible with existing VMMs.
>> In particular, QEMU reuses vCPU fds for CPU hotplug after unplug and it
>> calls KVM_SET_CPUID2. Relax the requirement by implementing an allowing
>> KVM_SET_CPUID{,2} with the exact same data.
>
>
> Can you check following scenario:
>  * on host that has IA32_TSX_CTRL and TSX enabled (RTM/HLE cpuid bits present)
>  * boot 2 vcpus VM with TSX enabled on VMM side but with tsx=off on kernel CLI
>
>      that should cause kernel to set MSR_IA32_TSX_CTRL to 3H from initial 0H
>      and clear RTM+HLE bits in CPUID, check that RTM/HLE cpuid it
>      cleared

Forgive me my ignorance around (not only) TSX :-) I took a "Intel(R)
Xeon(R) CPU E3-1270 v5 @ 3.60GHz" host which seems to have rtm/hle and
booted a guest with 'cpu=host' and with (and without) 'tsx=off' on the
kernel command line. I decided to check what's is MSR_IA32_TSX_CTRL but
I see the following:

# rdmsr 0x122
rdmsr: CPU 0 cannot read MSR 0x00000122

I tried adding 'tsx_ctrl' to my QEMU command line but it complains with
qemu-system-x86_64: warning: host doesn't support requested feature: MSR(10AH).tsx-ctrl [bit 7]

so I think my host is not good enough :-(

Also, I've looked at tsx_clear_cpuid() but it actually writes to
MSR_TSX_FORCE_ABORT MSR (0x10F), not MSR_IA32_TSX_CTRL so I'm confused.

>
>  * hotunplug a VCPU and then replug it again
>     if IA32_TSX_CTRL is reset to initial state, that should re-enable
>     RTM/HLE cpuid bits and KVM_SET_CPUID2 might fail due to difference

Could you please teach me this kung-fu, I mean hot to unplug a
cold-plugged CPU with QMP? Previoulsy, I only did un-plugging for what
I've hotplugged, something like:

(QEMU) device_add driver=host-x86_64-cpu socket-id=0 core-id=2 thread-id=0 id=cpu2
{"return": {}}
(QEMU) device_del id=cpu2
{"return": {}}

What's the ids of the cold-plugged CPUs?

>
> and as Sean pointed out there might be other non constant leafs,
> where exact match check could leave userspace broken.

Indeed, while testing your suggestion I've stumbled upon
CPUID.(EAX=0x12, ECX=1) (SGX) where we mangle ECX from
kvm_vcpu_after_set_cpuid():

        best = kvm_find_cpuid_entry(vcpu, 0x12, 0x1);
	if (best) {
                best->ecx &= vcpu->arch.guest_supported_xcr0 & 0xffffffff;
		best->edx &= vcpu->arch.guest_supported_xcr0 >> 32;
                best->ecx |= XFEATURE_MASK_FPSSE;
        }

In theory, we should just move this to __kvm_update_cpuid_runtime()...
I'll take a look tomorrow.

-- 
Vitaly

Re: [PATCH v2 0/4] KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN for CPU hotplug

[Sean Christopherson]

On Tue, Jan 18, 2022, Paolo Bonzini wrote:
> On 1/18/22 15:35, Igor Mammedov wrote:
> > Can you check following scenario:
> >   * on host that has IA32_TSX_CTRL and TSX enabled (RTM/HLE cpuid bits present)
> >   * boot 2 vcpus VM with TSX enabled on VMM side but with tsx=off on kernel CLI
> > 
> >       that should cause kernel to set MSR_IA32_TSX_CTRL to 3H from initial 0H
> >       and clear RTM+HLE bits in CPUID, check that RTM/HLE cpuid it cleared
> > 
> >   * hotunplug a VCPU and then replug it again
> >      if IA32_TSX_CTRL is reset to initial state, that should re-enable
> >      RTM/HLE cpuid bits and KVM_SET_CPUID2 might fail due to difference
> > 
> > and as Sean pointed out there might be other non constant leafs,
> > where exact match check could leave userspace broken.
> 
> 
> MSR_IA32_TSX_CTRL is handled differently straight during the CPUID call:
> 
>                 if (function == 7 && index == 0) {
>                         u64 data;
>                         if (!__kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data, true) &&
>                             (data & TSX_CTRL_CPUID_CLEAR))
>                                 *ebx &= ~(F(RTM) | F(HLE));
>                 }
> 
> 
> and I think we should redo all or most of kvm_update_cpuid_runtime
> the same way.

Please no.  xstate_required_size() requires multiple host CPUID calls, and glibc
does CPUID.0xD.0x0 and CPUID.0xD.0x1 as part of its initialization, i.e. launching
a new userspace process in the guest will see additional performance overhread due
to KVM dynamically computing the XSAVE size instead of caching it based on vCPU
state.  Nested virtualization would be especially painful as every one of those
"host" CPUID invocations will trigger and exit from L1=>L0.

Re: [PATCH v2 0/4] KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN for CPU hotplug

[Paolo Bonzini]

On 1/18/22 17:53, Sean Christopherson wrote:
>>
>> and I think we should redo all or most of kvm_update_cpuid_runtime
>> the same way.
> Please no.  xstate_required_size() requires multiple host CPUID calls, and glibc
> does CPUID.0xD.0x0 and CPUID.0xD.0x1 as part of its initialization, i.e. launching
> a new userspace process in the guest will see additional performance overhread due
> to KVM dynamically computing the XSAVE size instead of caching it based on vCPU
> state.  Nested virtualization would be especially painful as every one of those
> "host" CPUID invocations will trigger and exit from L1=>L0.
> 

Agreed, but all of the required data is by Linux cached in 
xstate_offsets, xstate_sizes and xstate_comp_offsets; moving 
xstate_required_size to xstate.c and skipping the host CPUID would 
probably be a good idea nevertheless.

Paolo

Re: [PATCH v2 0/4] KVM: x86: Partially allow KVM_SET_CPUID{,2} after KVM_RUN for CPU hotplug

[Igor Mammedov]

On Tue, 18 Jan 2022 17:34:09 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > On Mon, 17 Jan 2022 16:05:38 +0100
> > Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> >  
> >> Changes since v1:
> >> - Drop the allowlist of items which were allowed to change and just allow
> >> the exact same CPUID data [Sean, Paolo]. Adjust selftest accordingly.
> >> - Drop PATCH1 as the exact same change got merged upstream.
> >> 
> >> Recently, KVM made it illegal to change CPUID after KVM_RUN but
> >> unfortunately this change is not fully compatible with existing VMMs.
> >> In particular, QEMU reuses vCPU fds for CPU hotplug after unplug and it
> >> calls KVM_SET_CPUID2. Relax the requirement by implementing an allowing
> >> KVM_SET_CPUID{,2} with the exact same data.  
> >
> >
> > Can you check following scenario:
> >  * on host that has IA32_TSX_CTRL and TSX enabled (RTM/HLE cpuid bits present)
> >  * boot 2 vcpus VM with TSX enabled on VMM side but with tsx=off on kernel CLI
> >
> >      that should cause kernel to set MSR_IA32_TSX_CTRL to 3H from initial 0H
> >      and clear RTM+HLE bits in CPUID, check that RTM/HLE cpuid it
> >      cleared  
> 
> Forgive me my ignorance around (not only) TSX :-) I took a "Intel(R)
> Xeon(R) CPU E3-1270 v5 @ 3.60GHz" host which seems to have rtm/hle and
> booted a guest with 'cpu=host' and with (and without) 'tsx=off' on the
> kernel command line. I decided to check what's is MSR_IA32_TSX_CTRL but
> I see the following:
> 
> # rdmsr 0x122
> rdmsr: CPU 0 cannot read MSR 0x00000122
> 
> I tried adding 'tsx_ctrl' to my QEMU command line but it complains with
> qemu-system-x86_64: warning: host doesn't support requested feature: MSR(10AH).tsx-ctrl [bit 7]
> 
> so I think my host is not good enough :-(

I've seen it being available on "COOPER LAKE" Xeon

> 
> Also, I've looked at tsx_clear_cpuid() but it actually writes to
> MSR_TSX_FORCE_ABORT MSR (0x10F), not MSR_IA32_TSX_CTRL so I'm confused.

look at tsx_disable()

> >  * hotunplug a VCPU and then replug it again
> >     if IA32_TSX_CTRL is reset to initial state, that should re-enable
> >     RTM/HLE cpuid bits and KVM_SET_CPUID2 might fail due to difference  
> 
> Could you please teach me this kung-fu, I mean hot to unplug a
> cold-plugged CPU with QMP? Previoulsy, I only did un-plugging for what
> I've hotplugged, something like:
> 
> (QEMU) device_add driver=host-x86_64-cpu socket-id=0 core-id=2 thread-id=0 id=cpu2
> {"return": {}}
> (QEMU) device_del id=cpu2
> {"return": {}}
> 
> What's the ids of the cold-plugged CPUs?

it doesn't have to be coldplugged, hot(plug/unplug/plug) sequence is fine as well.
fyi you can use qom_path with device _del from 'info hotpluggable-cpus' output


> > and as Sean pointed out there might be other non constant leafs,
> > where exact match check could leave userspace broken.  
> 
> Indeed, while testing your suggestion I've stumbled upon
> CPUID.(EAX=0x12, ECX=1) (SGX) where we mangle ECX from
> kvm_vcpu_after_set_cpuid():
> 
>         best = kvm_find_cpuid_entry(vcpu, 0x12, 0x1);
> 	if (best) {
>                 best->ecx &= vcpu->arch.guest_supported_xcr0 & 0xffffffff;
> 		best->edx &= vcpu->arch.guest_supported_xcr0 >> 32;
>                 best->ecx |= XFEATURE_MASK_FPSSE;
>         }
> 
> In theory, we should just move this to __kvm_update_cpuid_runtime()...
> I'll take a look tomorrow.
>