Re: [PATCH] Support for relocatable kdump kernel

[Mohan Kumar M <mohan@in.ibm.com>]

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


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec