Re: [kvm-unit-tests PATCH v3 12/13] x86/pmu: Add assignment framework for Intel-specific HW resources

[Sean Christopherson <seanjc@google.com>]

On Fri, Aug 19, 2022, Like Xu wrote:
> @@ -65,7 +65,13 @@ struct pmu_event {
>  };
>  
>  #define PMU_CAP_FW_WRITES	(1ULL << 13)
> -static u64 gp_counter_base = MSR_IA32_PERFCTR0;
> +static u32 gp_counter_base;
> +static u32 gp_select_base;
> +static unsigned int gp_events_size;
> +static unsigned int nr_gp_counters;
> +
> +typedef struct pmu_event PMU_EVENTS_ARRAY_t[];
> +static PMU_EVENTS_ARRAY_t *gp_events = NULL;

There's no need for a layer of indirection, C d.  The NULL is also not strictly
required.  E.g. this should Just Work.

  static struct pmu_event *gp_events;

And then I beleve a large number of changes in this series go away.

>  char *buf;
>  
> @@ -114,9 +120,9 @@ static struct pmu_event* get_counter_event(pmu_counter_t *cnt)
>  	if (is_gp(cnt)) {
>  		int i;
>  
> -		for (i = 0; i < sizeof(gp_events)/sizeof(gp_events[0]); i++)
> -			if (gp_events[i].unit_sel == (cnt->config & 0xffff))
> -				return &gp_events[i];
> +		for (i = 0; i < gp_events_size; i++)
> +			if ((*gp_events)[i].unit_sel == (cnt->config & 0xffff))
> +				return &(*gp_events)[i];
>  	} else
>  		return &fixed_events[cnt->ctr - MSR_CORE_PERF_FIXED_CTR0];
>  
> @@ -142,12 +148,22 @@ static void global_disable(pmu_counter_t *cnt)
>  			~(1ull << cnt->idx));
>  }
>  
> +static inline uint32_t get_gp_counter_msr(unsigned int i)

Rather than helpers, what about macros?  The problem with "get" is that it sounds
like the helper is actually reading the counter/MSR.  E.g. see MSR_IA32_MCx_CTL()

Something like this?

  MSR_PERF_GP_CTRx()

> +{
> +	return gp_counter_base + i;
> +}
> +
> +static inline uint32_t get_gp_select_msr(unsigned int i)
> +{
> +	return gp_select_base + i;

Same here, maybe?

  MSR_PERF_GP_SELECTx()