[PATCH 0/3] KVM: x86: kvm_init_msr_list() cleanup

[PATCH 0/3] KVM: x86: kvm_init_msr_list() cleanup

[Sean Christopherson]

Three cleanup patches in kvm_init_msr_list() and related code.

Note, these were previously posted as the first three patches of larger
series[*] that mutated into an entirely different beast, which is how they
have been reviewed by Vitaly despite being v1.

[*] https://patchwork.kernel.org/cover/11357065/

Sean Christopherson (3):
  KVM: x86: Remove superfluous brackets from case statement
  KVM: x86: Take an unsigned 32-bit int for has_emulated_msr()'s index
  KVM: x86: Snapshot MSR index in a local variable when processing lists

 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/svm.c              |  2 +-
 arch/x86/kvm/vmx/vmx.c          |  2 +-
 arch/x86/kvm/x86.c              | 26 +++++++++++++++-----------
 4 files changed, 18 insertions(+), 14 deletions(-)

-- 
2.24.1

[PATCH 1/3] KVM: x86: Remove superfluous brackets from case statement

[Sean Christopherson]

Remove unnecessary brackets from a case statement that unintentionally
encapsulates unrelated case statements in the same switch statement.
While technically legal and functionally correct syntax, the brackets
are visually confusing and potentially dangerous, e.g. the last of the
encapsulated case statements has an undocumented fall-through that isn't
flagged by compilers due the encapsulation.

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/x86.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fbabb2f06273..0d3aa4e5f7bb 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5269,7 +5269,7 @@ static void kvm_init_msr_list(void)
 				 !intel_pt_validate_hw_cap(PT_CAP_single_range_output)))
 				continue;
 			break;
-		case MSR_IA32_RTIT_ADDR0_A ... MSR_IA32_RTIT_ADDR3_B: {
+		case MSR_IA32_RTIT_ADDR0_A ... MSR_IA32_RTIT_ADDR3_B:
 			if (!kvm_x86_ops->pt_supported() ||
 				msrs_to_save_all[i] - MSR_IA32_RTIT_ADDR0_A >=
 				intel_pt_validate_hw_cap(PT_CAP_num_address_ranges) * 2)
@@ -5284,7 +5284,7 @@ static void kvm_init_msr_list(void)
 			if (msrs_to_save_all[i] - MSR_ARCH_PERFMON_EVENTSEL0 >=
 			    min(INTEL_PMC_MAX_GENERIC, x86_pmu.num_counters_gp))
 				continue;
-		}
+			break;
 		default:
 			break;
 		}
-- 
2.24.1

[PATCH 2/3] KVM: x86: Take an unsigned 32-bit int for has_emulated_msr()'s index

[Sean Christopherson]

Take a u32 for the index in has_emulated_msr() to match hardware, which
treats MSR indices as unsigned 32-bit values.  Functionally, taking a
signed int doesn't cause problems with the current code base, but could
theoretically cause problems with 32-bit KVM, e.g. if the index were
checked via a less-than statement, which would evaluate incorrectly for
MSR indices with bit 31 set.

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/asm/kvm_host.h | 2 +-
 arch/x86/kvm/svm.c              | 2 +-
 arch/x86/kvm/vmx/vmx.c          | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 4dffbc10d3f8..cac9c40e505c 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1050,7 +1050,7 @@ struct kvm_x86_ops {
 	int (*hardware_setup)(void);               /* __init */
 	void (*hardware_unsetup)(void);            /* __exit */
 	bool (*cpu_has_accelerated_tpr)(void);
-	bool (*has_emulated_msr)(int index);
+	bool (*has_emulated_msr)(u32 index);
 	void (*cpuid_update)(struct kvm_vcpu *vcpu);
 
 	struct kvm *(*vm_alloc)(void);
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index a3e32d61d60c..2991a4a1fc30 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -5989,7 +5989,7 @@ static bool svm_cpu_has_accelerated_tpr(void)
 	return false;
 }
 
