[PATCH v2 0/7] Refactor handling flow of SET_CPUID*

[PATCH v2 0/7] Refactor handling flow of SET_CPUID*

[Xiaoyao Li]

This serial is the extended version of
https://lkml.kernel.org/r/20200528151927.14346-1-xiaoyao.li@intel.com

First two patches are bug fixing, and the others aim to refactor the flow
of SET_CPUID* as:

1. cpuid check: check if userspace provides legal CPUID settings;

2. cpuid update: Update some special CPUID bits based on current vcpu
                 state, e.g., OSXSAVE, OSPKE, ...

3. update vcpu model: Update vcpu model (settings) based on the final CPUID
                      settings. 


v2:
 - rebase to kvm/queue: a037ff353ba6 ("Merge branch 'kvm-master' into HEAD")
 - change the name of kvm_update_state_based_on_cpuid() to
   kvm_update_vcpu_model() [Sean]
 - Add patch 5 to rename kvm_x86_ops.cpuid_date() to
   kvm_x86_ops.update_vcpu_model()

v1:
https://lkml.kernel.org/r/20200529085545.29242-1-xiaoyao.li@intel.com


Xiaoyao Li (7):
  KVM: X86: Reset vcpu->arch.cpuid_nent to 0 if SET_CPUID fails
  KVM: X86: Go on updating other CPUID leaves when leaf 1 is absent
  KVM: X86: Introduce kvm_check_cpuid()
  KVM: X86: Split kvm_update_cpuid()
  KVM: X86: Rename cpuid_update() to update_vcpu_model()
  KVM: X86: Move kvm_x86_ops.update_vcpu_model() into
    kvm_update_vcpu_model()
  KVM: X86: Move kvm_apic_set_version() to kvm_update_vcpu_model()

 arch/x86/include/asm/kvm_host.h |   2 +-
 arch/x86/kvm/cpuid.c            | 108 ++++++++++++++++++++------------
 arch/x86/kvm/cpuid.h            |   3 +-
 arch/x86/kvm/svm/svm.c          |   4 +-
 arch/x86/kvm/vmx/nested.c       |   2 +-
 arch/x86/kvm/vmx/vmx.c          |   4 +-
 arch/x86/kvm/x86.c              |   1 +
 7 files changed, 77 insertions(+), 47 deletions(-)

-- 
2.18.2

[PATCH v2 1/7] KVM: X86: Reset vcpu->arch.cpuid_nent to 0 if SET_CPUID fails

[Xiaoyao Li]

It needs to invalidate CPUID configruations if usersapce provides
illegal input.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/kvm/cpuid.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 8a294f9747aa..1d13bad42bf9 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -207,6 +207,8 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 	kvm_apic_set_version(vcpu);
 	kvm_x86_ops.cpuid_update(vcpu);
 	r = kvm_update_cpuid(vcpu);
+	if (r)
+		vcpu->arch.cpuid_nent = 0;
 
 	kvfree(cpuid_entries);
 out:
@@ -230,6 +232,8 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
 	kvm_apic_set_version(vcpu);
 	kvm_x86_ops.cpuid_update(vcpu);
 	r = kvm_update_cpuid(vcpu);
+	if (r)
+		vcpu->arch.cpuid_nent = 0;
 out:
 	return r;
 }
-- 
2.18.2

[PATCH v2 2/7] KVM: X86: Go on updating other CPUID leaves when leaf 1 is absent

[Xiaoyao Li]

As handling of bits other leaf 1 added over time, kvm_update_cpuid()
should not return directly if leaf 1 is absent, but should go on
updateing other CPUID leaves.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/kvm/cpuid.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 1d13bad42bf9..0164dac95ef5 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -60,22 +60,21 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
 	best = kvm_find_cpuid_entry(vcpu, 1, 0);
