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

[Andreas Herrmann <andreas.herrmann3@amd.com>]

On Mon, Mar 09, 2009 at 12:42:49PM +0530, Jaswinder Singh Rajput wrote:
> On Sun, 2009-03-08 at 12:19 +0530, Jaswinder Singh Rajput wrote:
> > The following changes since commit cb553adb4e83bc5314b5a0c397e546a215b19307:
> >   Ingo Molnar (1):
> >         Merge branch 'tracing/printk'
> > 
> > are available in the git repository at:
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/jaswinder/linux-2.6-tip-cpu.git master
> > 
> > Jaswinder Singh Rajput (1):
> >       x86: cpu architecture debug code
> > 

I still doubt that this is going to be useful (I don't see use cases for it).
Nevertheless I have some comments.

> diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
> index c381330..6427a84 100644
> --- a/arch/x86/kernel/cpu/Makefile
> +++ b/arch/x86/kernel/cpu/Makefile
> @@ -9,7 +9,7 @@ endif
>  
>  obj-y			:= intel_cacheinfo.o addon_cpuid_features.o
>  obj-y			+= proc.o capflags.o powerflags.o common.o
> -obj-y			+= vmware.o hypervisor.o
> +obj-y			+= vmware.o hypervisor.o cpu_debug.o
>  
>  obj-$(CONFIG_X86_32)	+= bugs.o cmpxchg.o
>  obj-$(CONFIG_X86_64)	+= bugs_64.o

There is no kernel config option and this code will always be
compiled in. Why?

> diff --git a/arch/x86/kernel/cpu/cpu_debug.c b/arch/x86/kernel/cpu/cpu_debug.c
> new file mode 100755
> index 0000000..e50d73b
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/cpu_debug.c
> @@ -0,0 +1,692 @@

   ...

> +static int cpu_seq_show(struct seq_file *seq, void *v)
> +{
> +	struct cpu_private *priv = seq->private;
> +
> +	if (priv == NULL)
> +		return -EINVAL;
> +
> +	switch (cpu_base[priv->type].flag) {
> +	case CPU_TSS:
> +		smp_call_function_single(priv->cpu, print_stdreg, seq, 1);
> +		break;
> +	case CPU_CR:
> +		smp_call_function_single(priv->cpu, print_cr, seq, 1);
> +		break;
> +	case CPU_DT:
> +		smp_call_function_single(priv->cpu, print_dt, seq, 1);
> +		break;
> +	default:
> +		print_msr(seq, priv->cpu, cpu_base[priv->type].flag);
> +		break;
> +	}
> +	seq_printf(seq, "\n");

(As one example.) Please don't print empty lines.

General thoughts/questions:

Your code is not cpu-hotpluggable. The complete debugfs subtree shows up
for all CPUs - including CPUs that are offline. Was this your intention?

How does this (especially dumping of register state) fit into other
kernel debugging facilities, ie.
 - kgdb
 - sysrq


Regards,
Andreas

-- 
Operating | Advanced Micro Devices GmbH
  System  | Karl-Hammerschmidt-Str. 34, 85609 Dornach b. München, Germany
 Research | Geschäftsführer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
  Center  | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
  (OSRC)  | Registergericht München, HRB Nr. 43632