On Tue, Feb 12, 2008 at 12:03:35AM +0100, Ingo Molnar wrote:
quoted text
> > The synchronization code looks as bad as it was before.

First nobody answered the "kgdb clean enough for a module"
high level question yet. Is it good enough for that?

quoted text
> 
> i reworked and cleaned up all the kgdb locking code completely. No more 
> NR_CPUS spinlocks, there's now proper use of barriers, etc. No more 
> silly "timeouts" for locking either ... There's now a very well-defined 

A timeout for waiting for other CPUs is actually not a bad idea for a 
debugger. After all you still want to debug even if some other CPUs
are dead.

quoted text
> and clean locking state-machine for the primary CPU and the secondary 
> CPUs. I re-tested it all on SMP hardware as well.

If it's very clean you should consider sharing it with kdump.

quoted text
> previously Mark indicated some sort of sprintf return value breakage he 
> observed, and kgdb would rely on sprintf return values so i'm inclined 
> to leave it as-is. We can fix that later, it's not critical.

Pretty much all the proc output and sysfs show functions rely on these return 
values, so if there is a problem it is likely very obscure.

quoted text
> +	gdb_regs[GDB_BP]	= *(unsigned long *)p->thread.sp;
> +#ifdef CONFIG_X86_32
> +	gdb_regs[GDB_DS]	= __KERNEL_DS;
> +	gdb_regs[GDB_ES]	= __KERNEL_DS;
> +	gdb_regs[GDB_PS]	= 0;
> +	gdb_regs[GDB_CS]	= __KERNEL_CS;
> +	gdb_regs[GDB_PC]	= p->thread.ip;
> +	gdb_regs[GDB_SS]	= __KERNEL_DS;
> +	gdb_regs[GDB_FS]	= 0xFFFF;
> +	gdb_regs[GDB_GS]	= 0xFFFF;
> +#else
> +	gdb_regs[GDB_PS]	= *(unsigned long *)(p->thread.sp + 8);
> +	gdb_regs[GDB_PC]	= 0;
> +	gdb_regs[GDB_R8]	= 0;
> +	gdb_regs[GDB_R9]	= 0;
> +	gdb_regs[GDB_R10]	= 0;
> +	gdb_regs[GDB_R11]	= 0;
> +	gdb_regs[GDB_R12]	= 0;
> +	gdb_regs[GDB_R13]	= 0;
> +	gdb_regs[GDB_R14]	= 0;
> +	gdb_regs[GDB_R15]	= 0;

The 64bit gdb actually supports segment registers:

static int amd64_linux_gregset64_reg_offset[] =
{
  RAX * 8, RBX * 8,             /* %rax, %rbx */
  RCX * 8, RDX * 8,             /* %rcx, %rdx */
  RSI * 8, RDI * 8,             /* %rsi, %rdi */
  RBP * 8, RSP * 8,             /* %rbp, %rsp */
  R8 * 8, R9 * 8,               /* %r8 ... */
  R10 * 8, R11 * 8,
  R12 * 8, R13 * 8,
  R14 * 8, R15 * 8,             /* ... %r15 */
  RIP * 8, EFLAGS * 8,          /* %rip, %eflags */
  CS * 8, SS * 8,               /* %cs, %ss */
  DS * 8, ES * 8,               /* %ds, %es */
  FS * 8, GS * 8,               /* %fs, %gs */
  -1, -1, -1, -1, -1, -1, -1, -1,
  -1, -1, -1, -1, -1, -1, -1, -1,
  -1, -1, -1, -1, -1, -1, -1, -1,
  -1, -1, -1, -1, -1, -1, -1, -1, -1,
  ORIG_RAX * 8
};

quoted text
> + *
> + *	This function will be called if the particular architecture must
> + *	disable hardware debugging while it is processing gdb packets or
> + *	handling exception.
> + */
> +void kgdb_disable_hw_debug(struct pt_regs *regs)
> +{
> +	/* Disable hardware debugging while we are in kgdb: */
> +	set_debugreg(0UL, 7);

You silently overwrite any user ptrace hw breakpoints right? To do it cleanly 
would still require a reservation frame work.

quoted text
> +/**
> + *	kgdb_arch_handle_exception - Handle architecture specific GDB packets.

All the kerneldoc comments are useless if you don't add the file
to Documentation/DocBook/*.tmpl

quoted text
> +static int __kgdb_notify(struct die_args *args, unsigned long cmd)
> +{
> +	struct pt_regs *regs = args->regs;
> +
> +	switch (cmd) {
> +	case DIE_NMI:
> +		if (atomic_read(&kgdb_active) != -1) {
> +			/* KGDB CPU roundup */
> +			kgdb_nmicallback(raw_smp_processor_id(), regs);
> +			return NOTIFY_STOP;
> +		}
> +		return NOTIFY_DONE;
> +
> +	case DIE_NMI_IPI:
> +		if (atomic_read(&kgdb_active) != -1) {
> +			/* KGDB CPU roundup: */
> +			if (kgdb_nmicallback(raw_smp_processor_id(), regs))
> +				return NOTIFY_DONE;
> +			return NOTIFY_STOP;
> +		}
> +		return NOTIFY_DONE;
> +
> +	case DIE_NMIWATCHDOG:
> +		if (atomic_read(&kgdb_active) != -1) {

I don't think that case should happen during roundup for once. If it happens 
something is wrong.

In fact both handling DIE_NMI and DIE_NMI_IPI is fishy too.

quoted text
> +			/* KGDB CPU roundup: */
> +			kgdb_nmicallback(raw_smp_processor_id(), regs);
> +			return NOTIFY_STOP;
> +		}
> +		/* Enter debugger: */
> +		break;
> +
> +	case DIE_DEBUG:
> +		if (atomic_read(&kgdb_cpu_doing_single_step) ==
> +			raw_smp_processor_id() &&
> +			user_mode(regs))
> +			return single_step_cont(regs, args);
> +		/* fall through */
> +	default:
> +		if (user_mode(regs))
> +			return NOTIFY_DONE;

That seems weird. I think other parts try to support user mode debugging
too. In theory there is no reason it shouldn't be able to do this
(except that you have to make sure to not break regular gdb of course) 

quoted text
> +
> +static struct notifier_block kgdb_notifier = {
> +	.notifier_call	= kgdb_notify,
> +
> +	/*
> +	 * Lowest-prio notifier priority, we want to be notified last:
> +	 */
> +	.priority	= -INT_MAX,

This means kcrash will have priority won't it? Doesn't seem correct.

quoted text
> +	int			pass_exception;
> +	long			threadid;
> +	long			kgdb_usethreadid;
> +	struct pt_regs		*linux_regs;
> +};
> +
> +static struct debuggerinfo_struct {
> +	void			*debuggerinfo;
> +	struct task_struct	*task;
> +} kgdb_info[NR_CPUS];

quoted text
> +int __weak kgdb_validate_break_address(unsigned long addr)
> +{
> +	char tmp_variable[BREAK_INSTR_SIZE];
> +
> +	return probe_kernel_read(tmp_variable, (char *)addr, BREAK_INSTR_SIZE);

Ok I have not checked, but I hope you have strong protections against
reentry of the debugger just in case an exception here falls down to
the die notifiers

quoted text
> +{
> +	return probe_kernel_write((char *)addr,
> +				  (char *)bundle, BREAK_INSTR_SIZE);
> +}
> +
> +unsigned long __weak kgdb_arch_pc(int exception, struct pt_regs *regs)
> +{
> +	return instruction_pointer(regs);

That wrapper should not be needed; everybody can use instruction_pointer()
directly, no?


quoted text
> +static const char	hexchars[] = "0123456789abcdef";
> +
> +static int hex(char ch)

... Ok i already commented on the hex mess.


quoted text
> +{
> +	error = -error;
> +	pkt[0] = 'E';
> +	pkt[1] = hexchars[(error / 10)];
> +	pkt[2] = hexchars[(error % 10)];

e.g. you surely don't need sprintf return values for that one.

quoted text
> +static struct task_struct *getthread(struct pt_regs *regs, int tid)
> +{
> +	if (num_online_cpus() && (tid >= pid_max + num_online_cpus()))
> +		return NULL;

I still don't think this check makes sense.

quoted text
> +
> +	if (tid >= pid_max)
> +		return idle_task(tid - pid_max);

Hmm, so idle thread numbers depend on a sysctl? That seems weird.
Would be probably better to just give them negative numbers.



quoted text
> +
> +	if (!tid)
> +		return NULL;
> +
> +	/*
> +	 * find_task_by_pid() does not take the tasklist lock anymore
> +	 * but is nicely RCU locked - hence is a pretty resilient
> +	 * thing to use:
> +	 */
> +	return find_task_by_pid(tid);

You don't think find_task_by_pid() will handle 0 tid?


quoted text
> +
> +static inline int shadow_pid(int realpid)
> +{
> +	if (realpid)
> +		return realpid;
> +
> +	return pid_max + raw_smp_processor_id();

Whatever that shadowpid is. Seems like a weird concept.

quoted text
> +	if (strcmp(remcom_in_buffer, "R0") == 0) {
> +		printk(KERN_CRIT "Executing reboot\n");
> +		strcpy(remcom_out_buffer, "OK");
> +		put_packet(remcom_out_buffer);
> +
> +		/*
> +		 * Execution should not return from
> +		 * machine_restart()
> +		 */
> +		machine_restart(NULL);

It's still likely to deadlock on MP I think

[...] Haven't read further.

-Andi

--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/