This serial is the extended version of
https://lkml.kernel.org/r/[email protected]
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/[email protected]
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
It needs to invalidate CPUID configruations if usersapce provides
illegal input.
Signed-off-by: Xiaoyao Li <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Obviously, kvm_apic_set_version() fits well in kvm_update_vcpu_model().
Signed-off-by: Xiaoyao Li <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
---
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
Use kvm_check_cpuid() to validate if userspace provides legal cpuid
settings and call it before KVM updates CPUID.
Signed-off-by: Xiaoyao Li <[email protected]>
---
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
On Tue, Jun 23, 2020 at 4:58 AM Xiaoyao Li <[email protected]> wrote:
>
> It needs to invalidate CPUID configruations if usersapce provides
Nits: configurations, userspace
> illegal input.
>
> Signed-off-by: Xiaoyao Li <[email protected]>
> ---
> 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?
On 6/24/2020 2:20 AM, Jim Mattson wrote:
> On Tue, Jun 23, 2020 at 4:58 AM Xiaoyao Li <[email protected]> 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 <[email protected]>
>> ---
>> 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.
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 <[email protected]>
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
>
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 <[email protected]>
> ---
> ---
> 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
>
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
>
> 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
> >
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 <[email protected]>
>>
>> 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
>>>
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 <[email protected]>
>> ---
>> ---
>> 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
>>
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 <[email protected]>
>> ---
>> 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
>>