re-enable AMD Topology extension on certain models if disabled by BIOS

[avg]

Summary

Some BIOSes disable AMD Topology extension on AMD Family 15h notebook
processors. We re-enable the extension, so that we can properly discover
core and cache topology. Linux seems to do the same.

[avg]

Is identify_cpu() the best place to patch up the CPU configuration?

[kib]

I am not sure that this is the best place, but it is early enough so that the things actually work.

OTOH, I strongly recommend to re-enable the extension not only on BSP, but also on APs. If any user-mode application reads the data for any reason, it would be highly confused by inconsistent reporting, depending on the CPU the thread happens to execute.

Look at the intel_fix_cpuid() for very similar example, r286228.

[avg]

Thank you. I'll take a look.

[jhb]

It might be simplest to just rename intel_fix_cpuid() to 'fix_cpuid()' and include the AMD logic in there. You would probably want to only re-fetch amd_feature2 in the BSP in identifycpu() though.

Note that identifycpu() doesn't generally print things. We print CPU info slightly later in printcpuinfo().

[jhb]

In D5883#125647, @jhb wrote:

It might be simplest to just rename intel_fix_cpuid() to 'fix_cpuid()' and include the AMD logic in there. You would probably want to only re-fetch amd_feature2 in the BSP in identifycpu() though.

Note that identifycpu() doesn't generally print things. We print CPU info slightly later in printcpuinfo().

Actually, amd_feature2 is fetched after intel_fix_cpuid is called anyway. You could choose to follow the logic intel_fix_cpuid() follows where you examine the 0xc0011005 MSR directly and set bit 54 if it isn't already set.

[avg]

Re-enable AMD Topology Extension on APs as well

intel_fix_cpuid() is re-used for this task and, thus, it is renamed to
fix_cpuid(). One quirk is its return value, which is still specific
to the original Intel fix.

[avg]

fix previous update

[avg]

sigh...

[kib]

Looks fine. What about using symbolic designations for MSR and manipulated bit ?

[jhb]

Constants for the MSR values would also be nice.

sys/x86/x86/identcpu.c

1386 ↗

(On Diff #15072)

I would be tempted to let this return true. It doesn't do any harm to refetch. We might have more MSR changes like this in the future, at which point it might make sense to just have identcpu.c refetch all the cpuid info fetched so far if this returns true.

It might be even simpler if we could move the cpuid bits for i386 out of assembly and into identcpu.c as well to reduce the diff with amd64. We could then possibly invoke this sooner when there would be less data to refetch.

[jhb]

sys/x86/x86/identcpu.c

1386 ↗

(On Diff #15072)

To be clear, I don't want you to fix any of the refetch, etc. I would just make this return true so that the return value means "we changed something".

[avg]

• return 'true' to signify that a change has been made
• define a constant for the MSR

[kib]

Bit 54 also could get a name in specialreg.h.

After all the restructuring, I would prefer that fix_cpuid() only has single return (ret); at the very end of the funciton. I mean, it would be useful for future additions to not return early, but allow all condition clauses to evaluate.

[avg]

Can we stop making further improvements to this rather small change now? :-)

• it seems that not very many MSR bits have names; I am not sure that this MSR is going to be referenced again, let alone this particular bit
• let's make a change to have a single return statement when we actually need it?

In other words, I do not object to the requested changes, but are they really worth making?

[kib]

In D5883#126180, @avg wrote:

Can we stop making further improvements to this rather small change now? :-)

• it seems that not very many MSR bits have names; I am not sure that this MSR is going to be referenced again, let alone this particular bit
• let's make a change to have a single return statement when we actually need it?

In other words, I do not object to the requested changes, but are they really worth making?

Commit what you have.

I do think that further restructuring is useful, will do it myself.