-	if (!best)
-		return 0;
-
-	/* Update OSXSAVE bit */
-	if (boot_cpu_has(X86_FEATURE_XSAVE) && best->function == 0x1)
-		cpuid_entry_change(best, X86_FEATURE_OSXSAVE,
+	if (best) {
+		/* Update OSXSAVE bit */
+		if (boot_cpu_has(X86_FEATURE_XSAVE))
+			cpuid_entry_change(best, X86_FEATURE_OSXSAVE,
 				   kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE));
 
-	cpuid_entry_change(best, X86_FEATURE_APIC,
+		cpuid_entry_change(best, X86_FEATURE_APIC,
 			   vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE);
 
-	if (apic) {
-		if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
-			apic->lapic_timer.timer_mode_mask = 3 << 17;
-		else
-			apic->lapic_timer.timer_mode_mask = 1 << 17;
+		if (apic) {
+			if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
+				apic->lapic_timer.timer_mode_mask = 3 << 17;
+			else
+				apic->lapic_timer.timer_mode_mask = 1 << 17;
+		}
 	}
 
 	best = kvm_find_cpuid_entry(vcpu, 7, 0);
-- 
2.18.2

[PATCH v2 3/7] KVM: X86: Introduce kvm_check_cpuid()

[Xiaoyao Li]

Use kvm_check_cpuid() to validate if userspace provides legal cpuid
settings and call it before KVM updates CPUID.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
Is the check of virutal address width really necessary?
KVM doesn't check other bits at all. I guess the policy is that KVM allows
illegal CPUID settings as long as it doesn't break host kernel. Userspace
takes the consequences itself if it sets bogus CPUID settings that breaks
its guest.
But why vaddr_bits is special? It seems illegal vaddr_bits won't break host
kernel.
---
 arch/x86/kvm/cpuid.c | 54 ++++++++++++++++++++++++++++----------------
 arch/x86/kvm/cpuid.h |  2 +-
 2 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 0164dac95ef5..67e5c68fdb45 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -54,7 +54,26 @@ static u32 xstate_required_size(u64 xstate_bv, bool compacted)
 
 #define F feature_bit
 
-int kvm_update_cpuid(struct kvm_vcpu *vcpu)
+static int kvm_check_cpuid(struct kvm_vcpu *vcpu)
+{
+	struct kvm_cpuid_entry2 *best;
+
+	/*
+	 * The existing code assumes virtual address is 48-bit or 57-bit in the
+	 * canonical address checks; exit if it is ever changed.
+	 */
+	best = kvm_find_cpuid_entry(vcpu, 0x80000008, 0);
+	if (best) {
+		int vaddr_bits = (best->eax & 0xff00) >> 8;
+
+		if (vaddr_bits != 48 && vaddr_bits != 57 && vaddr_bits != 0)
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
+void kvm_update_cpuid(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpuid_entry2 *best;
 	struct kvm_lapic *apic = vcpu->arch.apic;
@@ -96,18 +115,6 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
 		     cpuid_entry_has(best, X86_FEATURE_XSAVEC)))
 		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
 
-	/*
-	 * The existing code assumes virtual address is 48-bit or 57-bit in the
-	 * canonical address checks; exit if it is ever changed.
-	 */
-	best = kvm_find_cpuid_entry(vcpu, 0x80000008, 0);
-	if (best) {
-		int vaddr_bits = (best->eax & 0xff00) >> 8;
-
-		if (vaddr_bits != 48 && vaddr_bits != 57 && vaddr_bits != 0)
-			return -EINVAL;
-	}
-
 	best = kvm_find_cpuid_entry(vcpu, KVM_CPUID_FEATURES, 0);
 	if (kvm_hlt_in_guest(vcpu->kvm) && best &&
 		(best->eax & (1 << KVM_FEATURE_PV_UNHALT)))
@@ -127,7 +134,6 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
 	kvm_mmu_reset_context(vcpu);
 
 	kvm_pmu_refresh(vcpu);
-	return 0;
 }
 
 static int is_efer_nx(void)
@@ -202,12 +208,16 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 		vcpu->arch.cpuid_entries[i].padding[2] = 0;
 	}
 	vcpu->arch.cpuid_nent = cpuid->nent;
+	r = kvm_check_cpuid(vcpu);
+	if (r) {
+		vcpu->arch.cpuid_nent = 0;
+		goto out;
+	}
+
 	cpuid_fix_nx_cap(vcpu);
 	kvm_apic_set_version(vcpu);
 	kvm_x86_ops.cpuid_update(vcpu);
-	r = kvm_update_cpuid(vcpu);
-	if (r)
-		vcpu->arch.cpuid_nent = 0;
+	kvm_update_cpuid(vcpu);
 
 	kvfree(cpuid_entries);
 out:
@@ -228,11 +238,15 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
 			   cpuid->nent * sizeof(struct kvm_cpuid_entry2)))
 		goto out;
 	vcpu->arch.cpuid_nent = cpuid->nent;
+	r = kvm_check_cpuid(vcpu);
+	if (r) {
+		vcpu->arch.cpuid_nent = 0;
+		goto out;
+	}
+
 	kvm_apic_set_version(vcpu);
 	kvm_x86_ops.cpuid_update(vcpu);
-	r = kvm_update_cpuid(vcpu);
-	if (r)
-		vcpu->arch.cpuid_nent = 0;
+	kvm_update_cpuid(vcpu);
 out:
 	return r;
 }
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 05434cd9342f..f136de1debad 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -9,7 +9,7 @@
 extern u32 kvm_cpu_caps[NCAPINTS] __read_mostly;
 void kvm_set_cpu_caps(void);
 
-int kvm_update_cpuid(struct kvm_vcpu *vcpu);
+void kvm_update_cpuid(struct kvm_vcpu *vcpu);
 struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
 					      u32 function, u32 index);
 int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
-- 
2.18.2

[PATCH v2 4/7] KVM: X86: Split kvm_update_cpuid()

[Xiaoyao Li]

Split the part of updating vcpu model out of kvm_update_cpuid(), and put
it into a new kvm_update_vcpu_model(). So it's more clear that
kvm_update_cpuid() is to update guest CPUID settings, while
kvm_update_vcpu_model() is to update vcpu model (settings) based on the
updated CPUID settings.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/kvm/cpuid.c | 38 ++++++++++++++++++++++++--------------
 arch/x86/kvm/cpuid.h |  1 +
 arch/x86/kvm/x86.c   |  1 +
 3 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 67e5c68fdb45..001f5a94880e 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -76,7 +76,6 @@ static int kvm_check_cpuid(struct kvm_vcpu *vcpu)
 void kvm_update_cpuid(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpuid_entry2 *best;
-	struct kvm_lapic *apic = vcpu->arch.apic;
 
 	best = kvm_find_cpuid_entry(vcpu, 1, 0);
 	if (best) {
@@ -87,13 +86,6 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
 
 		cpuid_entry_change(best, X86_FEATURE_APIC,
 			   vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE);
-
-		if (apic) {
-			if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
-				apic->lapic_timer.timer_mode_mask = 3 << 17;
-			else
-				apic->lapic_timer.timer_mode_mask = 1 << 17;
-		}
 	}
 
 	best = kvm_find_cpuid_entry(vcpu, 7, 0);
@@ -102,13 +94,8 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
 				   kvm_read_cr4_bits(vcpu, X86_CR4_PKE));
 
 	best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
-	if (!best) {
-		vcpu->arch.guest_supported_xcr0 = 0;
-	} else {
-		vcpu->arch.guest_supported_xcr0 =
-			(best->eax | ((u64)best->edx << 32)) & supported_xcr0;
+	if (best)
 		best->ebx = xstate_required_size(vcpu->arch.xcr0, false);
-	}
 
 	best = kvm_find_cpuid_entry(vcpu, 0xD, 1);
 	if (best && (cpuid_entry_has(best, X86_FEATURE_XSAVES) ||
@@ -127,6 +114,27 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
 					   vcpu->arch.ia32_misc_enable_msr &
 					   MSR_IA32_MISC_ENABLE_MWAIT);
 	}
+}
+
+void kvm_update_vcpu_model(struct kvm_vcpu *vcpu)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+	struct kvm_cpuid_entry2 *best;
+
+	best = kvm_find_cpuid_entry(vcpu, 1, 0);
+	if (best && apic) {
+		if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
+			apic->lapic_timer.timer_mode_mask = 3 << 17;
+		else
+			apic->lapic_timer.timer_mode_mask = 1 << 17;
+	}
+
+	best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
+	if (!best)
+		vcpu->arch.guest_supported_xcr0 = 0;
+	else
+		vcpu->arch.guest_supported_xcr0 =
+			(best->eax | ((u64)best->edx << 32)) & supported_xcr0;
 
 	/* Note, maxphyaddr must be updated before tdp_level. */
 	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
@@ -218,6 +226,7 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 	kvm_apic_set_version(vcpu);
 	kvm_x86_ops.cpuid_update(vcpu);
 	kvm_update_cpuid(vcpu);
+	kvm_update_vcpu_model(vcpu);
 
 	kvfree(cpuid_entries);
 out:
@@ -247,6 +256,7 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
 	kvm_apic_set_version(vcpu);
 	kvm_x86_ops.cpuid_update(vcpu);
 	kvm_update_cpuid(vcpu);
+	kvm_update_vcpu_model(vcpu);
 out:
 	return r;
 }
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index f136de1debad..45e3643e2fba 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -10,6 +10,7 @@ extern u32 kvm_cpu_caps[NCAPINTS] __read_mostly;
 void kvm_set_cpu_caps(void);
 
 void kvm_update_cpuid(struct kvm_vcpu *vcpu);
+void kvm_update_vcpu_model(struct kvm_vcpu *vcpu);
 struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
 					      u32 function, u32 index);
 int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 00c88c2f34e4..4dee28bbc087 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8149,6 +8149,7 @@ static void enter_smm(struct kvm_vcpu *vcpu)
 #endif
 
 	kvm_update_cpuid(vcpu);
+	kvm_update_vcpu_model(vcpu);
 	kvm_mmu_reset_context(vcpu);
 }
 
-- 
2.18.2

[PATCH v2 5/7] KVM: X86: Rename cpuid_update() to update_vcpu_model()

[Xiaoyao Li]

The name of callback cpuid_update() is misleading that it's not about
updating CPUID settings of vcpu but updating the configurations of vcpu
based on the CPUIDs. So rename it to update_vcpu_model().

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/include/asm/kvm_host.h | 2 +-
 arch/x86/kvm/cpuid.c            | 4 ++--
 arch/x86/kvm/svm/svm.c          | 4 ++--
 arch/x86/kvm/vmx/nested.c       | 2 +-
 arch/x86/kvm/vmx/vmx.c          | 4 ++--
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index f852ee350beb..e8ae89eef199 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1083,7 +1083,7 @@ struct kvm_x86_ops {
 	void (*hardware_unsetup)(void);
 	bool (*cpu_has_accelerated_tpr)(void);
 	bool (*has_emulated_msr)(u32 index);
-	void (*cpuid_update)(struct kvm_vcpu *vcpu);
+	void (*update_vcpu_model)(struct kvm_vcpu *vcpu);
 
 	unsigned int vm_size;
 	int (*vm_init)(struct kvm *kvm);
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 001f5a94880e..d2f93823f9fd 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -224,7 +224,7 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 
 	cpuid_fix_nx_cap(vcpu);
 	kvm_apic_set_version(vcpu);
-	kvm_x86_ops.cpuid_update(vcpu);
+	kvm_x86_ops.update_vcpu_model(vcpu);
 	kvm_update_cpuid(vcpu);
 	kvm_update_vcpu_model(vcpu);
 
@@ -254,7 +254,7 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
 	}
 
 	kvm_apic_set_version(vcpu);
-	kvm_x86_ops.cpuid_update(vcpu);
+	kvm_x86_ops.update_vcpu_model(vcpu);
 	kvm_update_cpuid(vcpu);
 	kvm_update_vcpu_model(vcpu);
 out:
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index c0da4dd78ac5..480d0354910a 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3551,7 +3551,7 @@ static u64 svm_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 	return 0;
 }
 
-static void svm_cpuid_update(struct kvm_vcpu *vcpu)
+static void svm_update_vcpu_model(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
@@ -4051,7 +4051,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
 
 	.get_exit_info = svm_get_exit_info,
 
-	.cpuid_update = svm_cpuid_update,
+	.update_vcpu_model = svm_update_vcpu_model,
 
 	.has_wbinvd_exit = svm_has_wbinvd_exit,
 
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index d1af20b050a8..86ba7cd49c97 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -6322,7 +6322,7 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps)
 
 	/*
 	 * secondary cpu-based controls.  Do not include those that
-	 * depend on CPUID bits, they are added later by vmx_cpuid_update.
+	 * depend on CPUID bits, they are added later by vmx_update_vcpu_model.
 	 */
 	if (msrs->procbased_ctls_high & CPU_BASED_ACTIVATE_SECONDARY_CONTROLS)
 		rdmsr(MSR_IA32_VMX_PROCBASED_CTLS2,
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index b1a23ad986ff..147c696d6445 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7251,7 +7251,7 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)
 		vmx->pt_desc.ctl_bitmask &= ~(0xfULL << (32 + i * 4));
 }
 
-static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
+static void vmx_update_vcpu_model(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 
@@ -7945,7 +7945,7 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = {
 
 	.get_exit_info = vmx_get_exit_info,
 
-	.cpuid_update = vmx_cpuid_update,
+	.update_vcpu_model = vmx_update_vcpu_model,
 
 	.has_wbinvd_exit = cpu_has_vmx_wbinvd_exit,
 
-- 
2.18.2

[PATCH v2 6/7] KVM: X86: Move kvm_x86_ops.update_vcpu_model() into kvm_update_vcpu_model()

[Xiaoyao Li]

kvm_x86_ops.update_vcpu_model() is used to update vmx/svm vcpu settings
based on updated CPUID settings. So it's supposed to be called after
CPUIDs are fully updated, i.e., kvm_update_cpuid().

Move it in kvm_update_vcpu_model().

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
---
 arch/x86/kvm/cpuid.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index d2f93823f9fd..5decc2dd5448 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -121,6 +121,8 @@ void kvm_update_vcpu_model(struct kvm_vcpu *vcpu)
 	struct kvm_lapic *apic = vcpu->arch.apic;
 	struct kvm_cpuid_entry2 *best;
 
+	kvm_x86_ops.update_vcpu_model(vcpu);
+
 	best = kvm_find_cpuid_entry(vcpu, 1, 0);
 	if (best && apic) {
 		if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
@@ -136,6 +138,7 @@ void kvm_update_vcpu_model(struct kvm_vcpu *vcpu)
 		vcpu->arch.guest_supported_xcr0 =
 			(best->eax | ((u64)best->edx << 32)) & supported_xcr0;
 
+
 	/* Note, maxphyaddr must be updated before tdp_level. */
 	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
 	vcpu->arch.tdp_level = kvm_x86_ops.get_tdp_level(vcpu);
@@ -224,7 +227,6 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 
 	cpuid_fix_nx_cap(vcpu);
 	kvm_apic_set_version(vcpu);
-	kvm_x86_ops.update_vcpu_model(vcpu);
 	kvm_update_cpuid(vcpu);
 	kvm_update_vcpu_model(vcpu);
 
@@ -254,7 +256,6 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
 	}
 
 	kvm_apic_set_version(vcpu);
-	kvm_x86_ops.update_vcpu_model(vcpu);
 	kvm_update_cpuid(vcpu);
 	kvm_update_vcpu_model(vcpu);
 out:
-- 
2.18.2

[PATCH v2 7/7] KVM: X86: Move kvm_apic_set_version() to kvm_update_vcpu_model()

[Xiaoyao Li]

Obviously, kvm_apic_set_version() fits well in kvm_update_vcpu_model().

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/kvm/cpuid.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 5decc2dd5448..3428f4d84b42 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -129,6 +129,8 @@ void kvm_update_vcpu_model(struct kvm_vcpu *vcpu)
 			apic->lapic_timer.timer_mode_mask = 3 << 17;
 		else
 			apic->lapic_timer.timer_mode_mask = 1 << 17;
+
+		kvm_apic_set_version(vcpu);
 	}
 
 	best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
@@ -226,7 +228,6 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 	}
 
 	cpuid_fix_nx_cap(vcpu);
