diff mbox series

[v7,04/19] KVM: x86/pmu: Setup fixed counters' eventsel during PMU initialization

Message ID 20231108003135.546002-5-seanjc@google.com
State New
Headers
Series KVM: x86/pmu: selftests: Fixes and new tests |

Commit Message

Sean Christopherson Nov. 8, 2023, 12:31 a.m. UTC 
  Set the eventsel for all fixed counters during PMU initialization, the
eventsel is hardcoded and consumed if and only if the counter is supported,
i.e. there is no reason to redo the setup every time the PMU is refreshed.

Configuring all KVM-supported fixed counter also eliminates a potential
pitfall if/when KVM supports discontiguous fixed counters, in which case
configuring only nr_arch_fixed_counters will be insufficient (ignoring the
fact that KVM will need many other changes to support discontiguous fixed
counters).

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/pmu_intel.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

Comments

Jim Mattson Nov. 8, 2023, 1:28 a.m. UTC | #1 
On Tue, Nov 7, 2023 at 4:31 PM Sean Christopherson <seanjc@google.com> wrote:
>
> Set the eventsel for all fixed counters during PMU initialization, the
> eventsel is hardcoded and consumed if and only if the counter is supported,
> i.e. there is no reason to redo the setup every time the PMU is refreshed.
>
> Configuring all KVM-supported fixed counter also eliminates a potential
> pitfall if/when KVM supports discontiguous fixed counters, in which case
> configuring only nr_arch_fixed_counters will be insufficient (ignoring the
> fact that KVM will need many other changes to support discontiguous fixed
> counters).
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/vmx/pmu_intel.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
> index c4f2c6a268e7..5fc5a62af428 100644
> --- a/arch/x86/kvm/vmx/pmu_intel.c
> +++ b/arch/x86/kvm/vmx/pmu_intel.c
> @@ -409,7 +409,7 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>   * Note, reference cycles is counted using a perf-defined "psuedo-encoding",
>   * as there is no architectural general purpose encoding for reference cycles.
>   */
> -static void setup_fixed_pmc_eventsel(struct kvm_pmu *pmu)
> +static u64 intel_get_fixed_pmc_eventsel(int index)
>  {
>         const struct {
>                 u8 eventsel;
> @@ -419,17 +419,11 @@ static void setup_fixed_pmc_eventsel(struct kvm_pmu *pmu)
>                 [1] = { 0x3c, 0x00 }, /* CPU Cycles/ PERF_COUNT_HW_CPU_CYCLES. */
>                 [2] = { 0x00, 0x03 }, /* Reference Cycles / PERF_COUNT_HW_REF_CPU_CYCLES*/
>         };
> -       int i;
>
>         BUILD_BUG_ON(ARRAY_SIZE(fixed_pmc_events) != KVM_PMC_MAX_FIXED);
>
> -       for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
> -               int index = array_index_nospec(i, KVM_PMC_MAX_FIXED);
> -               struct kvm_pmc *pmc = &pmu->fixed_counters[index];
> -
> -               pmc->eventsel = (fixed_pmc_events[index].unit_mask << 8) |
> -                                fixed_pmc_events[index].eventsel;
> -       }
> +       return (fixed_pmc_events[index].unit_mask << 8) |
> +               fixed_pmc_events[index].eventsel;

