[PATCH v3 0/8] Refactor handling flow of KVM_SET_CPUID*

[PATCH v3 0/8] Refactor handling flow of KVM_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. 

v3:
 - Add a note in KVM api doc to state the previous CPUID configuration
   is not reliable if current KVM_SET_CPUID* fails [Jim]
 - Adjust Patch 2 to reduce code churn [Sean]
 - Commit message refine to add more justification [Sean]
 - Add a new patch (7)

v2:
https://lkml.kernel.org/r/20200623115816.24132-1-xiaoyao.li@intel.com
 - 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 (8):
  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: lapic: Use guest_cpuid_has() in kvm_apic_set_version()
  KVM: X86: Move kvm_apic_set_version() to kvm_update_vcpu_model()

 Documentation/virt/kvm/api.rst  |   4 ++
 arch/x86/include/asm/kvm_host.h |   2 +-
 arch/x86/kvm/cpuid.c            | 107 ++++++++++++++++++++------------
 arch/x86/kvm/cpuid.h            |   3 +-
 arch/x86/kvm/lapic.c            |   4 +-
 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 +
 9 files changed, 81 insertions(+), 50 deletions(-)

-- 
2.18.4

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

[Xiaoyao Li]

Current implementation keeps userspace input of CPUID configuration and
cpuid->nent even if kvm_update_cpuid() fails. Reset vcpu->arch.cpuid_nent
to 0 for the case of failure as a simple fix.

Besides, update the doc to explicitly state that if IOCTL SET_CPUID*
fail KVM gives no gurantee that previous valid CPUID configuration is
kept.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 Documentation/virt/kvm/api.rst | 4 ++++
 arch/x86/kvm/cpuid.c           | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 1cfe79b932d6..3ca809a1a44f 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -669,6 +669,10 @@ MSRs that have been set successfully.
 Defines the vcpu responses to the cpuid instruction.  Applications
 should use the KVM_SET_CPUID2 ioctl if available.
 
+Note, when this IOCTL fails, KVM gives no guarantees that previous valid CPUID
+configuration (if there is) is not corrupted. Userspace can get a copy of valid
+CPUID configuration through KVM_GET_CPUID2 in case.
+
 ::
 
   struct kvm_cpuid_entry {
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.4

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

[Xiaoyao Li]

As handling of bits out of 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.

Keep the update of apic->lapic_timer.timer_mode_mask in a separate
wrapper, to minimize churn for code since it will be moved out of this
function in a future patch.

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

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 1d13bad42bf9..0e3a23c2ea55 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -60,18 +60,17 @@ 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 (best && apic) {
 		if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
 			apic->lapic_timer.timer_mode_mask = 3 << 17;
 		else
-- 
2.18.4

[PATCH v3 3/8] 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 0e3a23c2ea55..a825878b7f84 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.4

[PATCH v3 4/8] 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 a825878b7f84..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) {
@@ -89,26 +88,14 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
 			   vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE);
 	}
 
-	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, 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);
-	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 09ee54f5e385..6f376392e6e6 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8184,6 +8184,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.4

[PATCH v3 5/8] 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 97cb005c7aa7..c35d14b257c9 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1051,7 +1051,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 74096aa72ad9..01f359e590d5 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3550,7 +3550,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);
 
@@ -4050,7 +4050,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 b627c5f36b9e..85080a5b8d3c 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -6354,7 +6354,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 8187ca152ad2..4673c84b54ac 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7257,7 +7257,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);
 
@@ -7915,7 +7915,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.4

[PATCH v3 6/8] 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().

Currently, kvm_update_cpuid() only updates CPUID bits of OSXSAVE, APIC,
OSPKE, MWAIT, KVM_FEATURE_PV_UNHALT and ebx of (leaf 0xD, subleaf 0x0),
ebx of (leaf 0xD, subleaf 0x1). None of them is consumed by vmx/svm's
update_vcpu_model().

