[PATCH] Support for relocatable kdump kernel
Mohan Kumar M
mohan at in.ibm.com
Thu Oct 9 12:35:22 EDT 2008
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 at highest /* compute virtual base of kernel */
>> sldi r25,r25,32
>> - mr r3,r25
>> +#ifdef CONFIG_CRASH_DUMP
>> + ld r7,__kdump_flag at 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 at 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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
More information about the kexec
mailing list