Can I just say that it's really confusing that the value returned by
intel_get_fixed_pmc_eventsel() is the concatenation of an 8-bit "unit
mask" and an 8-bit "eventsel"?
Sean Christopherson Nov. 8, 2023, 2:39 p.m. UTC | #2 
On Tue, Nov 07, 2023, Jim Mattson wrote:
> On Tue, Nov 7, 2023 at 4:31 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > Set the eventsel for all fixed counters during PMU initialization, the
> > eventsel is hardcoded and consumed if and only if the counter is supported,
> > i.e. there is no reason to redo the setup every time the PMU is refreshed.
> >
> > Configuring all KVM-supported fixed counter also eliminates a potential
> > pitfall if/when KVM supports discontiguous fixed counters, in which case
> > configuring only nr_arch_fixed_counters will be insufficient (ignoring the
> > fact that KVM will need many other changes to support discontiguous fixed
> > counters).
> >
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  arch/x86/kvm/vmx/pmu_intel.c | 14 ++++----------
> >  1 file changed, 4 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
> > index c4f2c6a268e7..5fc5a62af428 100644
> > --- a/arch/x86/kvm/vmx/pmu_intel.c
> > +++ b/arch/x86/kvm/vmx/pmu_intel.c
> > @@ -409,7 +409,7 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >   * Note, reference cycles is counted using a perf-defined "psuedo-encoding",
> >   * as there is no architectural general purpose encoding for reference cycles.
> >   */
> > -static void setup_fixed_pmc_eventsel(struct kvm_pmu *pmu)
> > +static u64 intel_get_fixed_pmc_eventsel(int index)
> >  {
> >         const struct {
> >                 u8 eventsel;
> > @@ -419,17 +419,11 @@ static void setup_fixed_pmc_eventsel(struct kvm_pmu *pmu)
> >                 [1] = { 0x3c, 0x00 }, /* CPU Cycles/ PERF_COUNT_HW_CPU_CYCLES. */
> >                 [2] = { 0x00, 0x03 }, /* Reference Cycles / PERF_COUNT_HW_REF_CPU_CYCLES*/
> >         };
> > -       int i;
> >
> >         BUILD_BUG_ON(ARRAY_SIZE(fixed_pmc_events) != KVM_PMC_MAX_FIXED);
> >
> > -       for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
> > -               int index = array_index_nospec(i, KVM_PMC_MAX_FIXED);
> > -               struct kvm_pmc *pmc = &pmu->fixed_counters[index];
> > -
> > -               pmc->eventsel = (fixed_pmc_events[index].unit_mask << 8) |
> > -                                fixed_pmc_events[index].eventsel;
> > -       }
> > +       return (fixed_pmc_events[index].unit_mask << 8) |
> > +               fixed_pmc_events[index].eventsel;
> 
> Can I just say that it's really confusing that the value returned by
> intel_get_fixed_pmc_eventsel() is the concatenation of an 8-bit "unit
> mask" and an 8-bit "eventsel"?

Heh, blame the SDM for having an "event select" field in "event select" MSRs.

Is this better?

	const struct {
		u8 event;
		u8 unit_mask;
	} fixed_pmc_events[] = {
		[0] = { 0xc0, 0x00 }, /* Instruction Retired / PERF_COUNT_HW_INSTRUCTIONS. */
		[1] = { 0x3c, 0x00 }, /* CPU Cycles/ PERF_COUNT_HW_CPU_CYCLES. */
		[2] = { 0x00, 0x03 }, /* Reference Cycles / PERF_COUNT_HW_REF_CPU_CYCLES*/
	};

	BUILD_BUG_ON(ARRAY_SIZE(fixed_pmc_events) != KVM_PMC_MAX_FIXED);

	return (fixed_pmc_events[index].unit_mask << 8) |
		fixed_pmc_events[index].event;