-static bool svm_has_emulated_msr(int index)
+static bool svm_has_emulated_msr(u32 index)
 {
 	switch (index) {
 	case MSR_IA32_MCG_EXT_CTL:
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 9a6664886f2e..47017da35ac4 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6251,7 +6251,7 @@ static void vmx_handle_exit_irqoff(struct kvm_vcpu *vcpu,
 		*exit_fastpath = handle_fastpath_set_msr_irqoff(vcpu);
 }
 
-static bool vmx_has_emulated_msr(int index)
+static bool vmx_has_emulated_msr(u32 index)
 {
 	switch (index) {
 	case MSR_IA32_SMBASE:
-- 
2.24.1

[PATCH 3/3] KVM: x86: Snapshot MSR index in a local variable when processing lists

[Sean Christopherson]

Snapshot the MSR index when processing the virtualized and emulated MSR
lists in kvm_init_msr_list() to improve code readability, particularly
in the RTIT and PerfMon MSR checks.

No functional change intended.

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/x86.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0d3aa4e5f7bb..0475dfb337c6 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5223,7 +5223,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 static void kvm_init_msr_list(void)
 {
 	struct x86_pmu_capability x86_pmu;
-	u32 dummy[2];
+	u32 dummy[2], msr_index;
 	unsigned i;
 
 	BUILD_BUG_ON_MSG(INTEL_PMC_MAX_FIXED != 4,
@@ -5236,14 +5236,16 @@ static void kvm_init_msr_list(void)
 	num_msr_based_features = 0;
 
 	for (i = 0; i < ARRAY_SIZE(msrs_to_save_all); i++) {
-		if (rdmsr_safe(msrs_to_save_all[i], &dummy[0], &dummy[1]) < 0)
+		msr_index = msrs_to_save_all[i];
+
+		if (rdmsr_safe(msr_index, &dummy[0], &dummy[1]) < 0)
 			continue;
 
 		/*
 		 * Even MSRs that are valid in the host may not be exposed
 		 * to the guests in some cases.
 		 */
-		switch (msrs_to_save_all[i]) {
+		switch (msr_index) {
 		case MSR_IA32_BNDCFGS:
 			if (!kvm_mpx_supported())
 				continue;
@@ -5271,17 +5273,17 @@ static void kvm_init_msr_list(void)
 			break;
 		case MSR_IA32_RTIT_ADDR0_A ... MSR_IA32_RTIT_ADDR3_B:
 			if (!kvm_x86_ops->pt_supported() ||
-				msrs_to_save_all[i] - MSR_IA32_RTIT_ADDR0_A >=
+				msr_index - MSR_IA32_RTIT_ADDR0_A >=
 				intel_pt_validate_hw_cap(PT_CAP_num_address_ranges) * 2)
 				continue;
 			break;
 		case MSR_ARCH_PERFMON_PERFCTR0 ... MSR_ARCH_PERFMON_PERFCTR0 + 17:
-			if (msrs_to_save_all[i] - MSR_ARCH_PERFMON_PERFCTR0 >=
+			if (msr_index - MSR_ARCH_PERFMON_PERFCTR0 >=
 			    min(INTEL_PMC_MAX_GENERIC, x86_pmu.num_counters_gp))
 				continue;
 			break;
 		case MSR_ARCH_PERFMON_EVENTSEL0 ... MSR_ARCH_PERFMON_EVENTSEL0 + 17:
-			if (msrs_to_save_all[i] - MSR_ARCH_PERFMON_EVENTSEL0 >=
+			if (msr_index - MSR_ARCH_PERFMON_EVENTSEL0 >=
 			    min(INTEL_PMC_MAX_GENERIC, x86_pmu.num_counters_gp))
 				continue;
 			break;
@@ -5289,14 +5291,16 @@ static void kvm_init_msr_list(void)
 			break;
 		}
 
-		msrs_to_save[num_msrs_to_save++] = msrs_to_save_all[i];
+		msrs_to_save[num_msrs_to_save++] = msr_index;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(emulated_msrs_all); i++) {
-		if (!kvm_x86_ops->has_emulated_msr(emulated_msrs_all[i]))
+		msr_index = emulated_msrs_all[i];
+
+		if (!kvm_x86_ops->has_emulated_msr(msr_index))
 			continue;
 
-		emulated_msrs[num_emulated_msrs++] = emulated_msrs_all[i];
+		emulated_msrs[num_emulated_msrs++] = msr_index;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(msr_based_features_all); i++) {
-- 
2.24.1

Re: [PATCH 0/3] KVM: x86: kvm_init_msr_list() cleanup

[Paolo Bonzini]

On 19/02/20 00:40, Sean Christopherson wrote:
> Three cleanup patches in kvm_init_msr_list() and related code.
> 
> Note, these were previously posted as the first three patches of larger
> series[*] that mutated into an entirely different beast, which is how they
> have been reviewed by Vitaly despite being v1.
> 
> [*] https://patchwork.kernel.org/cover/11357065/
> 
> Sean Christopherson (3):
>   KVM: x86: Remove superfluous brackets from case statement
>   KVM: x86: Take an unsigned 32-bit int for has_emulated_msr()'s index
>   KVM: x86: Snapshot MSR index in a local variable when processing lists
> 
>  arch/x86/include/asm/kvm_host.h |  2 +-
>  arch/x86/kvm/svm.c              |  2 +-
>  arch/x86/kvm/vmx/vmx.c          |  2 +-
>  arch/x86/kvm/x86.c              | 26 +++++++++++++++-----------
>  4 files changed, 18 insertions(+), 14 deletions(-)
> 

Queued (better late than never).

Paolo