Re: [git-pull -tip] x86: msr architecture debug code

Ingo Molnar
2009-03-05T14:08:34Z

* Andreas Herrmann <andreas.herrmann3@amd.com> wrote:

> On Mon, Mar 02, 2009 at 09:54:37PM +0100, Ingo Molnar wrote:
> > * Jaswinder Singh Rajput <jaswinder@kernel.org> wrote:
>
> Oops, didn't read this mail till the end.
> Thus I missed this part.
>
> > > +{
> > > + struct cpuinfo_x86 *cpu = &cpu_data(0);
> > > +
> > > + if (!cpu_has(cpu, X86_FEATURE_MSR))
> > > + return -ENODEV;
> > > +
> > > + msr_dir = debugfs_create_dir("msr", arch_debugfs_dir);
> > > +
> > > + msr_file = debugfs_create_file("msr", S_IRUGO, msr_dir,
> > > + NULL, &msr_fops);
> > > + pmc_file = debugfs_create_file("pmc", S_IRUGO, msr_dir,
> > > + NULL, &pmc_fops);
> >
> > I think it would be possible to have a much more intuitive file
> > layout under /debug/x86/msr/ than these two /debug/x86/msr/msr
> > and /debug/x86/msr/pmc files.
> >
> > Firstly, it should move one level deeper, to /debug/x86/cpu/msr/
> > - because the MSR is really a property of the CPU, and there are
> > other properties of the CPU we might want to expose in the
> > future.
> >
> > Secondly, the picking of debugfs (as opposed to sysfs) is a good
> > choice, because we probably want to tweak the layout a number of
> > times and want to keep flexibility, without being limited by the
> > sysfs ABI.
> >
> > So i like the idea - but we really want to do even more and add
> > more structure to this. If we just want dumb msr enumeration we
> > already have /dev/msr.
> >
> > Regarding the msr directory: one good approach would be to have
> > have several "topic" directories under /debug/x86/cpu/msr/.
> >
> > One such topic would be the 'pmu', with a structure like:
> >
> > /debug/x86/cpu/msr/pmu/
> > /debug/x86/cpu/msr/pmu/pmc_0/
> > /debug/x86/cpu/msr/pmu/pmc_0/counter
> > /debug/x86/cpu/msr/pmu/pmc_0/eventsel
> >
> > There would also be a /debug/x86/cpu/msr/raw/ directory with all
> > MSR numbers we know about explicitly, for example:
> >
> > /debug/x86/cpu/msr/raw/0x372/value
> > /debug/x86/cpu/msr/raw/0x372/width
>
> Having this stuff in the kernel unnecessarily bloats up kernel code.

it should be a default-off Kconfig option and it is in debugfs
so there's no real bloat issue here.

> What the kernel needs to provide is a reliable interface to
> access MSRs -- to pass the data to userspace. This interface
> is already there.
>
> IMHO all kind of parsing and grouping of that data belongs in
> user space.
>
> One exception are MSRs that need to be checked early during
> boot (e.g. MTRRs). For debugging purposes you might want to
> dump certain MSRs early. But then you will use printk and not
> debugfs.

Well it's really nice to know the _kernel's_ enumeration of MSRs
and its knowledge about the structure of those MSRs.

Sure, we can and do export the flat MSR space to user-space, but
the kernel also enumerates them internally, in various places.
The debugfs interface shows them in one way - and as such also
acts as a central force to keep these things tidy.

a VFS namespace is also pretty educative. You can see which MSRs
matter to the lapic for example, you can see their symbolic
names, their current state, etc. etc.

> > Maybe a symlink pointing it back to the topic directory
> > would be useful as well. For example:
> >
> > /debug/x86/cpu/msr/raw/0x372/topic_dir -> /debug/x86/cpu/msr/pmu/pmc_0/
> >
> > Other "topic directories" are possible too: a
> > /debug/x86/cpu/msr/apic/ layout would be very useful and
> > informative as well, and so are some of the other MSRs we
> > tweak during bootup.
>
> All nice suggestions but why in-kernel?
>
> Just hack some script to do this. This is much more
> maintainable. You don't need a kernel update to add support
> for new CPUs or to fix bugs in this code itself -- you just
> have to tweak your script.

the kernel tends to know a lot about these MSRs already so we
just provide that information in a more structured form as well.

Such more structured form, beyond the debugging and
education/development advantages, also acts as a counter-force
back to the MSR enumeration code of the kernel and makes them
more structured. It will no doubt also extend the kernel's
knowledge of MSRs - read-only MSRs we dont normally read.

There's also a few other things like the IRR readout in the APIC
code or the perfcounters status dump can also be done cleanly
via /debug/x86/cpu/msr/.

Eventually i'd like /debug/x86/ to become a full CPU state dump:
the kernel pagetable dumping code could go there, we could show
control registers, we could show the GDT and IDT settings and
contents, etc. etc.

Ingo