So there is no dependency between kvm_x86_ops.update_vcpu_model() and
kvm_update_cpuid(). Move kvm_x86_ops.update_vcpu_model() into
kvm_update_vcpu_model() make it more reasonable.

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 d2f93823f9fd..89ffd9dccfc6 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))
@@ -224,7 +226,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 +255,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.4

[PATCH v3 7/8] KVM: lapic: Use guest_cpuid_has() in kvm_apic_set_version()

[Xiaoyao Li]

Only code cleanup and no functional change.

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

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 5bf72fc86a8e..e5dbb7ebae78 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -354,7 +354,6 @@ static inline int apic_lvt_nmi_mode(u32 lvt_val)
 void kvm_apic_set_version(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
-	struct kvm_cpuid_entry2 *feat;
 	u32 v = APIC_VERSION;
 
 	if (!lapic_in_kernel(vcpu))
@@ -367,8 +366,7 @@ void kvm_apic_set_version(struct kvm_vcpu *vcpu)
 	 * version first and level-triggered interrupts never get EOIed in
 	 * IOAPIC.
 	 */
-	feat = kvm_find_cpuid_entry(apic->vcpu, 0x1, 0);
-	if (feat && (feat->ecx & (1 << (X86_FEATURE_X2APIC & 31))) &&
+	if (guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) &&
 	    !ioapic_in_kernel(vcpu->kvm))
 		v |= APIC_LVR_DIRECTED_EOI;
 	kvm_lapic_set_reg(apic, APIC_LVR, v);
-- 
2.18.4

[PATCH v3 8/8] KVM: X86: Move kvm_apic_set_version() to kvm_update_vcpu_model()

[Xiaoyao Li]

There is no dependencies between kvm_apic_set_version() and
kvm_update_cpuid() because kvm_apic_set_version() queries X2APIC CPUID bit,
which is not touched/changed by kvm_update_cpuid().

Obviously, kvm_apic_set_version() belongs to the category of updating
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 89ffd9dccfc6..c183a11dbcff 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);
@@ -225,7 +227,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);
 
@@ -254,7 +255,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.4

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

[Paolo Bonzini]

On 08/07/20 08:50, Xiaoyao Li wrote:
> +Note, when this IOCTL fails, KVM gives no guarantees that previous valid CPUID
> +configuration (if there is) is not corrupted. Userspace can get a copy of valid
> +CPUID configuration through KVM_GET_CPUID2 in case.
> +
>  ::

I assume you mean "of the resulting CPUID configuration".

Paolo

Re: [PATCH v3 4/8] KVM: X86: Split kvm_update_cpuid()

[Paolo Bonzini]

On 08/07/20 08:50, Xiaoyao Li wrote:
> 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>

I would prefer to keep the kvm_update_cpuid name for what you called
kvm_update_vcpu_model(), and rename the rest to kvm_update_cpuid_runtime().

Paolo

> ---
>  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 a825878b7f84..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) {
> @@ -89,26 +88,14 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
>  			   vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE);
>  	}
>  
> -	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, 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);
> -	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 09ee54f5e385..6f376392e6e6 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8184,6 +8184,7 @@ static void enter_smm(struct kvm_vcpu *vcpu)
>  #endif
>  
>  	kvm_update_cpuid(vcpu);
> +	kvm_update_vcpu_model(vcpu);
>  	kvm_mmu_reset_context(vcpu);
>  }
>  
> 

Re: [PATCH v3 5/8] KVM: X86: Rename cpuid_update() to update_vcpu_model()

[Paolo Bonzini]

On 08/07/20 08:50, Xiaoyao Li wrote:
> 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>

With the change suggested in the previous patch, this one becomes
unnecessary (or you can rename it to update_cpuid).  While you're at it,
you could also move the call in kvm_update_cpuid.

Paolo

