libpmc: Strip leading zeros from the cpuid model

[ojanerif_amd.com]

Summary

mapfile.csv lists models without leading zeros, so entries for models
below 10h had to be fixed up by hand on every import. Strip them before
matching and use the entries as imported.

Sponsored by: Advanced Micro Devices, Inc.
Signed-off-by: Osvaldo Janeri Filho <[e-mail]>

[ali_mashtizadeh.com]

Looks good to me is that correct for Hygon? I saw upstream it seems there’s some issues and we may need to remove it until someone fixes the differences in DF programming.

[ojanerif_amd.com]

Hygon matches as before: HygonGenuine-24-00 becomes 24-0 on both sides, and low_models checks it. The DF/L3 differences are in hwpmc/libpmc, not the mapfile. Which issue do you mean?

[mhorne]

I don't trust this fully.

You are trimming the result of kern.hwpmc.cpuid, fine. But look! It is hwpmc that specifies the exact format of this string:

	if (family == 0x18)
		snprintf(pmc_cpuid, sizeof(pmc_cpuid), "HygonGenuine-%d-%02X-%X",
		    family, model, stepping);
	else
		snprintf(pmc_cpuid, sizeof(pmc_cpuid), "AuthenticAMD-%d-%02X-%X",
		    family, model, stepping);

The format string specifies that the model field (%02X) will be zero-padded, and at least two-digits wide. You are only trying to undo this later in userspace.

Additionally, you must look at the mapfile.csv for other architectures. Here is an excerpt from pmu-events/arch/arm64/mapfile.csv:

#Family-model,Version,Filename,EventType
0x00000000410fd020,v1,arm/cortex-a34,core
0x00000000410fd030,v1,arm/cortex-a53,core
0x00000000420f1000,v1,arm/cortex-a53,core
0x00000000410fd040,v1,arm/cortex-a35,core

What will happen if we strip leading zeros in this case?

I have not thought through all the different scenarios, but we need to be careful about compatibility as we proceed here.

[ojanerif_amd.com]

Thanks, fair points. Some context before I change anything:

The strip only runs when the id has a vendor-family-model layout (two -). The arm64 (0x%016lx) and powerpc (%08x) ids have no -, so they come through unchanged, e.g. 0x00000000410fd030 stays as is. On x86, no Intel entry in the mapfile has a model below 10h, so Intel matching does not change either. Only the AMD/Hygon low models are affected.

I agree that this works by accident on the other architectures rather than by design. I see three ways forward:

1. Keep the normalization in libpmc, but limit it explicitly to the x86 vendor prefixes (AuthenticAMD-, HygonGenuine-, GenuineIntel-). Add a comment saying the kernel pads the model with %02X and the mapfile does not, plus a test that a padded Intel id still resolves to the same table.
2. Change hwpmc to print the model with %X. This removes the need for any userspace fixup, but it changes kern.hwpmc.cpuid and breaks older userland on a newer kernel, since the current mapfile expects the padded form.
3. Drop this change and keep hand-editing the AMD lines on import, relying on the mapfile test from D60625 to catch it.

I would go with 1, since it keeps the sysctl as is. Would that work for you, or do you prefer another option?

[mhorne]

In D60790#1405769, @ojanerif_amd.com wrote:

Thanks, fair points. Some context before I change anything:

The strip only runs when the id has a vendor-family-model layout (two -). The arm64 (0x%016lx) and powerpc (%08x) ids have no -, so they come through unchanged, e.g. 0x00000000410fd030 stays as is. On x86, no Intel entry in the mapfile has a model below 10h, so Intel matching does not change either. Only the AMD/Hygon low models are affected.

Okay, great clarification!

I agree that this works by accident on the other architectures rather than by design. I see three ways forward:

1. Keep the normalization in libpmc, but limit it explicitly to the x86 vendor prefixes (AuthenticAMD-, HygonGenuine-, GenuineIntel-). Add a comment saying the kernel pads the model with %02X and the mapfile does not, plus a test that a padded Intel id still resolves to the same table.
2. Change hwpmc to print the model with %X. This removes the need for any userspace fixup, but it changes kern.hwpmc.cpuid and breaks older userland on a newer kernel, since the current mapfile expects the padded form.

Right. Older userland on a newer kernel is the compatibility we care about, so it seems we are stuck with the current format at least until we are willing to bump the PMC interface major version.

3. Drop this change and keep hand-editing the AMD lines on import, relying on the mapfile test from D60625 to catch it.

I would go with 1, since it keeps the sysctl as is. Would that work for you, or do you prefer another option?

Thanks for clarifying the different options. I agree with scenario 1 as the best option here.

Restoring parity with Linux's mapfile.csv is a worthwhile target. If we must carry some diffs there that is acceptable, but if it is not required, even better.

[ojanerif_amd.com]

Under this version the strip can't reach your arm64 case. _pmc_cpuid_strip_model_zero() checks for an AuthenticAMD-/HygonGenuine- prefix first and returns on anything else, before any dash-counting. arm64's 0x00000000410fd030 has no vendor prefix, so it comes back untouched; same for powerpc's PVR.

Added non_amd_cpuid_unchanged in pmc_mapfile_test.c, covering your exact example, a powerpc PVR, and an adversarial NotAVendor-1-00-1 string to prove the vendor check, not dash-counting, is what gates this. Confirmed it fails without the gate and passes with it.

[mhorne]

LGTM, I will try to merge today.

The comment verbosity is a little high; I may trim them before merging.