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

[Michael Ellerman]

On Tue, 2016-29-03 at 18:34:37 UTC, 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
> 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.
...
> 
> Signed-off-by: Hari Bathini 
> Signed-off-by: Mahesh Salgaonkar 

Applied to powerpc next with some modifications as discussed, thanks.

https://git.kernel.org/powerpc/c/8ed8ab40047a570fdd8043a40c

cheers
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Hari Bathini]

On 03/30/2016 04:47 PM, Michael Ellerman wrote:

On Wed, 2016-03-30 at 13:14 +0530, Hari Bathini wrote:

Alternatively, how about moving the OOLs handlers that can't be branched with
LOAD_HANDLER under __end_interrupts. This way we won't be copying more than a
few absolutely needed handlers.

STD_RELON_EXCEPTION_HV_OOL(0xe40, emulation_assist)
.
.
STD_RELON_EXCEPTION_HV_OOL(0xf80, hv_facility_unavailable)


We can leave __end_handlers marker to indicate code that should be part
of the first 64K of kernel image.

That might work. But I suspect you will run into issues with ".org backwards",
ie. running out of space in head_64.S

But try it and let me know if it works.


It worked. Doing some sanity testing.
Will post v3 soon with this approach.


I think we also need to write a script or little C program which looks at the
vmlinux and checks that nothing below __end_whatever does a direct branch. So
that we don't break it again in future.


Yep. That would make life easy..
Let me see if I can do something about it.

Thanks
Hari

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Michael Ellerman]

On Wed, 2016-03-30 at 12:44 +0530, Hari Bathini wrote:
> On 03/30/2016 05:55 AM, Michael Ellerman wrote:
> > On Tue, 2016-29-03 at 18:34:37 UTC, Hari Bathini wrote:
> > > @@ -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?

OK, that makes sense. I still wouldn't add a FIXME unless you have a plan to
fix it "properly" in the medium term. And I don't think we really do.

So if we merge it with a FIXME there's a good chance the FIXME will be there in
10 years, which is not really helpful. A comment saying "we only need this here
because of xx, and we could move it to y if z happened" is helpful.

As far as the size of the copy, that doesn't really concern me at all. What I
want is a solution that's robust into the future. This has been badly broken
for quite a while and we didn't notice :/

cheers

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Michael Ellerman]

On Wed, 2016-03-30 at 13:14 +0530, Hari Bathini wrote:
> 
> Alternatively, how about moving the OOLs handlers that can't be branched with
> LOAD_HANDLER under __end_interrupts. This way we won't be copying more than a
> few absolutely needed handlers.
> 
> STD_RELON_EXCEPTION_HV_OOL(0xe40, emulation_assist)
> .
> .
> STD_RELON_EXCEPTION_HV_OOL(0xf80, hv_facility_unavailable)
> 
> 
> We can leave __end_handlers marker to indicate code that should be part 
> of the first 64K of kernel image.

That might work. But I suspect you will run into issues with ".org backwards",
ie. running out of space in head_64.S

But try it and let me know if it works.

I think we also need to write a script or little C program which looks at the
vmlinux and checks that nothing below __end_whatever does a direct branch. So
that we don't break it again in future.

cheers

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Hari Bathini]

On 03/30/2016 12:44 PM, Hari Bathini wrote:



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 */
-.align7
-.globl__end_interrupts
-__end_interrupts:
-
  .align7
  system_call_entry:
  bsystem_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)

  -.align7
-.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?



Alternatively, how about moving the OOLs handlers that can't be branched 
with LOAD_HANDLER
under __end_interrupts. This way we won't be copying more than a few 
absolutely needed handlers.


STD_RELON_EXCEPTION_HV_OOL(0xe40, emulation_assist)
.
.
STD_RELON_EXCEPTION_HV_OOL(0xf80, hv_facility_unavailable)


We can leave __end_handlers marker to indicate code that should be part 
of the

first 64K of kernel image.

Thanks
Hari


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().

+ */
+.align7
+.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


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Hari Bathini]

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


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

[Michael Ellerman]

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.

> @@ -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?

> +  * 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.

> +  * 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