INTEL_PMC_MAX_GENERIC is currently 32, which exceeds the 18 contiguous
MSR indices reserved by Intel for event selectors. Since some machines
actually have MSRs past the reserved range, these may survive the
filtering of msrs_to_save array and would be rejected by KVM_GET/SET_MSR.
To avoid this, cut the list to whatever CPUID reports for the host's
architectural PMU.
Reported-by: Vitaly Kuznetsov <[email protected]>
Suggested-by: Vitaly Kuznetsov <[email protected]>
Cc: Jim Mattson <[email protected]>
Fixes: e2ada66ec418 ("kvm: x86: Add Intel PMU MSRs to msrs_to_save[]", 2019-08-21)
Signed-off-by: Paolo Bonzini <[email protected]>
---
arch/x86/kvm/x86.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8072acaaf028..31607174f442 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5105,13 +5105,14 @@ long kvm_arch_vm_ioctl(struct file *filp,
static void kvm_init_msr_list(void)
{
+ struct x86_pmu_capability x86_pmu;
u32 dummy[2];
unsigned i, j;
BUILD_BUG_ON_MSG(INTEL_PMC_MAX_FIXED != 4,
"Please update the fixed PMCs in msrs_to_save[]");
- BUILD_BUG_ON_MSG(INTEL_PMC_MAX_GENERIC != 32,
- "Please update the generic perfctr/eventsel MSRs in msrs_to_save[]");
+
+ perf_get_x86_pmu_capability(&x86_pmu);
for (i = j = 0; i < ARRAY_SIZE(msrs_to_save); i++) {
if (rdmsr_safe(msrs_to_save[i], &dummy[0], &dummy[1]) < 0)
@@ -5153,6 +5154,15 @@ static void kvm_init_msr_list(void)
intel_pt_validate_hw_cap(PT_CAP_num_address_ranges) * 2)
continue;
break;
+ case MSR_ARCH_PERFMON_PERFCTR0 ... MSR_ARCH_PERFMON_PERFCTR0 + 31:
+ if (msrs_to_save[i] - 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 + 31:
+ if (msrs_to_save[i] - MSR_ARCH_PERFMON_EVENTSEL0 >=
+ min(INTEL_PMC_MAX_GENERIC, x86_pmu.num_counters_gp))
+ continue;
}
default:
break;
--
1.8.3.1
On Thu, Oct 3, 2019 at 3:10 AM Paolo Bonzini <[email protected]> wrote:
>
> INTEL_PMC_MAX_GENERIC is currently 32, which exceeds the 18 contiguous
> MSR indices reserved by Intel for event selectors. Since some machines
> actually have MSRs past the reserved range, these may survive the
Not past, but *within* the reserved range.
> filtering of msrs_to_save array and would be rejected by KVM_GET/SET_MSR.
> To avoid this, cut the list to whatever CPUID reports for the host's
> architectural PMU.
>
> Reported-by: Vitaly Kuznetsov <[email protected]>
> Suggested-by: Vitaly Kuznetsov <[email protected]>
> Cc: Jim Mattson <[email protected]>
> Fixes: e2ada66ec418 ("kvm: x86: Add Intel PMU MSRs to msrs_to_save[]", 2019-08-21)
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> arch/x86/kvm/x86.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 8072acaaf028..31607174f442 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5105,13 +5105,14 @@ long kvm_arch_vm_ioctl(struct file *filp,
>
> static void kvm_init_msr_list(void)
> {
> + struct x86_pmu_capability x86_pmu;
> u32 dummy[2];
> unsigned i, j;
>
> BUILD_BUG_ON_MSG(INTEL_PMC_MAX_FIXED != 4,
> "Please update the fixed PMCs in msrs_to_save[]");
> - BUILD_BUG_ON_MSG(INTEL_PMC_MAX_GENERIC != 32,
> - "Please update the generic perfctr/eventsel MSRs in msrs_to_save[]");
> +
> + perf_get_x86_pmu_capability(&x86_pmu);
>
> for (i = j = 0; i < ARRAY_SIZE(msrs_to_save); i++) {
> if (rdmsr_safe(msrs_to_save[i], &dummy[0], &dummy[1]) < 0)
> @@ -5153,6 +5154,15 @@ static void kvm_init_msr_list(void)
> intel_pt_validate_hw_cap(PT_CAP_num_address_ranges) * 2)
> continue;
> break;
> + case MSR_ARCH_PERFMON_PERFCTR0 ... MSR_ARCH_PERFMON_PERFCTR0 + 31:
You've truncated the list I originally provided, so I think this need
only go to MSR_ARCH_PERFMON_PERFCTR0 + 17. Otherwise, we could lose
some valuable MSRs.
> + if (msrs_to_save[i] - MSR_ARCH_PERFMON_PERFCTR0 >=
> + min(INTEL_PMC_MAX_GENERIC, x86_pmu.num_counters_gp))
Why involve INTEL_PMC_MAX_GENERIC here?
> + continue;
> + break;
> + case MSR_ARCH_PERFMON_EVENTSEL0 ... MSR_ARCH_PERFMON_EVENTSEL0 + 31:
Same as the two comments above.
> + if (msrs_to_save[i] - MSR_ARCH_PERFMON_EVENTSEL0 >=
> + min(INTEL_PMC_MAX_GENERIC, x86_pmu.num_counters_gp))
> + continue;
> }
> default:
> break;
> --
> 1.8.3.1
>
On 03/10/19 19:20, Jim Mattson wrote:
> You've truncated the list I originally provided, so I think this need
> only go to MSR_ARCH_PERFMON_PERFCTR0 + 17. Otherwise, we could lose
> some valuable MSRs.
This is v2, so it was meant to replace the patch that truncates the
list. But I can include the other one too, perhaps even ask the x86
maintainers about decreasing INTEL_PMC_MAX_GENERIC to 18.
>> + if (msrs_to_save[i] - MSR_ARCH_PERFMON_PERFCTR0 >=
>> + min(INTEL_PMC_MAX_GENERIC, x86_pmu.num_counters_gp))
> Why involve INTEL_PMC_MAX_GENERIC here?
It's not really necessary, but I wanted to imitate how intel_pmu_refresh
initializes pmu->nr_arch_gp_counters.
Paolo
>> + continue;
>> + break;
>> + case MSR_ARCH_PERFMON_EVENTSEL0 ... MSR_ARCH_PERFMON_EVENTSEL0 + 31:
> Same as the two comments above.
>> + if (msrs_to_save[i] - MSR_ARCH_PERFMON_EVENTSEL0 >=
>> + min(INTEL_PMC_MAX_GENERIC, x86_pmu.num_counters_gp))
>> + continue;
On Thu, Oct 3, 2019 at 10:38 AM Paolo Bonzini <[email protected]> wrote:
>
> On 03/10/19 19:20, Jim Mattson wrote:
> > You've truncated the list I originally provided, so I think this need
> > only go to MSR_ARCH_PERFMON_PERFCTR0 + 17. Otherwise, we could lose
> > some valuable MSRs.
>
> This is v2, so it was meant to replace the patch that truncates the
> list. But I can include the other one too, perhaps even ask the x86
> maintainers about decreasing INTEL_PMC_MAX_GENERIC to 18.
The list should definitely be truncated, since
MSR_ARCH_PERFMON_EVENTSEL0 + 18 is IA32_PERF_STATUS.
On 03/10/19 20:23, Jim Mattson wrote:
> On Thu, Oct 3, 2019 at 10:38 AM Paolo Bonzini <[email protected]> wrote:
>>
>> On 03/10/19 19:20, Jim Mattson wrote:
>>> You've truncated the list I originally provided, so I think this need
>>> only go to MSR_ARCH_PERFMON_PERFCTR0 + 17. Otherwise, we could lose
>>> some valuable MSRs.
>>
>> This is v2, so it was meant to replace the patch that truncates the
>> list. But I can include the other one too, perhaps even ask the x86
>> maintainers about decreasing INTEL_PMC_MAX_GENERIC to 18.
>
> The list should definitely be truncated, since
> MSR_ARCH_PERFMON_EVENTSEL0 + 18 is IA32_PERF_STATUS.
Ok, thanks.
Paolo