> ---
>  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 97cb005c7aa7..c35d14b257c9 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1051,7 +1051,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 74096aa72ad9..01f359e590d5 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3550,7 +3550,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);
>  
> @@ -4050,7 +4050,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 b627c5f36b9e..85080a5b8d3c 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -6354,7 +6354,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 8187ca152ad2..4673c84b54ac 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7257,7 +7257,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);
>  
> @@ -7915,7 +7915,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,
>  
> 

Re: [PATCH v3 0/8] Refactor handling flow of KVM_SET_CPUID*

[Paolo Bonzini]

On 08/07/20 08:50, Xiaoyao Li wrote:
> 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. 
> 
> v3:
>  - Add a note in KVM api doc to state the previous CPUID configuration
>    is not reliable if current KVM_SET_CPUID* fails [Jim]
>  - Adjust Patch 2 to reduce code churn [Sean]
>  - Commit message refine to add more justification [Sean]
>  - Add a new patch (7)
> 
> v2:
> https://lkml.kernel.org/r/20200623115816.24132-1-xiaoyao.li@intel.com
>  - 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 (8):
>   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: lapic: Use guest_cpuid_has() in kvm_apic_set_version()
>   KVM: X86: Move kvm_apic_set_version() to kvm_update_vcpu_model()
> 
>  Documentation/virt/kvm/api.rst  |   4 ++
>  arch/x86/include/asm/kvm_host.h |   2 +-
>  arch/x86/kvm/cpuid.c            | 107 ++++++++++++++++++++------------
>  arch/x86/kvm/cpuid.h            |   3 +-
>  arch/x86/kvm/lapic.c            |   4 +-
>  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 +
>  9 files changed, 81 insertions(+), 50 deletions(-)
> 

Queued patches 1/2/3/7/8, thanks.

Paolo

Re: [PATCH v3 3/8] KVM: X86: Introduce kvm_check_cpuid()

[Xiaoyao Li]

On 7/8/2020 2:50 PM, Xiaoyao Li wrote:
> 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>
[...]
> @@ -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;

Paolo,

here lack a kvfree(cpuid_entries);
Can you help fix it?

Apologize for it.


> +		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:

Re: [PATCH v3 4/8] KVM: X86: Split kvm_update_cpuid()

[Xiaoyao Li]

On 7/8/2020 8:06 PM, Paolo Bonzini wrote:
> On 08/07/20 08:50, Xiaoyao Li wrote:
>> 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>
> 
> I would prefer to keep the kvm_update_cpuid name for what you called
> kvm_update_vcpu_model(), and rename the rest to kvm_update_cpuid_runtime().

But there is no CPUID being updated in kvm_update_cpuid(), after 
kvm_update_cpuid_runtime() is split out. This is confusing, IMO.

> Paolo
> 
>> ---
>>   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 a825878b7f84..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) {
>> @@ -89,26 +88,14 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
>>   			   vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE);
>>   	}
>>   
>> -	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, 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);
>> -	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 09ee54f5e385..6f376392e6e6 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -8184,6 +8184,7 @@ static void enter_smm(struct kvm_vcpu *vcpu)
>>   #endif
>>   
>>   	kvm_update_cpuid(vcpu);
>> +	kvm_update_vcpu_model(vcpu);
>>   	kvm_mmu_reset_context(vcpu);
>>   }
>>   
>>
> 

Re: [PATCH v3 4/8] KVM: X86: Split kvm_update_cpuid()

[Paolo Bonzini]

On 08/07/20 14:33, Xiaoyao Li wrote:
> On 7/8/2020 8:06 PM, Paolo Bonzini wrote:
>> On 08/07/20 08:50, Xiaoyao Li wrote:
>>> 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>
>>
>> I would prefer to keep the kvm_update_cpuid name for what you called
>> kvm_update_vcpu_model(), and rename the rest to
>> kvm_update_cpuid_runtime().
> 
> But there is no CPUID being updated in kvm_update_cpuid(), after
> kvm_update_cpuid_runtime() is split out. This is confusing, IMO.

Then what about kvm_vcpu_after_set_cpuid()?  It's the "model" that is
not clear.

Thanks,

Paolo

>> Paolo
>>
>>> ---
>>>   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 a825878b7f84..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) {
>>> @@ -89,26 +88,14 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
>>>                  vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE);
>>>       }
>>>   -    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, 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);
>>> -    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 09ee54f5e385..6f376392e6e6 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -8184,6 +8184,7 @@ static void enter_smm(struct kvm_vcpu *vcpu)
>>>   #endif
>>>         kvm_update_cpuid(vcpu);
>>> +    kvm_update_vcpu_model(vcpu);
>>>       kvm_mmu_reset_context(vcpu);
>>>   }
>>>  
>>
> 

Re: [PATCH v3 3/8] KVM: X86: Introduce kvm_check_cpuid()

[Paolo Bonzini]

On 08/07/20 14:28, Xiaoyao Li wrote:
>> @@ -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;
> 
> Paolo,
> 
> here lack a kvfree(cpuid_entries);
> Can you help fix it?
> 
> Apologize for it.

Actually I hadn't queued this patch; my mistake.  You can fix it and resend.

Paolo

Re: [PATCH v3 4/8] KVM: X86: Split kvm_update_cpuid()

[Xiaoyao Li]

On 7/8/2020 8:41 PM, Paolo Bonzini wrote:
> On 08/07/20 14:33, Xiaoyao Li wrote:
>> On 7/8/2020 8:06 PM, Paolo Bonzini wrote:
>>> On 08/07/20 08:50, Xiaoyao Li wrote:
>>>> 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>
>>>
>>> I would prefer to keep the kvm_update_cpuid name for what you called
>>> kvm_update_vcpu_model(), and rename the rest to
>>> kvm_update_cpuid_runtime().
>>
>> But there is no CPUID being updated in kvm_update_cpuid(), after
>> kvm_update_cpuid_runtime() is split out. This is confusing, IMO.
> 
> Then what about kvm_vcpu_after_set_cpuid()?  It's the "model" that is
> not clear.

I'm ok with kvm_vcpu_after_set_cpuid().

BTW there is an unknown for me regarding enter_smm(). Currently, it 
calls kvm_update_cpuid(). I'm not sure which part it really needs, 
update CPUID or update vcpu state based on CPUID? maybe both?
So in this Patch, after splitting kvm_update_cpuid(), I keep both 
functions there to ensure no functional change in enter_smm().
So using the name "kvm_vcpu_after_set_cpuid" seems weird in that function.

> Thanks,
> 
> Paolo
> 
>>> Paolo
>>>
>>>> ---
>>>>    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 a825878b7f84..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) {
>>>> @@ -89,26 +88,14 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
>>>>                   vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE);
>>>>        }
>>>>    -    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, 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);
>>>> -    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 09ee54f5e385..6f376392e6e6 100644
>>>> --- a/arch/x86/kvm/x86.c
>>>> +++ b/arch/x86/kvm/x86.c
>>>> @@ -8184,6 +8184,7 @@ static void enter_smm(struct kvm_vcpu *vcpu)
>>>>    #endif
>>>>          kvm_update_cpuid(vcpu);
>>>> +    kvm_update_vcpu_model(vcpu);
>>>>        kvm_mmu_reset_context(vcpu);
>>>>    }
>>>>   
>>>
>>
> 

Re: [PATCH v3 4/8] KVM: X86: Split kvm_update_cpuid()

[Paolo Bonzini]

On 08/07/20 15:27, Xiaoyao Li wrote:
>>
> 
> I'm ok with kvm_vcpu_after_set_cpuid().
> 
> BTW there is an unknown for me regarding enter_smm(). Currently, it
> calls kvm_update_cpuid(). I'm not sure which part it really needs,
> update CPUID or update vcpu state based on CPUID?

