All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] ppc64/book3s: fix branching to out of line handlers in relocation kernel
@ 2016-03-30 18:19 Hari Bathini
  2016-04-01  6:14 ` Michael Ellerman
  0 siblings, 1 reply; 7+ messages in thread
From: Hari Bathini @ 2016-03-30 18:19 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: Michael Neuling, Ananth N Mavinakayanahalli, Mahesh J Salgaonkar,
	Paul Mackerras, Michael Ellerman, Benjamin Herrenschmidt

Some of the interrupt vectors on 64-bit POWER server processors  are
only 32 bytes long (8 instructions), which is not enough for the full
first-level interrupt handler. For these we need to branch to an out-
of-line (OOL) handler. But when we are running a relocatable kernel,
interrupt vectors till __end_interrupts marker are copied down to real
address 0x100. So, branching to labels (read OOL handlers) outside this
section should be handled differently (see LOAD_HANDLER()), considering
relocatable kernel, which would need atleast 4 instructions.

However, branching from interrupt vector means that we corrupt the CFAR
(come-from address register) on POWER7 and later processors as mentioned
in commit 1707dd16. So, EXCEPTION_PROLOG_0 (6 instructions) that contains
the part up to the point where the CFAR is saved in the PACA should be
part of the short interrupt vectors before we branch out to OOL handlers.

But as mentioned already, there are interrupt vectors on 64-bit POWER server
processors that are only 32 bytes long (like vectors 0x4f00, 0x4f20, etc.),
which cannot accomodate the above two cases at the same time owing to space
constraint. Currently, in these interrupt vectors, we simply branch out to
OOL handlers, without using LOAD_HANDLER(), which leaves us vulnerable when
running a relocatable kernel (eg. kdump case). While this has been the case
for sometime now and kdump is used widely, we were fortunate not to see any
problems so far, for three reasons:

    1. In almost all cases, production kernel (relocatable) is used for
       kdump as well, which would mean that crashed kernel's OOL handler
       would be at the same place where we endup branching to, from short
       interrupt vector of kdump kernel.
    2. Also, OOL handler was unlikely the reason for crash in almost all
       the kdump scenarios, which meant we had a sane OOL handler from
       crashed kernel that we branched to.
    3. On most 64-bit POWER server processors, page size is large enough
       that marking interrupt vector code as executable (see commit
       429d2e83) leads to marking OOL handler code from crashed kernel,
       that sits right below interrupt vector code from kdump kernel, as
       executable as well.

Let us fix this undependable code path by moving these OOL handlers below
__end_interrupts marker to make sure we also copy these handlers to real
address 0x100 when running a relocatable kernel. Because the interrupt
vectors branching to these OOL handlers are not long enough to use
LOAD_HANDLER() for branching as discussed above.

This fix has been tested successfully in kdump scenario, on a lpar with 4K page
size by using different default/production kernel and kdump kernel.

Signed-off-by: Hari Bathini <hbathini@linux.vnet.ibm.com>
Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
---

changes from v2:
2. Move the OOL handlers before __end_interrupts marker instead of moving the __end_interrupts marker
3. Leave __end_handlers marker as is.

 arch/powerpc/kernel/exceptions-64s.S |   29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 7716ceb..9ac3a38 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -953,6 +953,25 @@ hv_facility_unavailable_relon_trampoline:
 #endif
 	STD_RELON_EXCEPTION_PSERIES(0x5700, 0x1700, altivec_assist)
 
+	/*
+	 * Out-Of-Line handlers for relocation-on interrupt vectors
+	 *
+	 * We need these OOL handlers to be below __end_interrupts
+	 * marker to enusre we also copy these OOL handlers along
+	 * with the interrupt vectors to real address 0x100 when
+	 * running a relocatable kernel. Because the interrupt
+	 * vectors branching to these OOL handlers are not long
+	 * enough to use LOAD_HANDLER() for branching.
+	 */
+	STD_RELON_EXCEPTION_HV_OOL(0xe40, emulation_assist)
+	MASKABLE_RELON_EXCEPTION_HV_OOL(0xe80, h_doorbell)
+
+	STD_RELON_EXCEPTION_PSERIES_OOL(0xf00, performance_monitor)
+	STD_RELON_EXCEPTION_PSERIES_OOL(0xf20, altivec_unavailable)
+	STD_RELON_EXCEPTION_PSERIES_OOL(0xf40, vsx_unavailable)
+	STD_RELON_EXCEPTION_PSERIES_OOL(0xf60, facility_unavailable)
+	STD_RELON_EXCEPTION_HV_OOL(0xf80, hv_facility_unavailable)
+
 	/* Other future vectors */
 	.align	7
 	.globl	__end_interrupts
@@ -1234,16 +1253,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_VSX)
 	.globl	__end_handlers
 __end_handlers:
 
-	/* Equivalents to the above handlers for relocation-on interrupt vectors */
-	STD_RELON_EXCEPTION_HV_OOL(0xe40, emulation_assist)
-	MASKABLE_RELON_EXCEPTION_HV_OOL(0xe80, h_doorbell)
-
-	STD_RELON_EXCEPTION_PSERIES_OOL(0xf00, performance_monitor)
-	STD_RELON_EXCEPTION_PSERIES_OOL(0xf20, altivec_unavailable)
-	STD_RELON_EXCEPTION_PSERIES_OOL(0xf40, vsx_unavailable)
-	STD_RELON_EXCEPTION_PSERIES_OOL(0xf60, facility_unavailable)
-	STD_RELON_EXCEPTION_HV_OOL(0xf80, hv_facility_unavailable)
-
 #if defined(CONFIG_PPC_PSERIES) || defined(CONFIG_PPC_POWERNV)
 /*
  * Data area reserved for FWNMI option.

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] ppc64/book3s: fix branching to out of line handlers in relocation kernel
  2016-03-30 18:19 [PATCH v3] ppc64/book3s: fix branching to out of line handlers in relocation kernel Hari Bathini
@ 2016-04-01  6:14 ` Michael Ellerman
  2016-04-01  6:37   ` Gabriel Paubert
  2016-04-01  6:53   ` Hari Bathini
  0 siblings, 2 replies; 7+ messages in thread
From: Michael Ellerman @ 2016-04-01  6:14 UTC (permalink / raw)
  To: Hari Bathini, linuxppc-dev
  Cc: Michael Neuling, Ananth N Mavinakayanahalli, Mahesh J Salgaonkar,
	Paul Mackerras, Benjamin Herrenschmidt

On Wed, 2016-03-30 at 23:49 +0530, Hari Bathini wrote:
> Some of the interrupt vectors on 64-bit POWER server processors  are
> only 32 bytes long (8 instructions), which is not enough for the full
...
> Let us fix this undependable code path by moving these OOL handlers below
> __end_interrupts marker to make sure we also copy these handlers to real
> address 0x100 when running a relocatable kernel. Because the interrupt
> vectors branching to these OOL handlers are not long enough to use
> LOAD_HANDLER() for branching as discussed above.
> 
...
> changes from v2:
> 2. Move the OOL handlers before __end_interrupts marker instead of moving the __end_interrupts marker
> 3. Leave __end_handlers marker as is.

Hi Hari,

Thanks for trying this. In the end I've decided it's not a good option.

If you build an allmodconfig, and turn on CONFIG_RELOCATABLE, and then look at
the disassembly, you see this:

  c000000000006ffc:       48 00 29 04     b       c000000000009900 <.ret_from_except>
  
  c000000000007000 <__end_handlers>:

At 0x7000 we have the FWNMI area, which is fixed and can't move. As you see
above we end up with only 4 bytes of space between the end of the handlers and
the FWNMI area.

So any tiny change that adds two more instructions prior to 0x7000 will then
fail to build.

None of that's your fault, it's just the nature of the code in there, it's very
space constrained.

For now I'll take your v2, but I'll edit the comment and drop the removal of
__end_handlers.

cheers

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] ppc64/book3s: fix branching to out of line handlers in relocation kernel
  2016-04-01  6:14 ` Michael Ellerman
@ 2016-04-01  6:37   ` Gabriel Paubert
  2016-04-01 10:40     ` Michael Ellerman
  2016-04-01  6:53   ` Hari Bathini
  1 sibling, 1 reply; 7+ messages in thread
From: Gabriel Paubert @ 2016-04-01  6:37 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Hari Bathini, linuxppc-dev, Mahesh J Salgaonkar, Michael Neuling,
	Paul Mackerras

    Hi Michael,

On Fri, Apr 01, 2016 at 05:14:35PM +1100, Michael Ellerman wrote:
> On Wed, 2016-03-30 at 23:49 +0530, Hari Bathini wrote:
> > Some of the interrupt vectors on 64-bit POWER server processors  are
> > only 32 bytes long (8 instructions), which is not enough for the full
> ...
> > Let us fix this undependable code path by moving these OOL handlers below
> > __end_interrupts marker to make sure we also copy these handlers to real
> > address 0x100 when running a relocatable kernel. Because the interrupt
> > vectors branching to these OOL handlers are not long enough to use
> > LOAD_HANDLER() for branching as discussed above.
> > 
> ...
> > changes from v2:
> > 2. Move the OOL handlers before __end_interrupts marker instead of moving the __end_interrupts marker
> > 3. Leave __end_handlers marker as is.
> 
> Hi Hari,
> 
> Thanks for trying this. In the end I've decided it's not a good option.
> 
> If you build an allmodconfig, and turn on CONFIG_RELOCATABLE, and then look at
> the disassembly, you see this:
> 
>   c000000000006ffc:       48 00 29 04     b       c000000000009900 <.ret_from_except>
>   
>   c000000000007000 <__end_handlers>:
> 
> At 0x7000 we have the FWNMI area, which is fixed and can't move. As you see
> above we end up with only 4 bytes of space between the end of the handlers and
> the FWNMI area.

Nitpicking a bit, if I correctly read the above disassembly and there is an instuction
at 0x6ffc, the free space is exactly 0! 

> 
> So any tiny change that adds two more instructions prior to 0x7000 will then
> fail to build.

Even one instruction provided I still know how to count.

> 
> None of that's your fault, it's just the nature of the code in there, it's very
> space constrained.

Calling it space very constrained makes you win the understatement of the month 
award, on April fool's day :-)

    Regards,
    Gabriel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] ppc64/book3s: fix branching to out of line handlers in relocation kernel
  2016-04-01  6:14 ` Michael Ellerman
  2016-04-01  6:37   ` Gabriel Paubert
@ 2016-04-01  6:53   ` Hari Bathini
  2016-04-01 10:37     ` Michael Ellerman
  1 sibling, 1 reply; 7+ messages in thread
From: Hari Bathini @ 2016-04-01  6:53 UTC (permalink / raw)
  To: Michael Ellerman, linuxppc-dev
  Cc: Michael Neuling, Ananth N Mavinakayanahalli, Mahesh J Salgaonkar,
	Paul Mackerras, Benjamin Herrenschmidt



On 04/01/2016 11:44 AM, Michael Ellerman wrote:
> On Wed, 2016-03-30 at 23:49 +0530, Hari Bathini wrote:
>> Some of the interrupt vectors on 64-bit POWER server processors  are
>> only 32 bytes long (8 instructions), which is not enough for the full
> ...
>> Let us fix this undependable code path by moving these OOL handlers below
>> __end_interrupts marker to make sure we also copy these handlers to real
>> address 0x100 when running a relocatable kernel. Because the interrupt
>> vectors branching to these OOL handlers are not long enough to use
>> LOAD_HANDLER() for branching as discussed above.
>>
> ...
>> changes from v2:
>> 2. Move the OOL handlers before __end_interrupts marker instead of moving the __end_interrupts marker
>> 3. Leave __end_handlers marker as is.
> Hi Hari,
>
> Thanks for trying this. In the end I've decided it's not a good option.
>
> If you build an allmodconfig, and turn on CONFIG_RELOCATABLE, and then look at
> the disassembly, you see this:
>
>    c000000000006ffc:       48 00 29 04     b       c000000000009900 <.ret_from_except>
>    
>    c000000000007000 <__end_handlers>:
>
> At 0x7000 we have the FWNMI area, which is fixed and can't move. As you see
> above we end up with only 4 bytes of space between the end of the handlers and
> the FWNMI area.
>
> So any tiny change that adds two more instructions prior to 0x7000 will then
> fail to build.

Hi Michael,

I agree. But the OOL handlers that are moved up in v3 were below
0x7000 earlier as well and moving them below __end_interrupts marker
shouldn't make any difference in terms of space consumption at least in
comparison between v2 & v3. So, I guess picking either v2 or v3
doesn't change this for better.

Also, there is code between __end_interrupts and __end_handlers
that is not location dependent as long as it is within 64K (0x10000)
that can be moved above 0x8000, if need be.

For these reasons, I feel v3 is better going forward as it keeps
__start_interrupts to __end_interrupts code compact and
leaves alone the code that doesn't need to be copied to real 0.

Am I missing something here?

Thanks
Hari

> None of that's your fault, it's just the nature of the code in there, it's very
> space constrained.
>
> For now I'll take your v2, but I'll edit the comment and drop the removal of
> __end_handlers.
>
> cheers
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] ppc64/book3s: fix branching to out of line handlers in relocation kernel
  2016-04-01  6:53   ` Hari Bathini
@ 2016-04-01 10:37     ` Michael Ellerman
  2016-04-01 19:41       ` Hari Bathini
  0 siblings, 1 reply; 7+ messages in thread
From: Michael Ellerman @ 2016-04-01 10:37 UTC (permalink / raw)
  To: Hari Bathini, linuxppc-dev
  Cc: Michael Neuling, Ananth N Mavinakayanahalli, Mahesh J Salgaonkar,
	Paul Mackerras, Benjamin Herrenschmidt

On Fri, 2016-04-01 at 12:23 +0530, Hari Bathini wrote:
> 
> On 04/01/2016 11:44 AM, Michael Ellerman wrote:
> > On Wed, 2016-03-30 at 23:49 +0530, Hari Bathini wrote:
> > > Some of the interrupt vectors on 64-bit POWER server processors  are
> > > only 32 bytes long (8 instructions), which is not enough for the full
> > ...
> > > Let us fix this undependable code path by moving these OOL handlers below
> > > __end_interrupts marker to make sure we also copy these handlers to real
> > > address 0x100 when running a relocatable kernel. Because the interrupt
> > > vectors branching to these OOL handlers are not long enough to use
> > > LOAD_HANDLER() for branching as discussed above.
> > > 
> > ...
> > > changes from v2:
> > > 2. Move the OOL handlers before __end_interrupts marker instead of moving the __end_interrupts marker
> > > 3. Leave __end_handlers marker as is.
> > Hi Hari,
> > 
> > Thanks for trying this. In the end I've decided it's not a good option.
> > 
> > If you build an allmodconfig, and turn on CONFIG_RELOCATABLE, and then look at
> > the disassembly, you see this:
> > 
> >    c000000000006ffc:       48 00 29 04     b       c000000000009900 <.ret_from_except>
> >    
> >    c000000000007000 <__end_handlers>:
> > 
> > At 0x7000 we have the FWNMI area, which is fixed and can't move. As you see
> > above we end up with only 4 bytes of space between the end of the handlers and
> > the FWNMI area.
> > 
> > So any tiny change that adds two more instructions prior to 0x7000 will then
> > fail to build.
> 
> Hi Michael,
> 
> I agree. But the OOL handlers that are moved up in v3 were below
> 0x7000 earlier as well and moving them below __end_interrupts marker
> shouldn't make any difference in terms of space consumption at least in
> comparison between v2 & v3. So, I guess picking either v2 or v3
> doesn't change this for better.

It does make a difference, due to alignment. Prior to your patch we have ~24
bytes free.

> Also, there is code between __end_interrupts and __end_handlers
> that is not location dependent as long as it is within 64K (0x10000)
> that can be moved above 0x8000, if need be.
 
That's true, but that sort of change is unlikely to backport well. And we need
to backport this fix to everything.

But if you can get that to work I'll consider it. I tried quickly but couldn't
get it working, due to problems with the feature else sections being too far
away from.

cheers

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] ppc64/book3s: fix branching to out of line handlers in relocation kernel
  2016-04-01  6:37   ` Gabriel Paubert
@ 2016-04-01 10:40     ` Michael Ellerman
  0 siblings, 0 replies; 7+ messages in thread
From: Michael Ellerman @ 2016-04-01 10:40 UTC (permalink / raw)
  To: Gabriel Paubert
  Cc: Hari Bathini, linuxppc-dev, Mahesh J Salgaonkar, Michael Neuling,
	Paul Mackerras

On Fri, 2016-04-01 at 08:37 +0200, Gabriel Paubert wrote:
> On Fri, Apr 01, 2016 at 05:14:35PM +1100, Michael Ellerman wrote:
> > If you build an allmodconfig, and turn on CONFIG_RELOCATABLE, and then look at
> > the disassembly, you see this:
> > 
> >   c000000000006ffc:       48 00 29 04     b       c000000000009900 <.ret_from_except>
> >   
> >   c000000000007000 <__end_handlers>:
> > 
> > At 0x7000 we have the FWNMI area, which is fixed and can't move. As you see
> > above we end up with only 4 bytes of space between the end of the handlers and
> > the FWNMI area.
> 
> Nitpicking a bit, if I correctly read the above disassembly and there is an instuction
> at 0x6ffc, the free space is exactly 0! 

Well spotted! It was of course an April fools .. joke ? :)

> > None of that's your fault, it's just the nature of the code in there, it's very
> > space constrained.
> 
> Calling it space very constrained makes you win the understatement of the month 
> award, on April fool's day :-)

Well there are some holes here and there, so we could write two instructions,
then branch to the next hole, five more instructions, branch to the next hole
etc. But that makes for hard to read code :)

cheers

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] ppc64/book3s: fix branching to out of line handlers in relocation kernel
  2016-04-01 10:37     ` Michael Ellerman
