* [PATCH 1/3] KVM: x86: Remove superfluous brackets from case statement
2020-02-18 23:40 [PATCH 0/3] KVM: x86: kvm_init_msr_list() cleanup Sean Christopherson
@ 2020-02-18 23:40 ` Sean Christopherson
2020-02-18 23:40 ` [PATCH 2/3] KVM: x86: Take an unsigned 32-bit int for has_emulated_msr()'s index Sean Christopherson
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Sean Christopherson @ 2020-02-18 23:40 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 2/3] KVM: x86: Take an unsigned 32-bit int for has_emulated_msr()'s index
2020-02-18 23:40 [PATCH 0/3] KVM: x86: kvm_init_msr_list() cleanup Sean Christopherson
2020-02-18 23:40 ` [PATCH 1/3] KVM: x86: Remove superfluous brackets from case statement Sean Christopherson
@ 2020-02-18 23:40 ` Sean Christopherson
2020-02-18 23:40 ` [PATCH 3/3] KVM: x86: Snapshot MSR index in a local variable when processing lists Sean Christopherson
2020-05-18 10:52 ` [PATCH 0/3] KVM: x86: kvm_init_msr_list() cleanup Paolo Bonzini
3 siblings, 0 replies; 5+ messages in thread
From: Sean Christopherson @ 2020-02-18 23:40 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 3/3] KVM: x86: Snapshot MSR index in a local variable when processing lists
2020-02-18 23:40 [PATCH 0/3] KVM: x86: kvm_init_msr_list() cleanup Sean Christopherson
2020-02-18 23:40 ` [PATCH 1/3] KVM: x86: Remove superfluous brackets from case statement Sean Christopherson
2020-02-18 23:40 ` [PATCH 2/3] KVM: x86: Take an unsigned 32-bit int for has_emulated_msr()'s index Sean Christopherson
@ 2020-02-18 23:40 ` Sean Christopherson
2020-05-18 10:52 ` [PATCH 0/3] KVM: x86: kvm_init_msr_list() cleanup Paolo Bonzini
3 siblings, 0 replies; 5+ messages in thread
From: Sean Christopherson @ 2020-02-18 23:40 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 0/3] KVM: x86: kvm_init_msr_list() cleanup
2020-02-18 23:40 [PATCH 0/3] KVM: x86: kvm_init_msr_list() cleanup Sean Christopherson
` (2 preceding siblings ...)
2020-02-18 23:40 ` [PATCH 3/3] KVM: x86: Snapshot MSR index in a local variable when processing lists Sean Christopherson
@ 2020-05-18 10:52 ` Paolo Bonzini
3 siblings, 0 replies; 5+ messages in thread
From: Paolo Bonzini @ 2020-05-18 10:52 UTC (permalink / raw)
To: Sean Christopherson
Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread