On Fri, Jan 14, 2022 at 11:15 AM David Dunn <daviddunn@xxxxxxxxxx> wrote:
>
> Jim,
>
> The patch set looks good to me. A couple comments and questions
> related just to this test are inline.
>
> On Thu, Jan 13, 2022 at 5:21 PM Jim Mattson <jmattson@xxxxxxxxxx> wrote:
>
> > + * Determining AMD support for a PMU event requires consulting the AMD
> > + * PPR for the CPU or reference material derived therefrom.
> > + */
> > +static bool vcpu_supports_amd_zen_br_retired(void)
> > +{
> > + struct kvm_cpuid_entry2 *entry;
> > + struct kvm_cpuid2 *cpuid;
> > +
> > + cpuid = kvm_get_supported_cpuid();
> > + entry = kvm_get_supported_cpuid_index(1, 0);
> > + return entry &&
> > + ((x86_family(entry->eax) == 0x17 &&
> > + (x86_model(entry->eax) == 1 ||
> > + x86_model(entry->eax) == 0x31)) ||
> > + (x86_family(entry->eax) == 0x19 &&
> > + x86_model(entry->eax) == 1));
> > +}
>
> The above function does not verify that the AMD host you are running
> on supports PMU. In particular, you might be running the KVM test
> suite within a guest. Is there a way to do that check here without
> direct access to MSRs? If not, maybe we need a KVM capability to
> query this information.
On Intel, if the parent hypervisor has constructed the CPUID:0AH leaf
correctly, the test will skip because it can't find what it wants in
CPUID:0AH, and the CPU family isn't what it is looking for on the AMD
side.
On AMD, the test assumes that the PMU is there (as the specification
requires). For the Zen line, a VM with vPMU enabled should report
CPUID:80000001H:ECX.PerfCtrExtCore[bit 23] set, and a VM with vPMU
disabled should report that bit clear.
In v3, I'm adding a PMU sanity check based on Linux's check_hw_exists().
> > + r = kvm_check_cap(KVM_CAP_PMU_EVENT_FILTER);
> > + if (!r) {
> > + print_skip("KVM_CAP_PMU_EVENT_FILTER not supported");
> > + exit(KSFT_SKIP);
> > + }
>
> This capability is still supported even when PMU has been disabled by
> Like Xu's new module parameter. Should all the PMU related
> capabilities be gated behind that module parameter?
Perhaps. That's something for Like Xu to consider with the current
rewrite of the module parameter.
> Dave Dunn