-	kvm_apic_set_version(vcpu);
 	kvm_update_cpuid(vcpu);
 	kvm_update_vcpu_model(vcpu);
 
@@ -255,7 +256,6 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
 		goto out;
 	}
 
-	kvm_apic_set_version(vcpu);
 	kvm_update_cpuid(vcpu);
 	kvm_update_vcpu_model(vcpu);
 out:
-- 
2.18.2

Re: [PATCH v2 1/7] KVM: X86: Reset vcpu->arch.cpuid_nent to 0 if SET_CPUID fails

[Jim Mattson]

On Tue, Jun 23, 2020 at 4:58 AM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>
> It needs to invalidate CPUID configruations if usersapce provides

Nits: configurations, userspace

> illegal input.
>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  arch/x86/kvm/cpuid.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 8a294f9747aa..1d13bad42bf9 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -207,6 +207,8 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>         kvm_apic_set_version(vcpu);
>         kvm_x86_ops.cpuid_update(vcpu);
>         r = kvm_update_cpuid(vcpu);
> +       if (r)
> +               vcpu->arch.cpuid_nent = 0;
>
>         kvfree(cpuid_entries);
>  out:
> @@ -230,6 +232,8 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
>         kvm_apic_set_version(vcpu);
>         kvm_x86_ops.cpuid_update(vcpu);
>         r = kvm_update_cpuid(vcpu);
> +       if (r)
> +               vcpu->arch.cpuid_nent = 0;
>  out:
>         return r;
>  }
> --
> 2.18.2

What if vcpu->arch.cpuid_nent was greater than 0 before the ioctl in question?

Re: [PATCH v2 1/7] KVM: X86: Reset vcpu->arch.cpuid_nent to 0 if SET_CPUID fails

[Xiaoyao Li]

On 6/24/2020 2:20 AM, Jim Mattson wrote:
> On Tue, Jun 23, 2020 at 4:58 AM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>>
>> It needs to invalidate CPUID configruations if usersapce provides
> 
> Nits: configurations, userspace

oh, I'll fix it.

>> illegal input.
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   arch/x86/kvm/cpuid.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 8a294f9747aa..1d13bad42bf9 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -207,6 +207,8 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>>          kvm_apic_set_version(vcpu);
>>          kvm_x86_ops.cpuid_update(vcpu);
>>          r = kvm_update_cpuid(vcpu);
>> +       if (r)
>> +               vcpu->arch.cpuid_nent = 0;
>>
>>          kvfree(cpuid_entries);
>>   out:
>> @@ -230,6 +232,8 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
>>          kvm_apic_set_version(vcpu);
>>          kvm_x86_ops.cpuid_update(vcpu);
>>          r = kvm_update_cpuid(vcpu);
>> +       if (r)
>> +               vcpu->arch.cpuid_nent = 0;
>>   out:
>>          return r;
>>   }
>> --
>> 2.18.2
> 
> What if vcpu->arch.cpuid_nent was greater than 0 before the ioctl in question?
>

Nice catch!

If considering it, then we have to restore the old CPUID configuration. 
So how about making it simpler to just add one line of comment in API doc:
If KVM_SET_CPUID{2} fails, the old valid configuration is cleared as a 
side effect.

Re: [PATCH v2 2/7] KVM: X86: Go on updating other CPUID leaves when leaf 1 is absent

[Sean Christopherson]

On Tue, Jun 23, 2020 at 07:58:11PM +0800, Xiaoyao Li wrote:
> As handling of bits other leaf 1 added over time, kvm_update_cpuid()
> should not return directly if leaf 1 is absent, but should go on
> updateing other CPUID leaves.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>

This should probably be marked for stable.

> ---
>  arch/x86/kvm/cpuid.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 1d13bad42bf9..0164dac95ef5 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -60,22 +60,21 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
>  	struct kvm_lapic *apic = vcpu->arch.apic;
>  
>  	best = kvm_find_cpuid_entry(vcpu, 1, 0);
> -	if (!best)
> -		return 0;

Rather than wrap the existing code, what about throwing it in a separate
helper?  That generates an easier to read diff and also has the nice
property of getting 'apic' out of the common code.

