Re: [PATCH 3/6] KVM: x86: selftests: Set up AMD VM in pmu_counters_test

[Colton Lewis]

Sean Christopherson <seanjc@xxxxxxxxxx> writes:

> On Wed, Aug 28, 2024, Mingwei Zhang wrote:
> > > >> +static void test_core_counters(void)
> > > >> +{
> > > >> +    uint8_t nr_counters = nr_core_counters();
> > > >> +    bool core_ext = kvm_cpu_has(X86_FEATURE_PERF_CTR_EXT_CORE);
> > > >> +    bool perf_mon_v2 = kvm_cpu_has(X86_FEATURE_PERF_MON_V2);
> > > >> +    struct kvm_vcpu *vcpu;
> > > >> +    struct kvm_vm *vm;
> > >
> > > >> -    kvm_pmu_version = kvm_cpu_property(X86_PROPERTY_PMU_VERSION);
> > > >> -    kvm_has_perf_caps = kvm_cpu_has(X86_FEATURE_PDCM);
> > > >> +    vm = vm_create_with_one_vcpu(&vcpu, guest_test_core_counters);
> > >
> > > >> -    test_intel_counters();
> > > >> +    /* This property may not be there in older underlying CPUs,
> > > >> +     * but it simplifies the test code for it to be set
> > > >> +     * unconditionally.

> But then the test isn't verifying that KVM is honoring the architecture.   
> I.e.
> backdooring information to the guest risks getting false passes because  
> KVM
> incorrectly peeks at the same information, which shouldn't exist.

> > > >> +     */

> 	/*
> 	 * Multi-line function comments should start on the line after the
> 	 * opening slash-asterisk, like so.
> 	 */

> > > >> +    vcpu_set_cpuid_property(vcpu, X86_PROPERTY_NUM_PERF_CTR_CORE,
> > > >> nr_counters);
> > > >> +    if (core_ext)
> > > >> +            vcpu_set_cpuid_feature(vcpu,  
> > X86_FEATURE_PERF_CTR_EXT_CORE);
> > > >> +    if (perf_mon_v2)
> > > >> +            vcpu_set_cpuid_feature(vcpu, X86_FEATURE_PERF_MON_V2);
> > >
> > > > hmm, I think this might not be enough. So, when the baremetal machine
> > > > supports Perfmon v2, this code is just testing v2. But we should be  
> > able
> > > > to test anything below v2, ie., v1, v1 without core_ext. So, three
> > > > cases need to be tested here: v1 with 4 counters; v1 with core_ext (6
> > > > counters); v2.
> > >
> > > > If, the machine running this selftest does not support v2 but it does
> > > > support core extension, then we fall back to test v1 with 4 counters  
> > and
> > > > v1 with 6 counters.
> > >
> > > This should cover all cases the way I wrote it. I detect the number of
> > > counters in nr_core_counters(). That tells me if I am dealing with 4 or
> > > 6 and then I set the cpuid property based on that so I can read that
> > > number in later code instead of duplicating the logic.

> > right. in the current code, you set up the counters properly according
> > to the hw capability. But the test can do more on a hw with perfmon
> > v2, right? Because it can test multiple combinations of setup for a
> > VM: say v1 + 4 counters and v1 + 6 counters etc. I am just following
> > the style of this selftest on Intel side, in which they do a similar
> > kind of enumeration of PMU version + PDCM capabilities. In each
> > configuration, it will invoke a VM and do the test.

> Ya.  This is similar my comments on setting NUM_PER_CTR_CORE when the  
> field
> shouldn't exist.  One of the main goals of this test is to verify the KVM  
> honors
> the architecture based on userspace's defined virtual CPU model, i.e.  
> guest
> CPUID.  That means testing all (or at least, within reason) possible  
> combinations
> that can feasibly be supported by KVM given the underlying hardware.

> As written, this essentially just tests the maximal configuration that  
> can be
> exposed to a guest, which isn't _that_ interesting because KVM tends to  
> get plenty
> of coverage for such setups, e.g. by running "real" VMs.

Ok. I get what you and Mingwei are saying. I can test those combinations.