* [PATCH] kvm: vmx: Limit guest PMCs to those supported on the host
@ 2019-09-30 23:38 Jim Mattson
2019-10-01 11:32 ` Vitaly Kuznetsov
0 siblings, 1 reply; 6+ messages in thread
From: Jim Mattson @ 2019-09-30 23:38 UTC (permalink / raw)
To: kvm, Sean Christopherson, Paolo Bonzini; +Cc: Jim Mattson, Marc Orr
KVM can only virtualize as many PMCs as the host supports.
Limit the number of generic counters and fixed counters to the number
of corresponding counters supported on the host, rather than to
INTEL_PMC_MAX_GENERIC and INTEL_PMC_MAX_FIXED, respectively.
Note that INTEL_PMC_MAX_GENERIC is currently 32, which exceeds the 18
contiguous MSR indices reserved by Intel for event selectors. Since
the existing code relies on a contiguous range of MSR indices for
event selectors, it can't possibly work for more than 18 general
purpose counters.
Fixes: f5132b01386b5a ("KVM: Expose a version 2 architectural PMU to a guests")
Signed-off-by: Jim Mattson <jmattson@google.com>
Reviewed-by: Marc Orr <marcorr@google.com>
---
arch/x86/kvm/vmx/pmu_intel.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 4dea0e0e7e392..3e9c059099e94 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -262,6 +262,7 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
{
struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
+ struct x86_pmu_capability x86_pmu;
struct kvm_cpuid_entry2 *entry;
union cpuid10_eax eax;
union cpuid10_edx edx;
@@ -283,8 +284,10 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
if (!pmu->version)
return;
+ perf_get_x86_pmu_capability(&x86_pmu);
+
pmu->nr_arch_gp_counters = min_t(int, eax.split.num_counters,
- INTEL_PMC_MAX_GENERIC);
+ x86_pmu.num_counters_gp);
pmu->counter_bitmask[KVM_PMC_GP] = ((u64)1 << eax.split.bit_width) - 1;
pmu->available_event_types = ~entry->ebx &
((1ull << eax.split.mask_length) - 1);
@@ -294,7 +297,7 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
} else {
pmu->nr_arch_fixed_counters =
min_t(int, edx.split.num_counters_fixed,
- INTEL_PMC_MAX_FIXED);
+ x86_pmu.num_counters_fixed);
pmu->counter_bitmask[KVM_PMC_FIXED] =
((u64)1 << edx.split.bit_width_fixed) - 1;
}
--
2.23.0.444.g18eeb5a265-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] kvm: vmx: Limit guest PMCs to those supported on the host
2019-09-30 23:38 [PATCH] kvm: vmx: Limit guest PMCs to those supported on the host Jim Mattson
@ 2019-10-01 11:32 ` Vitaly Kuznetsov
2019-10-01 13:28 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Vitaly Kuznetsov @ 2019-10-01 11:32 UTC (permalink / raw)
To: Jim Mattson; +Cc: Marc Orr, kvm, Sean Christopherson, Paolo Bonzini
Jim Mattson <jmattson@google.com> writes:
> KVM can only virtualize as many PMCs as the host supports.
>
> Limit the number of generic counters and fixed counters to the number
> of corresponding counters supported on the host, rather than to
> INTEL_PMC_MAX_GENERIC and INTEL_PMC_MAX_FIXED, respectively.
>
> Note that INTEL_PMC_MAX_GENERIC is currently 32, which exceeds the 18
> contiguous MSR indices reserved by Intel for event selectors. Since
> the existing code relies on a contiguous range of MSR indices for
> event selectors, it can't possibly work for more than 18 general
> purpose counters.
Should we also trim msrs_to_save[] by removing impossible entries
(18-31) then?
>
> Fixes: f5132b01386b5a ("KVM: Expose a version 2 architectural PMU to a guests")
> Signed-off-by: Jim Mattson <jmattson@google.com>
> Reviewed-by: Marc Orr <marcorr@google.com>
> ---
> arch/x86/kvm/vmx/pmu_intel.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
> index 4dea0e0e7e392..3e9c059099e94 100644
> --- a/arch/x86/kvm/vmx/pmu_intel.c
> +++ b/arch/x86/kvm/vmx/pmu_intel.c
> @@ -262,6 +262,7 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> {
> struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
> + struct x86_pmu_capability x86_pmu;
> struct kvm_cpuid_entry2 *entry;
> union cpuid10_eax eax;
> union cpuid10_edx edx;
> @@ -283,8 +284,10 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> if (!pmu->version)
> return;
>
> + perf_get_x86_pmu_capability(&x86_pmu);
> +
> pmu->nr_arch_gp_counters = min_t(int, eax.split.num_counters,
> - INTEL_PMC_MAX_GENERIC);
> + x86_pmu.num_counters_gp);
This is a theoretical fix which is orthogonal to the issue with
state_test I reported on Friday, right? Because in my case
'eax.split.num_counters' is already 8.
> pmu->counter_bitmask[KVM_PMC_GP] = ((u64)1 << eax.split.bit_width) - 1;
> pmu->available_event_types = ~entry->ebx &
> ((1ull << eax.split.mask_length) - 1);
> @@ -294,7 +297,7 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
> } else {
> pmu->nr_arch_fixed_counters =
> min_t(int, edx.split.num_counters_fixed,
> - INTEL_PMC_MAX_FIXED);
> + x86_pmu.num_counters_fixed);
> pmu->counter_bitmask[KVM_PMC_FIXED] =
> ((u64)1 << edx.split.bit_width_fixed) - 1;
> }
--
Vitaly
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kvm: vmx: Limit guest PMCs to those supported on the host
2019-10-01 11:32 ` Vitaly Kuznetsov
@ 2019-10-01 13:28 ` Paolo Bonzini
2019-10-01 14:07 ` Jim Mattson
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2019-10-01 13:28 UTC (permalink / raw)
To: Vitaly Kuznetsov, Jim Mattson; +Cc: Marc Orr, kvm, Sean Christopherson
On 01/10/19 13:32, Vitaly Kuznetsov wrote:
> Jim Mattson <jmattson@google.com> writes:
>
>> KVM can only virtualize as many PMCs as the host supports.
>>
>> Limit the number of generic counters and fixed counters to the number
>> of corresponding counters supported on the host, rather than to
>> INTEL_PMC_MAX_GENERIC and INTEL_PMC_MAX_FIXED, respectively.
>>
>> Note that INTEL_PMC_MAX_GENERIC is currently 32, which exceeds the 18
>> contiguous MSR indices reserved by Intel for event selectors. Since
>> the existing code relies on a contiguous range of MSR indices for
>> event selectors, it can't possibly work for more than 18 general
>> purpose counters.
>
> Should we also trim msrs_to_save[] by removing impossible entries
> (18-31) then?
Yes, I'll send a patch in a second.
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kvm: vmx: Limit guest PMCs to those supported on the host
2019-10-01 13:28 ` Paolo Bonzini
@ 2019-10-01 14:07 ` Jim Mattson
2019-10-01 14:24 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Jim Mattson @ 2019-10-01 14:07 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Vitaly Kuznetsov, Marc Orr, kvm list, Sean Christopherson
On Tue, Oct 1, 2019 at 6:29 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 01/10/19 13:32, Vitaly Kuznetsov wrote:
> > Jim Mattson <jmattson@google.com> writes:
> >
> >> KVM can only virtualize as many PMCs as the host supports.
> >>
> >> Limit the number of generic counters and fixed counters to the number
> >> of corresponding counters supported on the host, rather than to
> >> INTEL_PMC_MAX_GENERIC and INTEL_PMC_MAX_FIXED, respectively.
> >>
> >> Note that INTEL_PMC_MAX_GENERIC is currently 32, which exceeds the 18
> >> contiguous MSR indices reserved by Intel for event selectors. Since
> >> the existing code relies on a contiguous range of MSR indices for
> >> event selectors, it can't possibly work for more than 18 general
> >> purpose counters.
> >
> > Should we also trim msrs_to_save[] by removing impossible entries
> > (18-31) then?
>
> Yes, I'll send a patch in a second.
>
> Paolo
I thought you were going to revert that msrs_to_save patch. I've been
working on a replacement.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kvm: vmx: Limit guest PMCs to those supported on the host
2019-10-01 14:07 ` Jim Mattson
@ 2019-10-01 14:24 ` Paolo Bonzini
2019-10-01 14:30 ` Jim Mattson
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2019-10-01 14:24 UTC (permalink / raw)
To: Jim Mattson; +Cc: Vitaly Kuznetsov, Marc Orr, kvm list, Sean Christopherson
On 01/10/19 16:07, Jim Mattson wrote:
> On Tue, Oct 1, 2019 at 6:29 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 01/10/19 13:32, Vitaly Kuznetsov wrote:
>>> Jim Mattson <jmattson@google.com> writes:
>>>
>>>> KVM can only virtualize as many PMCs as the host supports.
>>>>
>>>> Limit the number of generic counters and fixed counters to the number
>>>> of corresponding counters supported on the host, rather than to
>>>> INTEL_PMC_MAX_GENERIC and INTEL_PMC_MAX_FIXED, respectively.
>>>>
>>>> Note that INTEL_PMC_MAX_GENERIC is currently 32, which exceeds the 18
>>>> contiguous MSR indices reserved by Intel for event selectors. Since
>>>> the existing code relies on a contiguous range of MSR indices for
>>>> event selectors, it can't possibly work for more than 18 general
>>>> purpose counters.
>>>
>>> Should we also trim msrs_to_save[] by removing impossible entries
>>> (18-31) then?
>>
>> Yes, I'll send a patch in a second.
>
> I thought you were going to revert that msrs_to_save patch. I've been
> working on a replacement.
We can use a little more time to think more about it and discuss it.
For example, trimming is enough for the basic usage of passing
KVM_SET_SUPPORTED_CPUID output to KVM_SET_CPUID2 and then retrieving all
MSRs in the list. If that is also okay for Google's userspace, we might
actually leave everything that way and retroactively decide that you
need to filter the MSRs but only if you pass your own CPUID.
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kvm: vmx: Limit guest PMCs to those supported on the host
2019-10-01 14:24 ` Paolo Bonzini
@ 2019-10-01 14:30 ` Jim Mattson
0 siblings, 0 replies; 6+ messages in thread
From: Jim Mattson @ 2019-10-01 14:30 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Vitaly Kuznetsov, Marc Orr, kvm list, Sean Christopherson
On Tue, Oct 1, 2019 at 7:24 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 01/10/19 16:07, Jim Mattson wrote:
> > On Tue, Oct 1, 2019 at 6:29 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>
> >> On 01/10/19 13:32, Vitaly Kuznetsov wrote:
> >>> Jim Mattson <jmattson@google.com> writes:
> >>>
> >>>> KVM can only virtualize as many PMCs as the host supports.
> >>>>
> >>>> Limit the number of generic counters and fixed counters to the number
> >>>> of corresponding counters supported on the host, rather than to
> >>>> INTEL_PMC_MAX_GENERIC and INTEL_PMC_MAX_FIXED, respectively.
> >>>>
> >>>> Note that INTEL_PMC_MAX_GENERIC is currently 32, which exceeds the 18
> >>>> contiguous MSR indices reserved by Intel for event selectors. Since
> >>>> the existing code relies on a contiguous range of MSR indices for
> >>>> event selectors, it can't possibly work for more than 18 general
> >>>> purpose counters.
> >>>
> >>> Should we also trim msrs_to_save[] by removing impossible entries
> >>> (18-31) then?
> >>
> >> Yes, I'll send a patch in a second.
> >
> > I thought you were going to revert that msrs_to_save patch. I've been
> > working on a replacement.
>
> We can use a little more time to think more about it and discuss it.
>
> For example, trimming is enough for the basic usage of passing
> KVM_SET_SUPPORTED_CPUID output to KVM_SET_CPUID2 and then retrieving all
> MSRs in the list. If that is also okay for Google's userspace, we might
> actually leave everything that way and retroactively decide that you
> need to filter the MSRs but only if you pass your own CPUID.
>
> Paolo
If just trimming the static list, remember to trim to even less than
18, since Intel has used one of the reserved MSRs following the event
selectors for something else. I was going to follow Sean's suggestion
and specifically enumerate all of the PMU MSRs based on CPUID 0AH.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-10-01 14:30 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-30 23:38 [PATCH] kvm: vmx: Limit guest PMCs to those supported on the host Jim Mattson
2019-10-01 11:32 ` Vitaly Kuznetsov
2019-10-01 13:28 ` Paolo Bonzini
2019-10-01 14:07 ` Jim Mattson
2019-10-01 14:24 ` Paolo Bonzini
2019-10-01 14:30 ` Jim Mattson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.