linux-kernel - Re: [PATCH 3/6] KVM: x86: selftests: Set up AMD VM in pmu_counters

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <gsntcyllaolq.fsf@coltonlewis-kvm.c.googlers.com>
Date: Mon, 02 Sep 2024 18:37:37 +0000
From: Colton Lewis <coltonlewis@...gle.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: mizhang@...gle.com, kvm@...r.kernel.org, ljr.kernel@...il.com, 
	jmattson@...gle.com, aaronlewis@...gle.com, pbonzini@...hat.com, 
	shuah@...nel.org, linux-kselftest@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/6] KVM: x86: selftests: Set up AMD VM in pmu_counters_test

Sean Christopherson <seanjc@...gle.com> 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.