Received-SPF: pass (google.com: domain of linux-kernel+bounces-120356-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Wed, 27 Mar 2024 06:07:57 +0000
From: Mingwei Zhang <mizhang@google.com>
To: Dapeng Mi <dapeng1.mi@linux.intel.com>
Cc: Sean Christopherson <seanjc@google.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Jim Mattson <jmattson@google.com>, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org, Zhenyu Wang <zhenyuw@linux.intel.com>,
	Zhang Xiong <xiong.y.zhang@intel.com>,
	Like Xu <like.xu.linux@gmail.com>,
	Jinrong Liang <cloudliang@tencent.com>,
	Dapeng Mi <dapeng1.mi@intel.com>
Subject: Re: [kvm-unit-tests Patch v3 07/11] x86: pmu: Enable and disable
 PMCs in loop() asm blob
Message-ID: <ZgO3vWIeC3sk_B5N@google.com>
References: <20240103031409.2504051-1-dapeng1.mi@linux.intel.com>
 <20240103031409.2504051-8-dapeng1.mi@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240103031409.2504051-8-dapeng1.mi@linux.intel.com>

On Wed, Jan 03, 2024, Dapeng Mi wrote:
> Currently enabling PMCs, executing loop() and disabling PMCs are divided
> 3 separated functions. So there could be other instructions executed
> between enabling PMCS and running loop() or running loop() and disabling
> PMCs, e.g. if there are multiple counters enabled in measure_many()
> function, the instructions which enabling the 2nd and more counters
> would be counted in by the 1st counter.
> 
> So current implementation can only verify the correctness of count by an
> rough range rather than a precise count even for instructions and
> branches events. Strictly speaking, this verification is meaningless as
> the test could still pass even though KVM vPMU has something wrong and
> reports an incorrect instructions or branches count which is in the rough
> range.
> 
> Thus, move the PMCs enabling and disabling into the loop() asm blob and
> ensure only the loop asm instructions would be counted, then the
> instructions or branches events can be verified with an precise count
> instead of an rough range.
> 
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>

> ---
>  x86/pmu.c | 83 +++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 69 insertions(+), 14 deletions(-)
> 
> diff --git a/x86/pmu.c b/x86/pmu.c
> index 46bed66c5c9f..88b89ad889b9 100644
> --- a/x86/pmu.c
> +++ b/x86/pmu.c
> @@ -18,6 +18,20 @@
>  #define EXPECTED_INSTR 17
>  #define EXPECTED_BRNCH 5
>  
> +// Instrustion number of LOOP_ASM code
> +#define LOOP_INSTRNS	10
> +#define LOOP_ASM					\
> +	"1: mov (%1), %2; add $64, %1;\n\t"		\
> +	"nop; nop; nop; nop; nop; nop; nop;\n\t"	\
> +	"loop 1b;\n\t"
> +
> +#define PRECISE_LOOP_ASM						\
> +	"wrmsr;\n\t"							\
> +	"mov %%ecx, %%edi; mov %%ebx, %%ecx;\n\t"			\
> +	LOOP_ASM							\
> +	"mov %%edi, %%ecx; xor %%eax, %%eax; xor %%edx, %%edx;\n\t"	\
> +	"wrmsr;\n\t"

Can we add "FEP" prefix into the above blob? This way, we can expand the
testing for emulated instructions.
> +
>  typedef struct {
>  	uint32_t ctr;
>  	uint64_t config;
> @@ -54,13 +68,43 @@ char *buf;
>  static struct pmu_event *gp_events;
>  static unsigned int gp_events_size;
>  
> -static inline void loop(void)
> +
> +static inline void __loop(void)
> +{
> +	unsigned long tmp, tmp2, tmp3;
> +
> +	asm volatile(LOOP_ASM
> +		     : "=c"(tmp), "=r"(tmp2), "=r"(tmp3)
> +		     : "0"(N), "1"(buf));
> +}
> +
> +/*
> + * Enable and disable counters in a whole asm blob to ensure
> + * no other instructions are counted in the time slot between
> + * counters enabling and really LOOP_ASM code executing.
> + * Thus counters can verify instructions and branches events
> + * against precise counts instead of a rough valid count range.
> + */
> +static inline void __precise_count_loop(u64 cntrs)
>  {
>  	unsigned long tmp, tmp2, tmp3;
> +	unsigned int global_ctl = pmu.msr_global_ctl;
> +	u32 eax = cntrs & (BIT_ULL(32) - 1);
> +	u32 edx = cntrs >> 32;
>  
> -	asm volatile("1: mov (%1), %2; add $64, %1; nop; nop; nop; nop; nop; nop; nop; loop 1b"
> -			: "=c"(tmp), "=r"(tmp2), "=r"(tmp3): "0"(N), "1"(buf));
> +	asm volatile(PRECISE_LOOP_ASM
> +		     : "=b"(tmp), "=r"(tmp2), "=r"(tmp3)
> +		     : "a"(eax), "d"(edx), "c"(global_ctl),
> +		       "0"(N), "1"(buf)
> +		     : "edi");
> +}
>  
> +static inline void loop(u64 cntrs)
> +{
> +	if (!this_cpu_has_perf_global_ctrl())
> +		__loop();
> +	else
> +		__precise_count_loop(cntrs);
>  }
>  
>  volatile uint64_t irq_received;
> @@ -159,18 +203,17 @@ static void __start_event(pmu_counter_t *evt, uint64_t count)
>  	    ctrl = (ctrl & ~(0xf << shift)) | (usrospmi << shift);
>  	    wrmsr(MSR_CORE_PERF_FIXED_CTR_CTRL, ctrl);
>      }
> -    global_enable(evt);
>      apic_write(APIC_LVTPC, PMI_VECTOR);
>  }
>  
>  static void start_event(pmu_counter_t *evt)
>  {
>  	__start_event(evt, 0);
> +	global_enable(evt);
>  }
>  
> -static void stop_event(pmu_counter_t *evt)
> +static void __stop_event(pmu_counter_t *evt)
>  {
> -	global_disable(evt);
>  	if (is_gp(evt)) {
>  		wrmsr(MSR_GP_EVENT_SELECTx(event_to_global_idx(evt)),
>  		      evt->config & ~EVNTSEL_EN);
> @@ -182,14 +225,24 @@ static void stop_event(pmu_counter_t *evt)
>  	evt->count = rdmsr(evt->ctr);
>  }
>  
> +static void stop_event(pmu_counter_t *evt)
> +{
> +	global_disable(evt);
> +	__stop_event(evt);
> +}
> +
>  static noinline void measure_many(pmu_counter_t *evt, int count)
>  {
>  	int i;
> +	u64 cntrs = 0;
> +
> +	for (i = 0; i < count; i++) {
> +		__start_event(&evt[i], 0);
> +		cntrs |= BIT_ULL(event_to_global_idx(&evt[i]));
> +	}
> +	loop(cntrs);
>  	for (i = 0; i < count; i++)
> -		start_event(&evt[i]);
> -	loop();
> -	for (i = 0; i < count; i++)
> -		stop_event(&evt[i]);
> +		__stop_event(&evt[i]);
>  }
>  
>  static void measure_one(pmu_counter_t *evt)
> @@ -199,9 +252,11 @@ static void measure_one(pmu_counter_t *evt)
>  
>  static noinline void __measure(pmu_counter_t *evt, uint64_t count)
>  {
> +	u64 cntrs = BIT_ULL(event_to_global_idx(evt));
> +
>  	__start_event(evt, count);
> -	loop();
> -	stop_event(evt);
> +	loop(cntrs);
> +	__stop_event(evt);
>  }
>  
>  static bool verify_event(uint64_t count, struct pmu_event *e)
> @@ -451,7 +506,7 @@ static void check_running_counter_wrmsr(void)
>  	report_prefix_push("running counter wrmsr");
>  
>  	start_event(&evt);
> -	loop();
> +	__loop();
>  	wrmsr(MSR_GP_COUNTERx(0), 0);
>  	stop_event(&evt);
>  	report(evt.count < gp_events[0].min, "cntr");
> @@ -468,7 +523,7 @@ static void check_running_counter_wrmsr(void)
>  
>  	wrmsr(MSR_GP_COUNTERx(0), count);
>  
> -	loop();
> +	__loop();
>  	stop_event(&evt);
>  
>  	if (this_cpu_has_perf_global_status()) {
> -- 
> 2.34.1
>