> -
> -	/* Update OSXSAVE bit */
> -	if (boot_cpu_has(X86_FEATURE_XSAVE) && best->function == 0x1)
> -		cpuid_entry_change(best, X86_FEATURE_OSXSAVE,
> +	if (best) {
> +		/* Update OSXSAVE bit */
> +		if (boot_cpu_has(X86_FEATURE_XSAVE))
> +			cpuid_entry_change(best, X86_FEATURE_OSXSAVE,
>  				   kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE));
>  
> -	cpuid_entry_change(best, X86_FEATURE_APIC,
> +		cpuid_entry_change(best, X86_FEATURE_APIC,
>  			   vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE);
>  
> -	if (apic) {
> -		if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
> -			apic->lapic_timer.timer_mode_mask = 3 << 17;
> -		else
> -			apic->lapic_timer.timer_mode_mask = 1 << 17;
> +		if (apic) {
> +			if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
> +				apic->lapic_timer.timer_mode_mask = 3 << 17;
> +			else
> +				apic->lapic_timer.timer_mode_mask = 1 << 17;
> +		}
>  	}
>  
>  	best = kvm_find_cpuid_entry(vcpu, 7, 0);
> -- 
> 2.18.2
> 

Re: [PATCH v2 6/7] KVM: X86: Move kvm_x86_ops.update_vcpu_model() into kvm_update_vcpu_model()

[Sean Christopherson]

On Tue, Jun 23, 2020 at 07:58:15PM +0800, Xiaoyao Li wrote:
> kvm_x86_ops.update_vcpu_model() is used to update vmx/svm vcpu settings
> based on updated CPUID settings. So it's supposed to be called after
> CPUIDs are fully updated, i.e., kvm_update_cpuid().
> 
> Move it in kvm_update_vcpu_model().

The changelog needs to provide an in-depth analysis of VMX and SVM to prove
that there are no existing dependencies in the ordering.  I've done the
analysis a few times over the past few years for a similar chage I carried
in my SGX code, but dropped that code a while back and haven't done the
analysis since.  Anyways, it should be documented.

> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> ---
>  arch/x86/kvm/cpuid.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index d2f93823f9fd..5decc2dd5448 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -121,6 +121,8 @@ void kvm_update_vcpu_model(struct kvm_vcpu *vcpu)
>  	struct kvm_lapic *apic = vcpu->arch.apic;
>  	struct kvm_cpuid_entry2 *best;
>  
> +	kvm_x86_ops.update_vcpu_model(vcpu);
> +
>  	best = kvm_find_cpuid_entry(vcpu, 1, 0);
>  	if (best && apic) {
>  		if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
> @@ -136,6 +138,7 @@ void kvm_update_vcpu_model(struct kvm_vcpu *vcpu)
>  		vcpu->arch.guest_supported_xcr0 =
>  			(best->eax | ((u64)best->edx << 32)) & supported_xcr0;
>  
> +

Spurious whitespace.

>  	/* Note, maxphyaddr must be updated before tdp_level. */
>  	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
>  	vcpu->arch.tdp_level = kvm_x86_ops.get_tdp_level(vcpu);
> @@ -224,7 +227,6 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>  
>  	cpuid_fix_nx_cap(vcpu);
>  	kvm_apic_set_version(vcpu);
> -	kvm_x86_ops.update_vcpu_model(vcpu);
>  	kvm_update_cpuid(vcpu);
>  	kvm_update_vcpu_model(vcpu);
>  
> @@ -254,7 +256,6 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
>  	}
>  
>  	kvm_apic_set_version(vcpu);
> -	kvm_x86_ops.update_vcpu_model(vcpu);
>  	kvm_update_cpuid(vcpu);
>  	kvm_update_vcpu_model(vcpu);
>  out:
> -- 
> 2.18.2
> 

Re: [PATCH v2 7/7] KVM: X86: Move kvm_apic_set_version() to kvm_update_vcpu_model()

[Sean Christopherson]

On Tue, Jun 23, 2020 at 07:58:16PM +0800, Xiaoyao Li wrote:
> Obviously, kvm_apic_set_version() fits well in kvm_update_vcpu_model().

Same as the last patch, it would be nice to explicitly document that there
are no dependencies between kvm_apic_set_version() and kvm_update_cpuid().

> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  arch/x86/kvm/cpuid.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 5decc2dd5448..3428f4d84b42 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -129,6 +129,8 @@ void kvm_update_vcpu_model(struct kvm_vcpu *vcpu)
>  			apic->lapic_timer.timer_mode_mask = 3 << 17;
>  		else
>  			apic->lapic_timer.timer_mode_mask = 1 << 17;
> +
> +		kvm_apic_set_version(vcpu);
>  	}
>  
>  	best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
> @@ -226,7 +228,6 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>  	}
>  
>  	cpuid_fix_nx_cap(vcpu);
> -	kvm_apic_set_version(vcpu);
>  	kvm_update_cpuid(vcpu);
>  	kvm_update_vcpu_model(vcpu);
>  
> @@ -255,7 +256,6 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
>  		goto out;
>  	}
>  
> -	kvm_apic_set_version(vcpu);
>  	kvm_update_cpuid(vcpu);
>  	kvm_update_vcpu_model(vcpu);
>  out:
> -- 
> 2.18.2
> 

Re: [PATCH v2 2/7] KVM: X86: Go on updating other CPUID leaves when leaf 1 is absent

[Sean Christopherson]