@ 2016-04-01 19:41       ` Hari Bathini
  0 siblings, 0 replies; 7+ messages in thread
From: Hari Bathini @ 2016-04-01 19:41 UTC (permalink / raw)
  To: Michael Ellerman, linuxppc-dev
  Cc: Michael Neuling, Ananth N Mavinakayanahalli, Mahesh J Salgaonkar,
	Paul Mackerras, Benjamin Herrenschmidt



On 04/01/2016 04:07 PM, Michael Ellerman wrote:
> On Fri, 2016-04-01 at 12:23 +0530, Hari Bathini wrote:
>> On 04/01/2016 11:44 AM, Michael Ellerman wrote:
>>> On Wed, 2016-03-30 at 23:49 +0530, Hari Bathini wrote:
>>>> Some of the interrupt vectors on 64-bit POWER server processors  are
>>>> only 32 bytes long (8 instructions), which is not enough for the full
>>> ...
>>>> Let us fix this undependable code path by moving these OOL handlers below
>>>> __end_interrupts marker to make sure we also copy these handlers to real
>>>> address 0x100 when running a relocatable kernel. Because the interrupt
>>>> vectors branching to these OOL handlers are not long enough to use
>>>> LOAD_HANDLER() for branching as discussed above.
>>>>
>>> ...
>>>> changes from v2:
>>>> 2. Move the OOL handlers before __end_interrupts marker instead of moving the __end_interrupts marker
>>>> 3. Leave __end_handlers marker as is.
>>> Hi Hari,
>>>
>>> Thanks for trying this. In the end I've decided it's not a good option.
>>>
>>> If you build an allmodconfig, and turn on CONFIG_RELOCATABLE, and then look at
>>> the disassembly, you see this:
>>>
>>>     c000000000006ffc:       48 00 29 04     b       c000000000009900 <.ret_from_except>
>>>     
>>>     c000000000007000 <__end_handlers>:
>>>
>>> At 0x7000 we have the FWNMI area, which is fixed and can't move. As you see
>>> above we end up with only 4 bytes of space between the end of the handlers and
>>> the FWNMI area.
>>>
>>> So any tiny change that adds two more instructions prior to 0x7000 will then
>>> fail to build.
>> Hi Michael,
>>
>> I agree. But the OOL handlers that are moved up in v3 were below
>> 0x7000 earlier as well and moving them below __end_interrupts marker
>> shouldn't make any difference in terms of space consumption at least in
>> comparison between v2 & v3. So, I guess picking either v2 or v3
>> doesn't change this for better.
> It does make a difference, due to alignment. Prior to your patch we have ~24
> bytes free.

Hi Michael,

Hmmm.. I thought ~24 bytes was not such a difference but with the scenario
you mentioned it does sound critical. Actually, this patch came into being
for want of another 8~12 bytes. So, I should have known better about
space constraint.

>
>> Also, there is code between __end_interrupts and __end_handlers
>> that is not location dependent as long as it is within 64K (0x10000)
>> that can be moved above 0x8000, if need be.
> That's true, but that sort of change is unlikely to backport well. And we need
> to backport this fix to everything.

That does sound like a maintainer's nightmare.

> But if you can get that to work I'll consider it. I tried quickly but couldn't
> get it working, due to problems with the feature else sections being too far
> away from.

Same case. May need sometime to get that right.
Also, exploring holes between __start_interrupts & __end_interrupts.
Will try and get back on this soon.
If none of this works, we have v2 anyway.

Thanks
Hari

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-04-01 19:42 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-30 18:19 [PATCH v3] ppc64/book3s: fix branching to out of line handlers in relocation kernel Hari Bathini
2016-04-01  6:14 ` Michael Ellerman
2016-04-01  6:37   ` Gabriel Paubert
2016-04-01 10:40     ` Michael Ellerman
2016-04-01  6:53   ` Hari Bathini
2016-04-01 10:37     ` Michael Ellerman
2016-04-01 19:41       ` Hari Bathini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.