Or are you complaining about the fact that they're split at all?  I'm open to any
format, though I personally found the seperate umask and event values helpful
when trying to understand what's going on.
Jim Mattson Nov. 8, 2023, 7 p.m. UTC | #3 
On Wed, Nov 8, 2023 at 6:39 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Nov 07, 2023, Jim Mattson wrote:
> > On Tue, Nov 7, 2023 at 4:31 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > Set the eventsel for all fixed counters during PMU initialization, the
> > > eventsel is hardcoded and consumed if and only if the counter is supported,
> > > i.e. there is no reason to redo the setup every time the PMU is refreshed.
> > >
> > > Configuring all KVM-supported fixed counter also eliminates a potential
> > > pitfall if/when KVM supports discontiguous fixed counters, in which case
> > > configuring only nr_arch_fixed_counters will be insufficient (ignoring the
> > > fact that KVM will need many other changes to support discontiguous fixed
> > > counters).
> > >
> > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > > ---
> > >  arch/x86/kvm/vmx/pmu_intel.c | 14 ++++----------
> > >  1 file changed, 4 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
> > > index c4f2c6a268e7..5fc5a62af428 100644
> > > --- a/arch/x86/kvm/vmx/pmu_intel.c
> > > +++ b/arch/x86/kvm/vmx/pmu_intel.c
> > > @@ -409,7 +409,7 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > >   * Note, reference cycles is counted using a perf-defined "psuedo-encoding",
> > >   * as there is no architectural general purpose encoding for reference cycles.
> > >   */
> > > -static void setup_fixed_pmc_eventsel(struct kvm_pmu *pmu)
> > > +static u64 intel_get_fixed_pmc_eventsel(int index)
> > >  {
> > >         const struct {
> > >                 u8 eventsel;
> > > @@ -419,17 +419,11 @@ static void setup_fixed_pmc_eventsel(struct kvm_pmu *pmu)
> > >                 [1] = { 0x3c, 0x00 }, /* CPU Cycles/ PERF_COUNT_HW_CPU_CYCLES. */
> > >                 [2] = { 0x00, 0x03 }, /* Reference Cycles / PERF_COUNT_HW_REF_CPU_CYCLES*/
> > >         };
> > > -       int i;
> > >
> > >         BUILD_BUG_ON(ARRAY_SIZE(fixed_pmc_events) != KVM_PMC_MAX_FIXED);
> > >
> > > -       for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
> > > -               int index = array_index_nospec(i, KVM_PMC_MAX_FIXED);
> > > -               struct kvm_pmc *pmc = &pmu->fixed_counters[index];
> > > -
> > > -               pmc->eventsel = (fixed_pmc_events[index].unit_mask << 8) |
> > > -                                fixed_pmc_events[index].eventsel;
> > > -       }
> > > +       return (fixed_pmc_events[index].unit_mask << 8) |
> > > +               fixed_pmc_events[index].eventsel;
> >
> > Can I just say that it's really confusing that the value returned by
> > intel_get_fixed_pmc_eventsel() is the concatenation of an 8-bit "unit
> > mask" and an 8-bit "eventsel"?
>
> Heh, blame the SDM for having an "event select" field in "event select" MSRs.
>
> Is this better?
>
>         const struct {
>                 u8 event;
>                 u8 unit_mask;
>         } fixed_pmc_events[] = {
>                 [0] = { 0xc0, 0x00 }, /* Instruction Retired / PERF_COUNT_HW_INSTRUCTIONS. */
>                 [1] = { 0x3c, 0x00 }, /* CPU Cycles/ PERF_COUNT_HW_CPU_CYCLES. */
>                 [2] = { 0x00, 0x03 }, /* Reference Cycles / PERF_COUNT_HW_REF_CPU_CYCLES*/
>         };

Better. Thank you.
diff mbox series

Patch

diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index c4f2c6a268e7..5fc5a62af428 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -409,7 +409,7 @@  static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
  * Note, reference cycles is counted using a perf-defined "psuedo-encoding",
  * as there is no architectural general purpose encoding for reference cycles.
  */
-static void setup_fixed_pmc_eventsel(struct kvm_pmu *pmu)
+static u64 intel_get_fixed_pmc_eventsel(int index)
 {
 	const struct {
 		u8 eventsel;
@@ -419,17 +419,11 @@  static void setup_fixed_pmc_eventsel(struct kvm_pmu *pmu)
 		[1] = { 0x3c, 0x00 }, /* CPU Cycles/ PERF_COUNT_HW_CPU_CYCLES. */
 		[2] = { 0x00, 0x03 }, /* Reference Cycles / PERF_COUNT_HW_REF_CPU_CYCLES*/
 	};
-	int i;
 
 	BUILD_BUG_ON(ARRAY_SIZE(fixed_pmc_events) != KVM_PMC_MAX_FIXED);
 
-	for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
-		int index = array_index_nospec(i, KVM_PMC_MAX_FIXED);
-		struct kvm_pmc *pmc = &pmu->fixed_counters[index];
-
-		pmc->eventsel = (fixed_pmc_events[index].unit_mask << 8) |
-				 fixed_pmc_events[index].eventsel;
-	}
+	return (fixed_pmc_events[index].unit_mask << 8) |
+		fixed_pmc_events[index].eventsel;
 }
 
 static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
@@ -495,7 +489,6 @@  static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
 						  kvm_pmu_cap.bit_width_fixed);
 		pmu->counter_bitmask[KVM_PMC_FIXED] =
 			((u64)1 << edx.split.bit_width_fixed) - 1;
-		setup_fixed_pmc_eventsel(pmu);
 	}
 
 	for (i = 0; i < pmu->nr_arch_fixed_counters; i++)
@@ -573,6 +566,7 @@  static void intel_pmu_init(struct kvm_vcpu *vcpu)
 		pmu->fixed_counters[i].vcpu = vcpu;
 		pmu->fixed_counters[i].idx = i + INTEL_PMC_IDX_FIXED;
 		pmu->fixed_counters[i].current_config = 0;
+		pmu->fixed_counters[i].eventsel = intel_get_fixed_pmc_eventsel(i);
 	}
 
 	lbr_desc->records.nr = 0;