On Thu, Jul 02, 2020 at 11:54:03AM -0700, Sean Christopherson wrote:
> On Tue, Jun 23, 2020 at 07:58:11PM +0800, Xiaoyao Li wrote:
> > As handling of bits other leaf 1 added over time, kvm_update_cpuid()
> > should not return directly if leaf 1 is absent, but should go on
> > updateing other CPUID leaves.
> > 
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> 
> This should probably be marked for stable.
> 
> > ---
> >  arch/x86/kvm/cpuid.c | 23 +++++++++++------------
> >  1 file changed, 11 insertions(+), 12 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index 1d13bad42bf9..0164dac95ef5 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -60,22 +60,21 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
> >  	struct kvm_lapic *apic = vcpu->arch.apic;
> >  
> >  	best = kvm_find_cpuid_entry(vcpu, 1, 0);
> > -	if (!best)
> > -		return 0;
> 
> Rather than wrap the existing code, what about throwing it in a separate
> helper?  That generates an easier to read diff and also has the nice
> property of getting 'apic' out of the common code.

Hrm, that'd be overkill once the apic code is moved in a few patches.
What if you keep the cpuid updates wrapped (as in this patch), but then
do

	if (best && apic) {
	}

for the apic path?  That'll minimize churn for code that is disappearing,
e.g. will make future git archaeologists happy :-).

> > -
> > -	/* Update OSXSAVE bit */
> > -	if (boot_cpu_has(X86_FEATURE_XSAVE) && best->function == 0x1)
> > -		cpuid_entry_change(best, X86_FEATURE_OSXSAVE,
> > +	if (best) {
> > +		/* Update OSXSAVE bit */
> > +		if (boot_cpu_has(X86_FEATURE_XSAVE))
> > +			cpuid_entry_change(best, X86_FEATURE_OSXSAVE,
> >  				   kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE));
> >  
> > -	cpuid_entry_change(best, X86_FEATURE_APIC,
> > +		cpuid_entry_change(best, X86_FEATURE_APIC,
> >  			   vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE);
> >  
> > -	if (apic) {
> > -		if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
> > -			apic->lapic_timer.timer_mode_mask = 3 << 17;
> > -		else
> > -			apic->lapic_timer.timer_mode_mask = 1 << 17;
> > +		if (apic) {
> > +			if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
> > +				apic->lapic_timer.timer_mode_mask = 3 << 17;
> > +			else
> > +				apic->lapic_timer.timer_mode_mask = 1 << 17;
> > +		}
> >  	}
> >  
> >  	best = kvm_find_cpuid_entry(vcpu, 7, 0);
> > -- 
> > 2.18.2
> > 

Re: [PATCH v2 2/7] KVM: X86: Go on updating other CPUID leaves when leaf 1 is absent

[Xiaoyao Li]

On 7/3/2020 3:02 AM, Sean Christopherson wrote:
> On Thu, Jul 02, 2020 at 11:54:03AM -0700, Sean Christopherson wrote:
>> On Tue, Jun 23, 2020 at 07:58:11PM +0800, Xiaoyao Li wrote:
>>> As handling of bits other leaf 1 added over time, kvm_update_cpuid()
>>> should not return directly if leaf 1 is absent, but should go on
>>> updateing other CPUID leaves.
>>>
>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>
>> This should probably be marked for stable.
>>
>>> ---
>>>   arch/x86/kvm/cpuid.c | 23 +++++++++++------------
>>>   1 file changed, 11 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>>> index 1d13bad42bf9..0164dac95ef5 100644
>>> --- a/arch/x86/kvm/cpuid.c
>>> +++ b/arch/x86/kvm/cpuid.c
>>> @@ -60,22 +60,21 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
>>>   	struct kvm_lapic *apic = vcpu->arch.apic;
>>>   
>>>   	best = kvm_find_cpuid_entry(vcpu, 1, 0);
>>> -	if (!best)
>>> -		return 0;
>>
>> Rather than wrap the existing code, what about throwing it in a separate
>> helper?  That generates an easier to read diff and also has the nice
>> property of getting 'apic' out of the common code.
> 
> Hrm, that'd be overkill once the apic code is moved in a few patches.
> What if you keep the cpuid updates wrapped (as in this patch), but then
> do
> 
> 	if (best && apic) {
> 	}
> 
> for the apic path?  That'll minimize churn for code that is disappearing,
> e.g. will make future git archaeologists happy :-).

Sure. I'll do it in next version.

>>> -
>>> -	/* Update OSXSAVE bit */
>>> -	if (boot_cpu_has(X86_FEATURE_XSAVE) && best->function == 0x1)
>>> -		cpuid_entry_change(best, X86_FEATURE_OSXSAVE,
>>> +	if (best) {
>>> +		/* Update OSXSAVE bit */
>>> +		if (boot_cpu_has(X86_FEATURE_XSAVE))
>>> +			cpuid_entry_change(best, X86_FEATURE_OSXSAVE,
>>>   				   kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE));
>>>   
>>> -	cpuid_entry_change(best, X86_FEATURE_APIC,
>>> +		cpuid_entry_change(best, X86_FEATURE_APIC,
>>>   			   vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE);
>>>   
>>> -	if (apic) {
>>> -		if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
>>> -			apic->lapic_timer.timer_mode_mask = 3 << 17;
>>> -		else
>>> -			apic->lapic_timer.timer_mode_mask = 1 << 17;
>>> +		if (apic) {
>>> +			if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
>>> +				apic->lapic_timer.timer_mode_mask = 3 << 17;
>>> +			else
>>> +				apic->lapic_timer.timer_mode_mask = 1 << 17;
>>> +		}
>>>   	}
>>>   
>>>   	best = kvm_find_cpuid_entry(vcpu, 7, 0);
>>> -- 
>>> 2.18.2
>>>

Re: [PATCH v2 6/7] KVM: X86: Move kvm_x86_ops.update_vcpu_model() into kvm_update_vcpu_model()

[Xiaoyao Li]

On 7/3/2020 2:59 AM, Sean Christopherson wrote:
> On Tue, Jun 23, 2020 at 07:58:15PM +0800, Xiaoyao Li wrote:
>> kvm_x86_ops.update_vcpu_model() is used to update vmx/svm vcpu settings
>> based on updated CPUID settings. So it's supposed to be called after
>> CPUIDs are fully updated, i.e., kvm_update_cpuid().
>>
>> Move it in kvm_update_vcpu_model().
> 
> The changelog needs to provide an in-depth analysis of VMX and SVM to prove
> that there are no existing dependencies in the ordering.  I've done the
> analysis a few times over the past few years for a similar chage I carried
> in my SGX code, but dropped that code a while back and haven't done the
> analysis since.  Anyways, it should be documented.

No problem. Will add the analysis in next version.

>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>> ---
>>   arch/x86/kvm/cpuid.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index d2f93823f9fd..5decc2dd5448 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -121,6 +121,8 @@ void kvm_update_vcpu_model(struct kvm_vcpu *vcpu)
>>   	struct kvm_lapic *apic = vcpu->arch.apic;
>>   	struct kvm_cpuid_entry2 *best;
>>   
>> +	kvm_x86_ops.update_vcpu_model(vcpu);
>> +
>>   	best = kvm_find_cpuid_entry(vcpu, 1, 0);
>>   	if (best && apic) {
>>   		if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
>> @@ -136,6 +138,7 @@ void kvm_update_vcpu_model(struct kvm_vcpu *vcpu)
>>   		vcpu->arch.guest_supported_xcr0 =
>>   			(best->eax | ((u64)best->edx << 32)) & supported_xcr0;
>>   
>> +
> 
> Spurious whitespace.
> 
>>   	/* Note, maxphyaddr must be updated before tdp_level. */
>>   	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
>>   	vcpu->arch.tdp_level = kvm_x86_ops.get_tdp_level(vcpu);
>> @@ -224,7 +227,6 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>>   
>>   	cpuid_fix_nx_cap(vcpu);
>>   	kvm_apic_set_version(vcpu);
>> -	kvm_x86_ops.update_vcpu_model(vcpu);
>>   	kvm_update_cpuid(vcpu);
>>   	kvm_update_vcpu_model(vcpu);
>>   
>> @@ -254,7 +256,6 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
>>   	}
>>   
>>   	kvm_apic_set_version(vcpu);
>> -	kvm_x86_ops.update_vcpu_model(vcpu);
>>   	kvm_update_cpuid(vcpu);
>>   	kvm_update_vcpu_model(vcpu);
>>   out:
>> -- 
>> 2.18.2
>>

Re: [PATCH v2 7/7] KVM: X86: Move kvm_apic_set_version() to kvm_update_vcpu_model()

[Xiaoyao Li]

On 7/3/2020 3:00 AM, Sean Christopherson wrote:
> On Tue, Jun 23, 2020 at 07:58:16PM +0800, Xiaoyao Li wrote:
>> Obviously, kvm_apic_set_version() fits well in kvm_update_vcpu_model().
> 
> Same as the last patch, it would be nice to explicitly document that there
> are no dependencies between kvm_apic_set_version() and kvm_update_cpuid().

Sure, will do.

Thanks!

>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   arch/x86/kvm/cpuid.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 5decc2dd5448..3428f4d84b42 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -129,6 +129,8 @@ void kvm_update_vcpu_model(struct kvm_vcpu *vcpu)
>>   			apic->lapic_timer.timer_mode_mask = 3 << 17;
>>   		else
>>   			apic->lapic_timer.timer_mode_mask = 1 << 17;
>> +
>> +		kvm_apic_set_version(vcpu);
>>   	}
>>   
>>   	best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
>> @@ -226,7 +228,6 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
>>   	}
>>   
>>   	cpuid_fix_nx_cap(vcpu);
>> -	kvm_apic_set_version(vcpu);
>>   	kvm_update_cpuid(vcpu);
>>   	kvm_update_vcpu_model(vcpu);
>>   
>> @@ -255,7 +256,6 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
>>   		goto out;
>>   	}
>>   
>> -	kvm_apic_set_version(vcpu);
>>   	kvm_update_cpuid(vcpu);
>>   	kvm_update_vcpu_model(vcpu);
>>   out:
>> -- 
>> 2.18.2
>>