Re: [v2] ppc64/book3s: fix branching to out of line handlers in relocation kernel

[Hari Bathini <hbathini@linux.vnet.ibm.com>]

On 03/30/2016 05:55 AM, Michael Ellerman wrote:
> On Tue, 2016-29-03 at 18:34:37 UTC, Hari Bathini wrote:
>> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
>> index 7716ceb..e598580 100644
>> --- a/arch/powerpc/kernel/exceptions-64s.S
>> +++ b/arch/powerpc/kernel/exceptions-64s.S
>> @@ -764,8 +764,8 @@ kvmppc_skip_Hinterrupt:
>>   #endif
>>   
>>   /*
>> - * Code from here down to __end_handlers is invoked from the
>> - * exception prologs above.  Because the prologs assemble the
>> + * Code from here down to end of out of line handlers is invoked from
>> + * the exception prologs above.  Because the prologs assemble the
> I think it would be better to just replace __end_handlers with __end_interrupts,
> that way it's entirely clear what location you're talking about.
>
>> @@ -953,11 +953,6 @@ hv_facility_unavailable_relon_trampoline:
>>   #endif
>>   	STD_RELON_EXCEPTION_PSERIES(0x5700, 0x1700, altivec_assist)
>>   
>> -	/* Other future vectors */
>> -	.align	7
>> -	.globl	__end_interrupts
>> -__end_interrupts:
>> -
>>   	.align	7
>>   system_call_entry:
>>   	b	system_call_common
>> @@ -1230,10 +1225,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_VSX)
>>   	STD_EXCEPTION_COMMON(0xf60, facility_unavailable, facility_unavailable_exception)
>>   	STD_EXCEPTION_COMMON(0xf80, hv_facility_unavailable, facility_unavailable_exception)
>>   
>> -	.align	7
>> -	.globl	__end_handlers
>> -__end_handlers:
>> -
> Sorry I wasn't clear in my last mail, please do this as a separate cleanup patch
> after this patch.

ok..

>> @@ -1244,6 +1235,16 @@ __end_handlers:
>>   	STD_RELON_EXCEPTION_PSERIES_OOL(0xf60, facility_unavailable)
>>   	STD_RELON_EXCEPTION_HV_OOL(0xf80, hv_facility_unavailable)
>>   
>> +	/* FIXME: For now, let us move the __end_interrupts marker down past
> Why is it FIXME?
>
> In general I don't want to merge code that adds a FIXME unless there is some
> very good reason.
>
> AFAICS this is a permanent solution isn't it?

Except for a few short interrupt vectors like 0x4f00, 04f20, etc., all other
vectors defined till __end_interrupts marker ensure that LOAD_HANDLER() is
used for branching to labels like system_call_entry, data_access_common, 
etc.
that are currently not copied to real 0 in relocation case.

So, we are forced to move the __end_interrupts marker down only to handle
space constraint in the short vectors. So, I added the FIXME to remind the
scope for improvement in the code. But after thinking over again now, moving
the marker down makes us copy an additional 1~2 KB along with the 21~22 KB
that we are copying already. So, not much of an improvement to lose 
sleep over
or to add a FIXME, I guess. Your thoughts?

Also, FIXME is the reason, why I did not replace __end_handlers with
__end_interrupts in the comment earlier.

>> +	 * the out-of-line handlers, to make sure we also copy OOL handlers
>> +	 * to real adress 0x100 when running a relocatable kernel. This helps
> It doesn't "help" it's 100% required.

Yep. Will change the wording.
Thanks for the review!

- Hari

>> +	 * in cases where interrupt vectors are not long enough (like 0x4f00,
>> +	 * 0x4f20, etc.) to branch out to OOL handlers with LOAD_HANDLER().
>> +	 */
>> +	.align	7
>> +	.globl	__end_interrupts
>> +__end_interrupts:
>> +
>>   #if defined(CONFIG_PPC_PSERIES) || defined(CONFIG_PPC_POWERNV)
>>   /*
>>    * Data area reserved for FWNMI option.
>
> cheers
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev