Re: [PATCH v1 00/26] x86: Introduce centralized CPUID model
"Ahmed S. Darwish" <[email protected]>
| Newsgroups | dev.linux.lists.x86-cpuid,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <aBsor3iOb0SJsLSQ@lx-t490> |
On Tue, 06 May, Ingo Molnar wrote: > > Wrt. <asm/cpuid/api.h>, we'll need a few followup cleanups there too > I think, such as migrating to the cpuid_*() namespace: > Perfect, then I'll move ahead and do a "CPUID headers cleanup" patch queue /before/ moving into a v2 of this series. Any other wishes in that domain? > Rename have_cpuid_p() to cpuid_feature() or so. ACK. > I find the cpudata_cpuid_ namespace a bit confusing: > > __cpudata_cpuid_subleaf_idx(__table, __leaf, __subleaf, __idx) > __cpudata_cpuid_subleaf(__table, __leaf, __subleaf) > cpudata_cpuid_subleaf(_cpuinfo, _leaf, _subleaf) > cpudata_cpuid(_cpuinfo, _leaf) > cpudata_cpuid_nr_entries(_cpuinfo, _leaf) > cpudata_cpuid_index(_cpuinfo, _leaf, _idx) > cpudata_cpuid_regs(_cpuinfo, _leaf) > cpudata_cpuid_index_regs(_cpuinfo, _leaf, _idx) > > All of CPUID processing is related to 'data', and we don't > really have any 'cpudata' primitives, so the cpudata_ prefix is > confusing to me. > > It's particularly confusing for methods like cpudata_cpuid(), > which sounds like a generic method, while in reality it accesses > subleaf 0, right? Why not name it cpuid_subleaf_0() or so? > > My suggestion would be to use a structure like this: > > __cpuid_subleaf_idx(__table, __leaf, __subleaf, __idx) > __cpuid_subleaf(__table, __leaf, __subleaf) > cpuid_subleaf(_cpuinfo, _leaf, _subleaf) > cpuid_subleaf_0(_cpuinfo, _leaf) > cpuid_leaf_nr_entries(_cpuinfo, _leaf) > cpuid_leaf_index(_cpuinfo, _leaf, _idx) > cpuid_leaf_regs(_cpuinfo, _leaf) > cpuid_leaf_index_regs(_cpuinfo, _leaf, _idx) > > Or so? Yeah, that's honestly much much better. (cpuid_subleaf() is already a raw CPUID OP at <asm/cpuid/api.h> now, but luckily it has one external call site, so it'll be easy to rename.) > In my book it's a nice bonus that they thus become part of the overall > cpuid_*() API family. Note how these accessors still are all still > either cpuid_leaf_ or cpuid_subleaf_ prefixed. Yeah, now a single <asm/cpuid/api.h> makes full sense as well :) Thanks! Ahmed