Message ID | 20160329183333.15267.29181.stgit@hbathini.in.ibm.com (mailing list archive) |
---|---|
State | Accepted |
Headers | show |
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
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
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 */ >>> - .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? > 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(). >>> + */ >>> + .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
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
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
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
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 <hbathini@linux.vnet.ibm.com> > Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com> Applied to powerpc next with some modifications as discussed, thanks. https://git.kernel.org/powerpc/c/8ed8ab40047a570fdd8043a40c cheers
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 * addresses of these handlers using the LOAD_HANDLER macro, * which uses an ori instruction, these handlers must be in * the first 64k of the kernel image. @@ -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: - /* 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) @@ -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 + * the out-of-line handlers, to make sure we also copy OOL handlers + * to real adress 0x100 when running a relocatable kernel. This helps + * 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.