Re: [PATCH v3 5/5] KVM: selftests: Test the PMU event "Instructions retired"

[Sean Christopherson <seanjc@google.com>]

On Tue, Mar 07, 2023, Aaron Lewis wrote:
> @@ -71,6 +86,16 @@ static const uint64_t event_list[] = {
>  	AMD_ZEN_BR_RETIRED,
>  };
>  
> +struct perf_results {
> +	union {
> +		uint64_t raw;
> +		struct {
> +			uint64_t br_count:32;
> +			uint64_t ir_count:32;
> +		};
> +	};
> +};
> +
>  /*
>   * If we encounter a #GP during the guest PMU sanity check, then the guest
>   * PMU is not functional. Inform the hypervisor via GUEST_SYNC(0).
> @@ -102,13 +127,20 @@ static void check_msr(uint32_t msr, uint64_t bits_to_flip)
>  
>  static uint64_t test_guest(uint32_t msr_base)
>  {
> +	struct perf_results r;
>  	uint64_t br0, br1;
> +	uint64_t ir0, ir1;
>  
>  	br0 = rdmsr(msr_base + 0);
> +	ir0 = rdmsr(msr_base + 1);
>  	__asm__ __volatile__("loop ." : "+c"((int){NUM_BRANCHES}));
>  	br1 = rdmsr(msr_base + 0);
> +	ir1 = rdmsr(msr_base + 1);
>  
> -	return br1 - br0;
> +	r.br_count = br1 - br0;
> +	r.ir_count = ir1 - ir0;

The struct+union is problematic on 2+ fronts.  I don't like that it truncates
a 64-bit MSR value into a 32-bit field.  And this test now ends up with two
structs (perf_results and perf_counter) that serve the same purpose, but count
different events, and without any indiciation in the name _what_ event(s) the
struct tracks.

The existing "struct perf_count" has the same truncation problem.  It _shouldn't_
cause problems, but unless I'm missing something, it means that, very theoretically,
the test could have false passes, e.g. if KVM manages to corrupt the upper 32 bits.

There's really no reason to limit ourselves to 64 bits of data, as the selftests
framework provides helpers to copy arbitrary structs to/from the guest.  If we
throw all of the counts into a single struct, then we solve the naming issue and
can provide a helper to do the copies to/from the guest.

I have all of this typed up and it appears to work.  I'll post a new version of
just the selftests patches.