Message-ID: <20090530183709.GA8739@nowhere>
Date:	Sat, 30 May 2009 20:37:10 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	"K.Prasad" <prasad@...ux.vnet.ibm.com>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Ingo Molnar <mingo@...e.hu>,
	Alan Stern <stern@...land.harvard.edu>
Subject: Re: [Patch 05/12] Use wrapper routines around debug registers in
	processor related functions

On Sat, May 30, 2009 at 04:21:29PM +0530, K.Prasad wrote:
> This patch enables the use of wrapper routines to access the debug/breakpoint
> registers.
> 
> Original-patch-by: Alan Stern <stern@...land.harvard.edu>
> Signed-off-by: K.Prasad <prasad@...ux.vnet.ibm.com>
> Reviewed-by: Alan Stern <stern@...land.harvard.edu>
> ---
>  arch/x86/kernel/smpboot.c |    3 +++
>  arch/x86/power/cpu_32.c   |   16 +++-------------
>  arch/x86/power/cpu_64.c   |   15 +++------------
>  3 files changed, 9 insertions(+), 25 deletions(-)
> 
> Index: linux-2.6-tip.hbkpt/arch/x86/kernel/smpboot.c
> ===================================================================
> --- linux-2.6-tip.hbkpt.orig/arch/x86/kernel/smpboot.c
> +++ linux-2.6-tip.hbkpt/arch/x86/kernel/smpboot.c
> @@ -63,6 +63,7 @@
>  #include <asm/apic.h>
>  #include <asm/setup.h>
>  #include <asm/uv/uv.h>
> +#include <asm/debugreg.h>
>  #include <linux/mc146818rtc.h>
>  
>  #include <asm/smpboot_hooks.h>
> @@ -326,6 +327,7 @@ notrace static void __cpuinit start_seco
>  	setup_secondary_clock();
>  
>  	wmb();
> +	load_debug_registers();
>  	cpu_idle();
>  }
>  
> @@ -1252,6 +1254,7 @@ void cpu_disable_common(void)
>  	remove_cpu_from_maps(cpu);
>  	unlock_vector_lock();
>  	fixup_irqs();
> +	hw_breakpoint_disable();
>  }
>  
>  int native_cpu_disable(void)
> Index: linux-2.6-tip.hbkpt/arch/x86/power/cpu_32.c
> ===================================================================
> --- linux-2.6-tip.hbkpt.orig/arch/x86/power/cpu_32.c
> +++ linux-2.6-tip.hbkpt/arch/x86/power/cpu_32.c
> @@ -13,6 +13,7 @@
>  #include <asm/mce.h>
>  #include <asm/xcr.h>
>  #include <asm/suspend.h>
> +#include <asm/debugreg.h>
>  
>  static struct saved_context saved_context;
>  
> @@ -48,6 +49,7 @@ static void __save_processor_state(struc
>  	ctxt->cr2 = read_cr2();
>  	ctxt->cr3 = read_cr3();
>  	ctxt->cr4 = read_cr4_safe();
> +	hw_breakpoint_disable();
>  }
>  
>  /* Needed by apm.c */
> @@ -80,19 +82,7 @@ static void fix_processor_context(void)
>  	load_TR_desc();				/* This does ltr */
>  	load_LDT(&current->active_mm->context);	/* This does lldt */
>  
> -	/*
> -	 * Now maybe reload the debug registers
> -	 */
> -	if (current->thread.debugreg7) {
> -		set_debugreg(current->thread.debugreg0, 0);
> -		set_debugreg(current->thread.debugreg1, 1);
> -		set_debugreg(current->thread.debugreg2, 2);
> -		set_debugreg(current->thread.debugreg3, 3);
> -		/* no 4 and 5 */
> -		set_debugreg(current->thread.debugreg6, 6);
> -		set_debugreg(current->thread.debugreg7, 7);
> -	}
> -
> +	load_debug_registers();


This part is breaking the things in the middle.

I think the conversion from thread.debugreg0/1/2/3 to an
array should be done completely early, when you add the headers.

The problem is that your patchset iterates through those two versions
of virtual debug registers, and those two version can't be carried
well while keeping the good granularity of your patchset.

You are dropping the old version of virtual debug registers from
cpu management in favour of the new version which uses the array through
wrappers, whereas the thread code still deals with the old version.

The result, which I haven't tried, is likely to break the things
if I don't apply further patches of this series.

Of course it will build, but the breakpoints won't work anymore
with cpu hotplug or suspend because the threads management still
uses the old version of registers.

I can't apply this patchset if it induces a know regression in
the middle otherwise bisection will become impossible.

I think you need to convert all sites from
thread->debugregX to thread->debugreg[X] early in
the first patch. It will also bring your patchset into
a better logic of progression changes.

Thanks,
Frederic.



>  }
>  
>  static void __restore_processor_state(struct saved_context *ctxt)
> Index: linux-2.6-tip.hbkpt/arch/x86/power/cpu_64.c
> ===================================================================
> --- linux-2.6-tip.hbkpt.orig/arch/x86/power/cpu_64.c
> +++ linux-2.6-tip.hbkpt/arch/x86/power/cpu_64.c
> @@ -16,6 +16,7 @@
>  #include <asm/mtrr.h>
>  #include <asm/xcr.h>
>  #include <asm/suspend.h>
> +#include <asm/debugreg.h>
>  
>  static void fix_processor_context(void);
>  
> @@ -71,6 +72,7 @@ static void __save_processor_state(struc
>  	ctxt->cr3 = read_cr3();
>  	ctxt->cr4 = read_cr4();
>  	ctxt->cr8 = read_cr8();
> +	hw_breakpoint_disable();
>  }
>  
>  void save_processor_state(void)
> @@ -159,16 +161,5 @@ static void fix_processor_context(void)
>  	load_TR_desc();				/* This does ltr */
>  	load_LDT(&current->active_mm->context);	/* This does lldt */
>  
> -	/*
> -	 * Now maybe reload the debug registers
> -	 */
> -	if (current->thread.debugreg7){
> -                loaddebug(&current->thread, 0);
> -                loaddebug(&current->thread, 1);
> -                loaddebug(&current->thread, 2);
> -                loaddebug(&current->thread, 3);
> -                /* no 4 and 5 */
> -                loaddebug(&current->thread, 6);
> -                loaddebug(&current->thread, 7);
> -	}
> +	load_debug_registers();
>  }
> 

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

Powered by blists - more mailing lists