From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from e38.co.us.ibm.com (e38.co.us.ibm.com [32.97.110.159]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "e38.co.us.ibm.com", Issuer "Equifax" (verified OK)) by ozlabs.org (Postfix) with ESMTPS id 5023ADE303 for ; Fri, 10 Oct 2008 03:35:43 +1100 (EST) Received: from d03relay02.boulder.ibm.com (d03relay02.boulder.ibm.com [9.17.195.227]) by e38.co.us.ibm.com (8.13.1/8.13.1) with ESMTP id m99GZ6T7019160 for ; Thu, 9 Oct 2008 10:35:06 -0600 Received: from d03av01.boulder.ibm.com (d03av01.boulder.ibm.com [9.17.195.167]) by d03relay02.boulder.ibm.com (8.13.8/8.13.8/NCO v9.1) with ESMTP id m99GZYeA213434 for ; Thu, 9 Oct 2008 10:35:35 -0600 Received: from d03av01.boulder.ibm.com (loopback [127.0.0.1]) by d03av01.boulder.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id m99GZTMu019347 for ; Thu, 9 Oct 2008 10:35:29 -0600 Message-ID: <48EE32CA.5070401@in.ibm.com> Date: Thu, 09 Oct 2008 22:05:22 +0530 From: Mohan Kumar M MIME-Version: 1.0 To: Paul Mackerras Subject: Re: [PATCH] Support for relocatable kdump kernel References: <20081001182645.GB20319@in.ibm.com> <18669.38444.112286.785452@cargo.ozlabs.ibm.com> In-Reply-To: <18669.38444.112286.785452@cargo.ozlabs.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Cc: ppcdev , kexec@lists.infradead.org List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Hi Paul, Thank you for the review. I will implement the changes you suggested and send the patches. Regards, Mohan. > Mohan Kumar M writes: > >> Support for relocatable kdump kernel > > [snip] > >> @@ -1384,7 +1392,15 @@ _STATIC(__after_prom_start) >> /* process relocations for the final address of the kernel */ >> lis r25,PAGE_OFFSET@highest /* compute virtual base of kernel */ >> sldi r25,r25,32 >> - mr r3,r25 >> +#ifdef CONFIG_CRASH_DUMP >> + ld r7,__kdump_flag@got(r2) >> + add r7,r7,r26 >> + ld r7,0(r7) > > I think it is dangerous to use an address from the GOT at this point > when we haven't called relocate() yet. In fact those 3 instructions > can be replaced by one: > > ld r7,__kdump_flag-_stext(r26) > > since we have our base address (i.e. the address of _stext) in r26 at > this point. > >> +#ifdef CONFIG_RELOCATABLE >> +#ifdef CONFIG_CRASH_DUMP >> +/* >> + * Check if the kernel has to be running as relocatable kernel based on the >> + * variable __kdump_flag, if it is set the kernel is treated as relocatble >> + * kernel, otherwise it will be moved to PHYSICAL_START >> + */ >> + ld r7,__kdump_flag@got(r2) >> + ld r7,0(r7) > > Here again I would rather you did > > ld r7,__kdump_flag-_stext(r26) > > since the kernel is relocated for its final location by this point, > but it is not running at the final location yet. > >> + cmpldi cr0,r7,1 >> + bne regular >> + >> + li r5,__end_interrupts - _stext /* just copy interrupts */ >> + b 5f >> +regular: >> +#endif >> + lis r5,(copy_to_here - _stext)@ha >> + addi r5,r5,(copy_to_here - _stext)@l /* # bytes of memory to copy */ >> >> bl .copy_and_flush /* copy the first n bytes */ >> /* this includes the code being */ >> @@ -1411,15 +1443,33 @@ _STATIC(__after_prom_start) >> mtctr r8 >> bctr >> >> +p_end: .llong _end - _stext >> + >> 4: /* Now copy the rest of the kernel up to _end */ >> addis r5,r26,(p_end - _stext)@ha >> ld r5,(p_end - _stext)@l(r5) /* get _end */ >> - bl .copy_and_flush /* copy the rest */ >> +#else >> + lis r5,(copy_to_here - _stext)@ha >> + addi r5,r5,(copy_to_here - _stext)@l /* # bytes of memory to copy */ >> >> -9: b .start_here_multiplatform >> + bl .copy_and_flush /* copy the first n bytes */ >> + /* this includes the code being */ >> + /* executed here. */ >> + addis r8,r3,(4f - _stext)@ha /* Jump to the copy of this code */ >> + addi r8,r8,(4f - _stext)@l /* that we just made */ >> + mtctr r8 >> + bctr >> >> p_end: .llong _end - _stext >> >> +4: /* Now copy the rest of the kernel up to _end */ >> + addis r5,r26,(p_end - _stext)@ha >> + ld r5,(p_end - _stext)@l(r5) /* get _end */ >> +#endif >> +5: bl .copy_and_flush /* copy the rest */ >> + >> +9: b .start_here_multiplatform > > You have ended up with two separate copies of the code here depending > on whether or not we have CONFIG_RELOCATABLE set. I don't think the > code paths should be different to such an extent. Please try to make > the ifdef as small as possible (ideally, nonexistent). > > Paul. > > _______________________________________________ > kexec mailing list > kexec@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/kexec