Re: [PATCH 4/5] Relocation support

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

Paul Mackerras wrote:
> Mohan Kumar M writes:
>
Hi Paul,

Thanks for your comments.

I will update the patches as per your comment and will give a detailed 
description for each patch.

Regards,
Mohan.


> 
>>  static inline int in_kernel_text(unsigned long addr)
>>  {
>> -	if (addr >= (unsigned long)_stext && addr < (unsigned long)__init_end)
>> +	if (addr >= (unsigned long)_stext && addr < (unsigned long)__init_end
>> +								+ kernel_base)
> 
> Your patch adds kernel_base to some addresses but not to all of them,
> so your patch description should have told us why you added it in the
> those places and not others.  If you tell us the general principle
> you're following (even if it seems obvious to you) it will be useful
> to people chasing bugs or adding new code later on, or even just
> trying to understand what the code does.
> 
>> -	RELOC(alloc_bottom) = PAGE_ALIGN((unsigned long)&RELOC(_end) + 0x4000);
>> +#ifndef CONFIG_RELOCATABLE_PPC64
>> + 	RELOC(alloc_bottom) = PAGE_ALIGN((unsigned long)&RELOC(_end) + 0x4000);
>> +#else
>> +	RELOC(alloc_bottom) = PAGE_ALIGN((unsigned long)&RELOC(_end) + 0x4000 +
>> +							RELOC(reloc_delta));
>> +#endif
> 
> Ifdefs in code inside a function are frowned upon in the Linux
> kernel.  Try to find an alternative way to do this, such as ensuring
> that reloc_delta is 0 when CONFIG_RELOCATABLE_PPC64 is not set.
> Also it's not clear (to me at least) why you need to add reloc_data in
> the relocatable case.
> 
>> +#ifndef CONFIG_RELOCATABLE_PPC64
>>  	unsigned long *spinloop
>>  		= (void *) LOW_ADDR(__secondary_hold_spinloop);
>>  	unsigned long *acknowledge
>>  		= (void *) LOW_ADDR(__secondary_hold_acknowledge);
>> +#else
>> +	unsigned long *spinloop
>> +		= (void *) &__secondary_hold_spinloop;
>> +	unsigned long *acknowledge
>> +		= (void *) &__secondary_hold_acknowledge;
>> +#endif
> 
> This also needs some explanation.  (Put it in the patch description or
> in a comment in the code, not in a reply to this mail. :)
> 
>> +#ifndef CONFIG_RELOCATABLE_PPC64
>>  	ld	r4,htab_hash_mask@got(2)
>> +#else
>> +	LOAD_REG_IMMEDIATE(r4,htab_hash_mask)
>> +#endif
>>  	ld	r27,0(r4)	/* htab_hash_mask -> r27 */
> 
> Here and in the other similar places, I would prefer you just changed
> it to LOAD_REG_ADDR and not have any ifdef.
> 
> Paul.