Re: [Xen-devel] [PATCH][3 of 3] GDB serial port debugging: Changes to add SMP pausing, x86_64 register mappings for serial port GDB, and others.

[Isaku Yamahata <yamahata@xxxxxxxxxxxxx>]

On Wed, Dec 19, 2007 at 02:50:45PM -0800, Dan Doucette wrote:
> Hello,

Hi.

> diff -Nur --exclude=setup.c --exclude=nmi.c --exclude='*.o' --exclude='*.s' 
> --exclude='*.swp' --exclude=compile.h --exclude=serial.c 
> xen-unstable.hg/xen/arch/x86/gdbstub.c 
> xen-unstable.hg.new/xen/arch/x86/gdbstub.c
> --- xen-unstable.hg/xen/arch/x86/gdbstub.c    2007-12-19 14:18:45.000000000 
> -0800
> +++ xen-unstable.hg.new/xen/arch/x86/gdbstub.c        2007-12-19 
> 09:21:29.000000000 -0800
...
> @@ -107,13 +67,107 @@
>  void
>  gdb_arch_enter(struct cpu_user_regs *regs)
>  {
> -    /* nothing */
> +    /*
> +     * We must pause all other CPUs.
> +     */
> +    gdb_smp_pause();
>  }
>  
>  void
>  gdb_arch_exit(struct cpu_user_regs *regs)
>  {
> -    /* nothing */
> +    /*
> +     * Resume all CPUs.
> +     */
> +    gdb_smp_resume();
> +}
> +
> +static void gdb_pause_this_cpu(void *unused)
> +{
> +    unsigned long flags;
> +
> +    local_irq_save(flags);
> +
> +    atomic_set(&gdb_cpu[smp_processor_id()].ack, 1);
> +    atomic_inc(&gdb_smp_paused_count);
> +
> +    while (atomic_read(&gdb_cpu[smp_processor_id()].paused)) 
> +        mdelay(1);
> +
> +    atomic_dec(&gdb_smp_paused_count);
> +    atomic_set(&gdb_cpu[smp_processor_id()].ack, 0);
> +
> +    /* Restore interrupts */
> +    local_irq_restore(flags);
> +}
> +
> +static void gdb_smp_pause(void)
> +{
> +    int timeout = 100;
> +    int cpu;
> +
> +    for_each_online_cpu(cpu)
> +    {
> +        atomic_set(&gdb_cpu[cpu].ack, 0);
> +        atomic_set(&gdb_cpu[cpu].paused, 1);
> +    }
> +
> +    atomic_set(&gdb_smp_paused_count, 0);
> +
> +    smp_call_function(gdb_pause_this_cpu, NULL, /* dont wait! */0, 0);
> +
> +    /* Wait 100ms for all other CPUs to enter pause loop */
> +    while ( (atomic_read(&gdb_smp_paused_count) < (num_online_cpus() - 1)) 
> +            && (timeout-- > 0) )
> +        mdelay(1);
> +
> +    if ( atomic_read(&gdb_smp_paused_count) < (num_online_cpus() - 1) )
> +    {
> +        printk("GDB: Not all CPUs have paused, missing CPUs ");
> +        for_each_online_cpu(cpu)
> +        {
> +            if ( cpu == smp_processor_id() )
> +                continue;
> +
> +            if ( !atomic_read(&gdb_cpu[cpu].ack) )
> +            {
> +                printk("%d ", cpu);
> +            }
> +        }
> +        printk("\n");
> +    }
> +}
> +
> +static void gdb_smp_resume(void)
> +{
> +    int cpu;
> +    int timeout = 100;
> +
> +    for_each_online_cpu(cpu)
> +    {
> +        atomic_set(&gdb_cpu[cpu].paused, 0);
> +    }
> +
> +    /* Make sure all CPUs resume */
> +    while ( (atomic_read(&gdb_smp_paused_count) > 0)
> +            && (timeout-- > 0) )
> +        mdelay(1);
> +
> +    if ( atomic_read(&gdb_smp_paused_count) > 0 )
> +    {
> +        printk("GDB: Not all CPUs have resumed execution, missing CPUs ");
> +        for_each_online_cpu(cpu)
> +        {
> +            if ( cpu == smp_processor_id() )
> +                continue;
> +
> +            if ( !atomic_read(&gdb_cpu[cpu].ack) )
> +            {
> +                printk("%d ", cpu);
> +            }
> +        }
> +        printk("\n");
> +    }
>  }

Pausing/resuming cpus looks like arch generic.
Could you move them to the common code?

> diff -Nur --exclude=setup.c --exclude=nmi.c --exclude='*.o' --exclude='*.s' 
> --exclude='*.swp' --exclude=compile.h --exclude=serial.c 
> xen-unstable.hg/xen/common/keyhandler.c 
> xen-unstable.hg.new/xen/common/keyhandler.c
> --- xen-unstable.hg/xen/common/keyhandler.c   2007-12-19 14:18:45.000000000 
> -0800
> +++ xen-unstable.hg.new/xen/common/keyhandler.c       2007-12-19 
> 09:21:29.000000000 -0800
> @@ -276,7 +276,7 @@
>  static void do_debug_key(unsigned char key, struct cpu_user_regs *regs)
>  {
>      printk("'%c' pressed -> trapping into debugger\n", key);
> -    (void)debugger_trap_fatal(0xf001, regs);
> +    (void)debugger_trap_fatal(TRAP_int3, regs);
>      nop(); /* Prevent the compiler doing tail call
>                               optimisation, as that confuses xendbg a
>                               bit. */

TRAP_int3 is x86 specific.
Please define a constant like TRAP_gdbstab in the arch header and use it.


> diff -Nur --exclude=setup.c --exclude=nmi.c --exclude='*.o' --exclude='*.s' 
> --exclude='*.swp' --exclude=compile.h --exclude=serial.c 
> xen-unstable.hg/xen/include/xen/gdbstub.h 
> xen-unstable.hg.new/xen/include/xen/gdbstub.h
> --- xen-unstable.hg/xen/include/xen/gdbstub.h 2007-12-19 14:18:46.000000000 
> -0800
> +++ xen-unstable.hg.new/xen/include/xen/gdbstub.h     2007-12-19 
> 09:21:29.000000000 -0800
> @@ -69,6 +69,9 @@
>      struct cpu_user_regs *regs, const char* buf, struct gdb_context *ctx);
>  void gdb_arch_read_reg(
>      unsigned long regnum, struct cpu_user_regs *regs, struct gdb_context 
> *ctx);
> +void gdb_arch_write_reg(
> +    unsigned long regnum, unsigned long val, struct cpu_user_regs *regs, 
> +    struct gdb_context *ctx);
>  unsigned int gdb_arch_copy_from_user(
>      void *dest, const void *src, unsigned len);
>  unsigned int gdb_arch_copy_to_user(

Adding gdb_arch_write_reg() breaks other arch (IA64 and power).
How about adding the default definition which just returns
as a weak symbol?

-- 
yamahata

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel