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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 3208 Build 3241: arc lint + arc unit
Event Timeline
Comment Actions
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.
Comment Actions
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().
Comment Actions
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.
Comment Actions
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.
Comment Actions
Constants for the MSR values would also be nice.
| sys/x86/x86/identcpu.c | ||
|---|---|---|
| 1386 | 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. | |
| sys/x86/x86/identcpu.c | ||
|---|---|---|
| 1386 | 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". | |
Comment Actions
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.
Comment Actions
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?
Comment Actions
Commit what you have.
I do think that further restructuring is useful, will do it myself.