It needs to update CPUID because it affects CR4.OSXSAVE among others.

Paolo

Re: [PATCH v3 0/8] Refactor handling flow of KVM_SET_CPUID*

[Xiaoyao Li]

On 7/8/2020 8:10 PM, Paolo Bonzini wrote:
> On 08/07/20 08:50, Xiaoyao Li wrote:
>> 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.
>>
>> v3:
>>   - Add a note in KVM api doc to state the previous CPUID configuration
>>     is not reliable if current KVM_SET_CPUID* fails [Jim]
>>   - Adjust Patch 2 to reduce code churn [Sean]
>>   - Commit message refine to add more justification [Sean]
>>   - Add a new patch (7)
>>
>> v2:
>> https://lkml.kernel.org/r/20200623115816.24132-1-xiaoyao.li@intel.com
>>   - 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 (8):
>>    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: lapic: Use guest_cpuid_has() in kvm_apic_set_version()
>>    KVM: X86: Move kvm_apic_set_version() to kvm_update_vcpu_model()
>>
>>   Documentation/virt/kvm/api.rst  |   4 ++
>>   arch/x86/include/asm/kvm_host.h |   2 +-
>>   arch/x86/kvm/cpuid.c            | 107 ++++++++++++++++++++------------
>>   arch/x86/kvm/cpuid.h            |   3 +-
>>   arch/x86/kvm/lapic.c            |   4 +-
>>   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 +
>>   9 files changed, 81 insertions(+), 50 deletions(-)
>>
> 
> Queued patches 1/2/3/7/8, thanks.

Paolo,

I notice that you queued patch 8 into kvm/queue branch as
commit 84dd4897524e "KVM: X86: Move kvm_apic_set_version() to 
kvm_update_vcpu_model()"

Can you change the subject of that commit to "KVM: X86: Move 
kvm_apic_set_version() to kvm_update_cpuid()" ?

> Paolo
> 

Re: [PATCH v3 0/8] Refactor handling flow of KVM_SET_CPUID*

[Paolo Bonzini]

On 09/07/20 06:27, Xiaoyao Li wrote:
> On 7/8/2020 8:10 PM, Paolo Bonzini wrote:
>> On 08/07/20 08:50, Xiaoyao Li wrote:
>>> 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.
>>>
>>> v3:
>>>   - Add a note in KVM api doc to state the previous CPUID configuration
>>>     is not reliable if current KVM_SET_CPUID* fails [Jim]
>>>   - Adjust Patch 2 to reduce code churn [Sean]
>>>   - Commit message refine to add more justification [Sean]
>>>   - Add a new patch (7)
>>>
>>> v2:
>>> https://lkml.kernel.org/r/20200623115816.24132-1-xiaoyao.li@intel.com
>>>   - 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 (8):
>>>    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: lapic: Use guest_cpuid_has() in kvm_apic_set_version()
>>>    KVM: X86: Move kvm_apic_set_version() to kvm_update_vcpu_model()
>>>
>>>   Documentation/virt/kvm/api.rst  |   4 ++
>>>   arch/x86/include/asm/kvm_host.h |   2 +-
>>>   arch/x86/kvm/cpuid.c            | 107 ++++++++++++++++++++------------
>>>   arch/x86/kvm/cpuid.h            |   3 +-
>>>   arch/x86/kvm/lapic.c            |   4 +-
>>>   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 +
>>>   9 files changed, 81 insertions(+), 50 deletions(-)
>>>
>>
>> Queued patches 1/2/3/7/8, thanks.
> 
> Paolo,
> 
> I notice that you queued patch 8 into kvm/queue branch as
> commit 84dd4897524e "KVM: X86: Move kvm_apic_set_version() to
> kvm_update_vcpu_model()"
> 
> Can you change the subject of that commit to "KVM: X86: Move
> kvm_apic_set_version() to kvm_update_cpuid()" ?

Good catch, thanks.

Paolo