Re: [PATCH v3 05/11] KVM: selftests: Test consistency of CPUID with num of gp counters

[Sean Christopherson <seanjc@xxxxxxxxxx>]

On Thu, Aug 17, 2023, Sean Christopherson wrote:
> On Mon, Aug 14, 2023, Jinrong Liang wrote:
> > From: Jinrong Liang <cloudliang@xxxxxxxxxxx>
> > 
> > Add test to check if non-existent counters can be accessed in guest after
> > determining the number of Intel generic performance counters by CPUID.
> > When the num of counters is less than 3, KVM does not emulate #GP if
> > a counter isn't present due to compatibility MSR_P6_PERFCTRx handling.
> > Nor will the KVM emulate more counters than it can support.
> > 
> > Co-developed-by: Like Xu <likexu@xxxxxxxxxxx>
> > Signed-off-by: Like Xu <likexu@xxxxxxxxxxx>
> > Signed-off-by: Jinrong Liang <cloudliang@xxxxxxxxxxx>
> > ---
> >  .../kvm/x86_64/pmu_basic_functionality_test.c | 78 +++++++++++++++++++
> >  1 file changed, 78 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/kvm/x86_64/pmu_basic_functionality_test.c b/tools/testing/selftests/kvm/x86_64/pmu_basic_functionality_test.c
> > index daa45aa285bb..b86033e51d5c 100644
> > --- a/tools/testing/selftests/kvm/x86_64/pmu_basic_functionality_test.c
> > +++ b/tools/testing/selftests/kvm/x86_64/pmu_basic_functionality_test.c
> > @@ -16,6 +16,11 @@
> >  /* Guest payload for any performance counter counting */
> >  #define NUM_BRANCHES			10
> >  
> > +static const uint64_t perf_caps[] = {
> > +	0,
> > +	PMU_CAP_FW_WRITES,
> > +};
> > +
> >  static struct kvm_vm *pmu_vm_create_with_one_vcpu(struct kvm_vcpu **vcpu,
> >  						  void *guest_code)
> >  {
> > @@ -164,6 +169,78 @@ static void intel_test_arch_events(void)
> >  	}
> >  }
> >  
> > +static void guest_wr_and_rd_msrs(uint32_t base, uint8_t begin, uint8_t offset)
> > +{
> > +	uint8_t wr_vector, rd_vector;
> > +	uint64_t msr_val;
> > +	unsigned int i;
> > +
> > +	for (i = begin; i < begin + offset; i++) {
> > +		wr_vector = wrmsr_safe(base + i, 0xffff);
> > +		rd_vector = rdmsr_safe(base + i, &msr_val);

Unless I'm missing something, there is zero reason to pass "base" and "being"
separately, just do the math in the host.  A "base" that isn't actually the base
when viewed without the full context is super confusing.

> > +		if (wr_vector == GP_VECTOR || rd_vector == GP_VECTOR)
> > +			GUEST_SYNC(GP_VECTOR);
> 
> Rather than pass around the "expected" vector, and shuffle #GP vs. the msr_val
> up (which can get false negatives if msr_val == 13), just read
> MSR_IA32_PERF_CAPABILITIES from within the guest and GUEST_ASSERT accordingly.

Ah, you did that so that the fixed counter test can reuse the guest code.  Just
use separate trampolines in the guest, e.g.

static void __guest_wrmsr_rdmsr(uint32_t base, uint8_t nr_msrs, bool expect_gp)
{
	uint64_t msr_val;
	uint8_t vector;
	uint32_t i;

	for (i = base; i < base + nr_msrs; i++) {
		vector = wrmsr_safe(i, 0xffff);
		GUEST_ASSERT(expect_gp ? vector == GP_VECTOR : !vector,
			     "...");

		vector = rdmsr_safe(i, &msr_val);
		GUEST_ASSERT(expect_gp ? vector == GP_VECTOR : !vector,
			     "...");
		if (!expect_gp)
			GUEST_ASSERT_EQ(msr_val, 0);
	}

	GUEST_DONE();
}

static void guest_rd_wr_fixed_counter(uint32_t base, uint8_t nr_msrs)
{
	__guest_wrmsr_rdmsr(base, nr_msrs, true);
}

static void guest_rd_wr_gp_counter(uint32_t base, uint8_t nr_msrs)
{
	uint64_t perf_capabilities = rdmsr();

	__guest_wrmsr_rdmsr(base, nr_msrs, !!perf_capabilities);
}